[loader-v2] Addressing TODOs (#15316) (#15342)

- Switching from undefined to script location
- Keeping error remapping because the status code exists on-chain.
   It is probably fine t keep it as is.
- Removed useless TODO for alerts on concurrent manager uses.
- Changed errors.rs TODO into a note, as it is not a P0/P1/P2 to fix.

(cherry picked from commit dbdb613)

Co-authored-by: George Mitenkov <[email protected]>

aptos-move/block-executor/src/code_cache_global_manager.rs

@@ -173,7 +173,6 @@ impl AptosModuleCacheManager {
                 AptosModuleCacheManagerGuard::Guard { guard }
             },
             None => {
-                // TODO(loader_v2): Should we return an error here instead?
                 alert_or_println!("Locking module cache manager failed, fallback to empty caches");
 
                 // If this is true, we failed to acquire a lock, and so default storage environment

third_party/move/move-vm/integration-tests/src/tests/bad_storage_tests.rs

@@ -792,8 +792,7 @@ fn test_storage_returns_bogus_error_when_loading_resource() {
             .unwrap_err();
 
         if *error_code == StatusCode::UNKNOWN_VERIFICATION_ERROR {
-            // MoveVM maps `UNKNOWN_VERIFICATION_ERROR` to `VERIFICATION_ERROR` in
-            // `maybe_core_dump` function in interpreter.rs.
+            // MoveVM maps `UNKNOWN_VERIFICATION_ERROR` to `VERIFICATION_ERROR`.
             assert_eq!(err.major_status(), StatusCode::VERIFICATION_ERROR);
         } else {
             assert_eq!(err.major_status(), *error_code);

third_party/move/move-vm/runtime/src/loader/mod.rs

@@ -1303,8 +1303,7 @@ impl<'a> Resolver<'a> {
                     function_name,
                 )
                 .map_err(|_| {
-                    // TODO(loader_v2):
-                    //   Loader V1 implementation uses this error, but it should be a linker error.
+                    // Note: legacy loader implementation used this error, so we need to remap.
                     PartialVMError::new(StatusCode::FUNCTION_RESOLUTION_FAILURE).with_message(
                         format!(
                             "Module or function do not exist for {}::{}::{}",

third_party/move/move-vm/runtime/src/storage/loader.rs

@@ -139,14 +139,11 @@ impl LoaderV2 {
             .iter()
             .map(|ty_tag| self.load_ty(code_storage, ty_tag))
             .collect::<PartialVMResult<Vec<_>>>()
-            // TODO(loader_v2):
-            //   Loader V1 implementation returns undefined here, causing some tests to fail. We
-            //   should probably map this to script.
-            .map_err(|e| e.finish(Location::Undefined))?;
+            .map_err(|err| err.finish(Location::Script))?;
 
         let main = script.entry_point();
         Type::verify_ty_arg_abilities(main.ty_param_abilities(), &ty_args)
-            .map_err(|e| e.finish(Location::Script))?;
+            .map_err(|err| err.finish(Location::Script))?;
 
         Ok(LoadedFunction {
             owner: LoadedFunctionOwner::Script(script),
@@ -205,7 +202,7 @@ impl LoaderV2 {
     ) -> PartialVMResult<Arc<StructType>> {
         let module = self
             .load_module(module_storage, address, module_name)
-            .map_err(|e| e.to_partial())?;
+            .map_err(|err| err.to_partial())?;
         Ok(module
             .struct_map
             .get(struct_name)
@@ -239,9 +236,9 @@ impl LoaderV2 {
                     st.module.as_ident_str(),
                     st.name.as_ident_str(),
                 )
-                .map_err(|e| e.finish(Location::Undefined))
+                .map_err(|err| err.finish(Location::Undefined))
             })
-            .map_err(|e| e.to_partial())
+            .map_err(|err| err.to_partial())
     }
 }
 

third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_few_type_args_inner.exp

@@ -4,7 +4,7 @@ task 1 'run'. lines 6-10:
 Error: Script execution failed with VMError: {
     major_status: NUMBER_OF_TYPE_ARGUMENTS_MISMATCH,
     sub_status: None,
-    location: undefined,
+    location: script,
     indices: [],
     offsets: [],
 }

third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_few_type_args_inner.v2_exp

@@ -4,7 +4,7 @@ task 1 'run'. lines 6-10:
 Error: Script execution failed with VMError: {
     major_status: NUMBER_OF_TYPE_ARGUMENTS_MISMATCH,
     sub_status: None,
-    location: undefined,
+    location: script,
     indices: [],
     offsets: [],
 }

third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_many_type_args_inner.exp

@@ -4,7 +4,7 @@ task 1 'run'. lines 6-10:
 Error: Script execution failed with VMError: {
     major_status: NUMBER_OF_TYPE_ARGUMENTS_MISMATCH,
     sub_status: None,
-    location: undefined,
+    location: script,
     indices: [],
     offsets: [],
 }

third_party/move/move-vm/transactional-tests/tests/entry_points/script_too_many_type_args_inner.v2_exp

@@ -4,7 +4,7 @@ task 1 'run'. lines 6-10:
 Error: Script execution failed with VMError: {
     major_status: NUMBER_OF_TYPE_ARGUMENTS_MISMATCH,
     sub_status: None,
-    location: undefined,
+    location: script,
     indices: [],
     offsets: [],
 }

third_party/move/move-vm/types/src/code/errors.rs

@@ -26,9 +26,9 @@ macro_rules! module_storage_error {
     };
 }
 
-// TODO(loader_v2):
-//   The error message is formatted in the same way as V1, to ensure that replay and tests work in
-//   the same way, but ideally we should use proper formatting here.
+// Note:
+//   The error message is formatted in the same way as by the legacy loader implementation, to
+//   ensure that replay and tests work in the same way.
 #[macro_export]
 macro_rules! module_linker_error {
     ($addr:expr, $name:expr) => {