[move] Use clever error locations for macro-rewritten lines in Move u…

…nit test errors (#18221)

## Description 

This changes the way we report source locations for clever errors coming
from an `assert!(<bool>)` within a macro. In particular, since the line
number is rewritten during macro-expansion, we use this as the location
of the error in unit tests when reporting the error if there is no
overlap between the line number from the clever assertion error and the
"actual" error.

Note that it was easiest to do this by plumbing in the `MappedFiles`
instead of using the `FilesSourcesText` in unit tests anymore. This
simplified a number of other places as well (e.g., for getting the line
number from a location).

## Test plan 

Made sure existing tests continued to pass, and added tests for the new
behavior.

---

## Release notes

Check each box that your changes affect. If none of the boxes relate to
your changes, release notes aren't required.

For each box you select, include information after the relevant heading
that describes the impact of your changes that a user might notice and
any actions they must take to implement updates.

- [ ] Protocol: 
- [ ] Nodes (Validators and Full nodes): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:

external-crates/move/crates/move-cli/src/base/test.rs

@@ -5,7 +5,7 @@ use super::reroot_path;
 use crate::NativeFunctionRecord;
 use anyhow::Result;
 use clap::*;
-use move_command_line_common::files::{FileHash, MOVE_COVERAGE_MAP_EXTENSION};
+use move_command_line_common::files::MOVE_COVERAGE_MAP_EXTENSION;
 use move_compiler::{
     diagnostics::{self, Diagnostics},
     shared::{NumberFormat, NumericalAddress},
@@ -16,7 +16,7 @@ use move_coverage::coverage_map::{output_map_to_file, CoverageMap};
 use move_package::{compilation::build_plan::BuildPlan, BuildConfig};
 use move_unit_test::UnitTestingConfig;
 use move_vm_test_utils::gas_schedule::CostTable;
-use std::{collections::HashMap, fs, io::Write, path::Path, process::ExitStatus, sync::Arc};
+use std::{io::Write, path::Path, process::ExitStatus};
 // if windows
 #[cfg(target_family = "windows")]
 use std::os::windows::process::ExitStatusExt;
@@ -158,23 +158,6 @@ pub fn run_move_unit_tests<W: Write + Send>(
         })
         .collect();
 
-    // Get the source files for all modules. We need this in order to report source-mapped error
-    // messages.
-    let dep_file_map: HashMap<_, _> = resolution_graph
-        .package_table
-        .iter()
-        .flat_map(|(_, rpkg)| {
-            rpkg.get_sources(&resolution_graph.build_options)
-                .unwrap()
-                .iter()
-                .map(|fname| {
-                    let contents = fs::read_to_string(Path::new(fname.as_str())).unwrap();
-                    let fhash = FileHash::new(&contents);
-                    (fhash, (*fname, Arc::from(contents)))
-                })
-                .collect::<HashMap<_, _>>()
-        })
-        .collect();
     let root_package = resolution_graph.root_package();
     let build_plan = BuildPlan::create(resolution_graph)?;
     // Compile the package. We need to intercede in the compilation, process being performed by the
@@ -189,6 +172,7 @@ pub fn run_move_unit_tests<W: Write + Send>(
         let (mut compiler, cfgir) = compiler.into_ast();
         let compilation_env = compiler.compilation_env();
         let built_test_plan = construct_test_plan(compilation_env, Some(root_package), &cfgir);
+        let mapped_files = compilation_env.file_mapping().clone();
 
         let compilation_result = compiler.at_cfgir(cfgir).build();
         let (units, warnings) =
@@ -199,16 +183,15 @@ pub fn run_move_unit_tests<W: Write + Send>(
             .into_iter()
             .map(|unit| unit.named_module)
             .collect();
-        test_plan = Some((built_test_plan, files.clone(), named_units));
+        test_plan = Some((built_test_plan, mapped_files, named_units));
         warning_diags = Some(warnings);
         Ok((files, units))
     })?;
 
-    let (test_plan, mut files, units) = test_plan.unwrap();
-    files.extend(dep_file_map);
+    let (test_plan, mapped_files, units) = test_plan.unwrap();
     let test_plan = test_plan.unwrap();
     let no_tests = test_plan.is_empty();
-    let test_plan = TestPlan::new(test_plan, files, units);
+    let test_plan = TestPlan::new(test_plan, mapped_files, units);
 
     let trace_path = pkg_path.join(".trace");
     let coverage_map_path = pkg_path

external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/args.exp

@@ -2,21 +2,64 @@ Command `test -t 1`:
 INCLUDING DEPENDENCY MoveStdlib
 BUILDING PackageBasics
 Running Move unit tests
+[ FAIL    ] std::AModule::abort_in_macro_same_module
 [ PASS    ] std::AModule::double_three
 [ PASS    ] std::AModule::double_two
 [ PASS    ] std::AModuleTests::double_one_one
 [ PASS    ] std::AModuleTests::double_three
 [ PASS    ] std::AModuleTests::double_three_location_based_valid
 [ PASS    ] std::AModuleTests::double_zero_zero
+[ FAIL    ] std::AModuleTestsShouldAllFail::abort_in_macro
+[ FAIL    ] std::AModuleTestsShouldAllFail::clever_error_line_abort_in_non_macro
 [ FAIL    ] std::AModuleTestsShouldAllFail::double_three_const_based_different_module_fail
 [ FAIL    ] std::AModuleTestsShouldAllFail::double_three_location_based_invalid
 [ FAIL    ] std::AModuleTestsShouldAllFail::double_three_should_fail
 [ FAIL    ] std::AModuleTestsShouldAllFail::double_three_should_fail_named_const
 
 Test failures:
 
+Failures in std::AModule:
+
+┌── abort_in_macro_same_module ──────
+│ error[E11001]: test failure
+│    ┌─ ./sources/AModule.move:40:9
+│    │
+│ 39 │     fun abort_in_macro_same_module() {
+│    │         -------------------------- In this function in std::AModule
+│ 40 │         abort_!();
+│    │         ^^^^^^^^^^ Test was not expected to error, but it aborted originating in the module std::AModule rooted here
+│ 
+│ 
+└──────────────────
+
 Failures in std::AModuleTestsShouldAllFail:
 
+┌── abort_in_macro ──────
+│ error[E11001]: test failure
+│    ┌─ ./tests/AModuleTestsShouldAllFail.move:33:9
+│    │
+│ 32 │     fun abort_in_macro() {
+│    │         -------------- In this function in std::AModuleTestsShouldAllFail
+│ 33 │         AModule::abort_!();
+│    │         ^^^^^^^^^^^^^^^^^^^ Test was not expected to error, but it aborted originating in the module std::AModuleTestsShouldAllFail rooted here
+│ 
+│ 
+└──────────────────
+
+
+┌── clever_error_line_abort_in_non_macro ──────
+│ error[E11001]: test failure
+│    ┌─ ./tests/AModuleTestsShouldAllFail.move:38:9
+│    │
+│ 37 │     fun clever_error_line_abort_in_non_macro() {
+│    │         ------------------------------------ In this function in std::AModuleTestsShouldAllFail
+│ 38 │         assert!(false);
+│    │         ^^^^^^^^^^^^^^ Test was not expected to error, but it aborted originating in the module std::AModuleTestsShouldAllFail rooted here
+│ 
+│ 
+└──────────────────
+
+
 ┌── double_three_const_based_different_module_fail ──────
 │ error[E11001]: test failure
 │    ┌─ ./sources/AModule.move:10:9
@@ -68,7 +111,7 @@ Failures in std::AModuleTestsShouldAllFail:
 │ 
 └──────────────────
 
-Test result: FAILED. Total tests: 10; passed: 6; failed: 4
+Test result: FAILED. Total tests: 13; passed: 6; failed: 7
 warning[W10007]: issue with attribute value
   ┌─ ./tests/AModuleTestsShouldAllFail.move:8:24
   │

external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/sources/AModule.move

@@ -30,4 +30,13 @@ module std::AModule {
     fun double_three() {
         double_except_three(3);
     }
+
+    public macro fun abort_() {
+        assert!(false);
+    }
+
+    #[test]
+    fun abort_in_macro_same_module() {
+        abort_!();
+    }
 }

external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/tests/AModuleTestsShouldAllFail.move

@@ -27,4 +27,14 @@ module std::AModuleTestsShouldAllFail {
     fun double_three_const_based_different_module_fail() {
         AModule::double_except_three(3);
     }
+
+    #[test]
+    fun abort_in_macro() {
+        AModule::abort_!();
+    }
+
+    #[test]
+    fun clever_error_line_abort_in_non_macro() {
+        assert!(false);
+    }
 }

external-crates/move/crates/move-compiler/src/diagnostics/mod.rs

@@ -132,6 +132,7 @@ pub struct Migration {
 }
 
 /// A mapping from file ids to file contents along with the mapping of filehash to fileID.
+#[derive(Debug, Clone)]
 pub struct MappedFiles {
     files: SimpleFiles<Symbol, Arc<str>>,
     file_mapping: HashMap<FileHash, FileId>,
@@ -263,6 +264,35 @@ pub trait PositionInfo {
         };
         Some(posn)
     }
+
+    /// Given a line number in the file return the `Loc` for the line.
+    fn line_to_loc_opt(&self, file_hash: &FileHash, line_number: usize) -> Option<Loc> {
+        let file_id = self.file_mapping().get(file_hash)?;
+        let line_range = self.files().line_range(*file_id, line_number).ok()?;
+        Some(Loc::new(
+            *file_hash,
+            line_range.start as u32,
+            line_range.end as u32,
+        ))
+    }
+
+    /// Given a location `Loc` return a new loc only for source with leading and trailing
+    /// whitespace removed.
+    fn trimmed_loc_opt(&self, loc: &Loc) -> Option<Loc> {
+        let source_str = self.source_of_loc_opt(loc)?;
+        let trimmed_front = source_str.trim_start();
+        let new_start = loc.start() as usize + (source_str.len() - trimmed_front.len());
+        let trimmed_back = trimmed_front.trim_end();
+        let new_end = (loc.end() as usize).saturating_sub(trimmed_front.len() - trimmed_back.len());
+        Some(Loc::new(loc.file_hash(), new_start as u32, new_end as u32))
+    }
+
+    /// Given a location `Loc` return the source for the location. This include any leading and
+    /// trailing whitespace.
+    fn source_of_loc_opt(&self, loc: &Loc) -> Option<&str> {
+        let file_id = *self.file_mapping().get(&loc.file_hash())?;
+        Some(&self.files().source(file_id).ok()?[loc.usize_range()])
+    }
 }
 
 impl PositionInfo for MappedFiles {
@@ -351,6 +381,20 @@ pub fn report_diagnostics_to_buffer(
     writer.into_inner()
 }
 
+pub fn report_diagnostics_to_buffer_with_mapped_files(
+    mapped_files: &MappedFiles,
+    diags: Diagnostics,
+    ansi_color: bool,
+) -> Vec<u8> {
+    let mut writer = if ansi_color {
+        Buffer::ansi()
+    } else {
+        Buffer::no_color()
+    };
+    render_diagnostics(&mut writer, mapped_files, diags);
+    writer.into_inner()
+}
+
 fn env_color() -> ColorChoice {
     match read_env_var(COLOR_MODE_ENV_VAR).as_str() {
         "NONE" => ColorChoice::Never,
@@ -366,10 +410,10 @@ fn output_diagnostics<W: WriteColor>(
     diags: Diagnostics,
 ) {
     let mapping = MappedFiles::new(sources.clone());
-    render_diagnostics(writer, mapping, diags);
+    render_diagnostics(writer, &mapping, diags);
 }
 
-fn render_diagnostics(writer: &mut dyn WriteColor, mapping: MappedFiles, diags: Diagnostics) {
+fn render_diagnostics(writer: &mut dyn WriteColor, mapping: &MappedFiles, diags: Diagnostics) {
     let Diagnostics {
         diags: Some(mut diags),
         format,
@@ -387,8 +431,8 @@ fn render_diagnostics(writer: &mut dyn WriteColor, mapping: MappedFiles, diags:
         loc1.cmp(loc2)
     });
     match format {
-        DiagnosticsFormat::Text => emit_diagnostics_text(writer, &mapping, diags),
-        DiagnosticsFormat::JSON => emit_diagnostics_json(writer, &mapping, diags),
+        DiagnosticsFormat::Text => emit_diagnostics_text(writer, mapping, diags),
+        DiagnosticsFormat::JSON => emit_diagnostics_json(writer, mapping, diags),
     }
 }
 

external-crates/move/crates/move-compiler/src/unit_test/mod.rs

@@ -3,7 +3,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use crate::{
-    compiled_unit::NamedCompiledModule, diagnostics::FilesSourceText, shared::NumericalAddress,
+    compiled_unit::NamedCompiledModule, diagnostics::MappedFiles, shared::NumericalAddress,
 };
 use move_core_types::{
     account_address::AccountAddress,
@@ -19,9 +19,8 @@ pub mod plan_builder;
 
 pub type TestName = String;
 
-#[derive(Debug, Clone)]
 pub struct TestPlan {
-    pub files: FilesSourceText,
+    pub mapped_files: MappedFiles,
     pub module_tests: BTreeMap<ModuleId, ModuleTestPlan>,
     pub module_info: BTreeMap<ModuleId, NamedCompiledModule>,
 }
@@ -90,7 +89,7 @@ impl ModuleTestPlan {
 impl TestPlan {
     pub fn new(
         tests: Vec<ModuleTestPlan>,
-        files: FilesSourceText,
+        mapped_files: MappedFiles,
         units: Vec<NamedCompiledModule>,
     ) -> Self {
         let module_tests: BTreeMap<_, _> = tests
@@ -104,7 +103,7 @@ impl TestPlan {
             .collect();
 
         Self {
-            files,
+            mapped_files,
             module_tests,
             module_info,
         }

external-crates/move/crates/move-unit-test/src/lib.rs

@@ -172,26 +172,24 @@ impl UnitTestingConfig {
         let (mut compiler, cfgir) = compiler.into_ast();
         let compilation_env = compiler.compilation_env();
         let test_plan = unit_test::plan_builder::construct_test_plan(compilation_env, None, &cfgir);
+        let mapped_files = compilation_env.file_mapping().clone();
 
         let compilation_result = compiler.at_cfgir(cfgir).build();
         let (units, warnings) =
             diagnostics::unwrap_or_report_pass_diagnostics(&files, compilation_result);
         diagnostics::report_warnings(&files, warnings);
         let units: Vec<_> = units.into_iter().map(|unit| unit.named_module).collect();
-        test_plan.map(|tests| TestPlan::new(tests, files, units))
+        test_plan.map(|tests| TestPlan::new(tests, mapped_files, units))
     }
 
     /// Build a test plan from a unit test config
     pub fn build_test_plan(&self) -> Option<TestPlan> {
         let deps = self.dep_files.clone();
 
-        let TestPlan {
-            files, module_info, ..
-        } = self.compile_to_test_plan(deps.clone(), vec![])?;
+        let TestPlan { module_info, .. } = self.compile_to_test_plan(deps.clone(), vec![])?;
 
         let mut test_plan = self.compile_to_test_plan(self.source_files.clone(), deps)?;
         test_plan.module_info.extend(module_info);
-        test_plan.files.extend(files);
         Some(test_plan)
     }