diff --git a/external-crates/move/crates/move-cli/src/base/test.rs b/external-crates/move/crates/move-cli/src/base/test.rs index 40ee6c3e8aea9..7890922976eed 100644 --- a/external-crates/move/crates/move-cli/src/base/test.rs +++ b/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( }) .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::>() - }) - .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( 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( .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 diff --git a/external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/args.exp b/external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/args.exp index 5bdd7ad057fff..31a900de65d36 100644 --- a/external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/args.exp +++ b/external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/args.exp @@ -2,12 +2,15 @@ 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 @@ -15,8 +18,48 @@ Running Move unit tests 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 │ diff --git a/external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/sources/AModule.move b/external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/sources/AModule.move index 9d3998e27cba9..7ce8f44add026 100644 --- a/external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/sources/AModule.move +++ b/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_!(); + } } diff --git a/external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/tests/AModuleTestsShouldAllFail.move b/external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/tests/AModuleTestsShouldAllFail.move index 00d06cb321b68..94c837cfa9815 100644 --- a/external-crates/move/crates/move-cli/tests/sandbox_tests/clever_errors/tests/AModuleTestsShouldAllFail.move +++ b/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); + } } diff --git a/external-crates/move/crates/move-compiler/src/diagnostics/mod.rs b/external-crates/move/crates/move-compiler/src/diagnostics/mod.rs index 92d0b66cd45c3..c520494e1c042 100644 --- a/external-crates/move/crates/move-compiler/src/diagnostics/mod.rs +++ b/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>, file_mapping: HashMap, @@ -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 { + 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 { + 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 { + 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( 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), } } diff --git a/external-crates/move/crates/move-compiler/src/unit_test/mod.rs b/external-crates/move/crates/move-compiler/src/unit_test/mod.rs index c35cd7c9ca190..4a3a8d5b816ed 100644 --- a/external-crates/move/crates/move-compiler/src/unit_test/mod.rs +++ b/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, pub module_info: BTreeMap, } @@ -90,7 +89,7 @@ impl ModuleTestPlan { impl TestPlan { pub fn new( tests: Vec, - files: FilesSourceText, + mapped_files: MappedFiles, units: Vec, ) -> Self { let module_tests: BTreeMap<_, _> = tests @@ -104,7 +103,7 @@ impl TestPlan { .collect(); Self { - files, + mapped_files, module_tests, module_info, } diff --git a/external-crates/move/crates/move-unit-test/src/lib.rs b/external-crates/move/crates/move-unit-test/src/lib.rs index 508a1f71c69af..cc923914810e5 100644 --- a/external-crates/move/crates/move-unit-test/src/lib.rs +++ b/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 { 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) } diff --git a/external-crates/move/crates/move-unit-test/src/test_reporter.rs b/external-crates/move/crates/move-unit-test/src/test_reporter.rs index a622ba91a9620..13cd2f5cbc64c 100644 --- a/external-crates/move/crates/move-unit-test/src/test_reporter.rs +++ b/external-crates/move/crates/move-unit-test/src/test_reporter.rs @@ -3,21 +3,22 @@ // SPDX-License-Identifier: Apache-2.0 use crate::format_module_id; -use codespan_reporting::files::{Files, SimpleFiles}; use colored::{control, Colorize}; use move_binary_format::errors::{ExecutionState, Location, VMError}; -use move_command_line_common::files::FileHash; +use move_command_line_common::error_bitset::ErrorBitset; use move_compiler::{ - diagnostics::{self, Diagnostic, Diagnostics}, + diagnostics::{self, Diagnostic, Diagnostics, PositionInfo}, unit_test::{ModuleTestPlan, MoveErrorType, TestPlan}, }; -use move_core_types::{language_storage::ModuleId, vm_status::StatusType}; +use move_core_types::{ + language_storage::ModuleId, + vm_status::{StatusCode, StatusType}, +}; use move_ir_types::location::Loc; -use move_symbol_pool::Symbol; use std::{ - collections::{BTreeMap, BTreeSet, HashMap}, + collections::{BTreeMap, BTreeSet}, io::{Result, Write}, - sync::{Arc, Mutex}, + sync::Mutex, time::Duration, }; @@ -61,7 +62,7 @@ pub struct TestStatistics { failed: BTreeMap>, } -#[derive(Debug, Clone)] +// #[derive(Debug, Clone)] pub struct TestResults { final_statistics: TestStatistics, test_plan: TestPlan, @@ -110,6 +111,31 @@ impl FailureReason { } } +fn clever_error_line_number_to_loc(test_plan: &TestPlan, vm_error: &VMError) -> Option { + let abort_code = match (vm_error.major_status(), vm_error.sub_status()) { + (StatusCode::ABORTED, Some(abort_code)) => abort_code, + _ => return None, + }; + let location = vm_error.location(); + let bitset = ErrorBitset::from_u64(abort_code)?; + if bitset.identifier_index().is_some() || bitset.constant_index().is_some() { + return None; + } + let line_number = bitset.line_number()? - 1; + + match location { + Location::Undefined => None, + Location::Module(module_id) => { + let source_map = &test_plan.module_info.get(module_id)?.source_map; + let file_hash = source_map.definition_location.file_hash(); + let loc = test_plan + .mapped_files + .line_to_loc_opt(&file_hash, line_number as usize)?; + test_plan.mapped_files.trimmed_loc_opt(&loc) + } + } +} + impl TestFailure { pub fn new( failure_reason: FailureReason, @@ -177,46 +203,11 @@ impl TestFailure { } } - fn get_line_number( - loc: &Loc, - files: &SimpleFiles>, - file_mapping: &HashMap, - ) -> String { - Self::get_line_number_internal(loc, files, file_mapping) - .unwrap_or_else(|_| "no_source_line".to_string()) - } - - fn get_line_number_internal( - loc: &Loc, - files: &SimpleFiles>, - file_mapping: &HashMap, - ) -> std::result::Result { - let id = file_mapping - .get(&loc.file_hash()) - .ok_or(codespan_reporting::files::Error::FileMissing)?; - let start_line_index = files.line_index(*id, loc.start() as usize)?; - let start_line_number = files.line_number(*id, start_line_index)?; - let end_line_index = files.line_index(*id, loc.end() as usize)?; - let end_line_number = files.line_number(*id, end_line_index)?; - if start_line_number == end_line_number { - Ok(start_line_number.to_string()) - } else { - Ok(format!("{}-{}", start_line_number, end_line_number)) - } - } - fn report_exec_state(test_plan: &TestPlan, exec_state: &ExecutionState) -> String { let stack_trace = exec_state.stack_trace(); let mut buf = String::new(); if !stack_trace.is_empty() { buf.push_str("stack trace\n"); - let mut files = SimpleFiles::new(); - let mut file_mapping = HashMap::new(); - for (fhash, (fname, source)) in &test_plan.files { - let id = files.add(*fname, source.clone()); - file_mapping.insert(*fhash, id); - } - for frame in stack_trace { let module_id = &frame.0; let named_module = match test_plan.module_info.get(module_id) { @@ -233,9 +224,14 @@ impl TestFailure { let fn_handle_idx = named_module.module.function_def_at(frame.1).function; let fn_id_idx = named_module.module.function_handle_at(fn_handle_idx).name; let fn_name = named_module.module.identifier_at(fn_id_idx).as_str(); - let file_name = match test_plan.files.get(&loc.file_hash()) { - Some(v) => format!("{}", v.0), - None => "unknown_source".to_string(), + let file_name = test_plan.mapped_files.filename(&loc.file_hash()); + let formatted_line = { + let position = test_plan.mapped_files.position(&loc); + if position.start.line == position.end.line { + format!("{}", position.start.line) + } else { + format!("{}-{}", position.start.line, position.end.line) + } }; buf.push_str( &format!( @@ -243,7 +239,7 @@ impl TestFailure { module_id.name(), fn_name, file_name, - Self::get_line_number(&loc, &files, &file_mapping) + formatted_line ) .to_string(), ); @@ -257,9 +253,9 @@ impl TestFailure { base_message: String, vm_error: &Option, ) -> String { - let report_diagnostics = |files, diags| { - diagnostics::report_diagnostics_to_buffer( - files, + let report_diagnostics = |mapped_files, diags| { + diagnostics::report_diagnostics_to_buffer_with_mapped_files( + mapped_files, diags, control::SHOULD_COLORIZE.should_colorize(), ) @@ -280,6 +276,15 @@ impl TestFailure { .get_function_source_map(*fdef_idx) .ok()?; let loc = function_source_map.get_code_location(*offset).unwrap(); + + let alternate_location_opt = + clever_error_line_number_to_loc(test_plan, vm_error); + let loc = + if alternate_location_opt.is_some_and(|alt_loc| !loc.overlaps(&alt_loc)) { + alternate_location_opt.unwrap() + } else { + loc + }; let msg = format!( "In this function in {}", format_module_id(&test_plan.module_info, module_id) @@ -295,7 +300,7 @@ impl TestFailure { match diag_opt { None => base_message, Some(diag) => String::from_utf8(report_diagnostics( - &test_plan.files, + &test_plan.mapped_files, Diagnostics::from(vec![diag]), )) .unwrap(),