From 6e9aaa99e7fc74fe02789ce47b3f21614a4bf531 Mon Sep 17 00:00:00 2001 From: Jordan Jennings Date: Fri, 11 Oct 2024 13:37:18 -0700 Subject: [PATCH] pull error rendering out of error type, snapshot tests, only return an error don't print, use named params in fomrat, remove mut from finish() cache files, capitalization show be lower --- crates/sui/src/client_commands.rs | 9 +- .../Move.toml | 0 .../sources/UpgradeErrors.move | 9 + .../Move.toml | 0 .../sources/UpgradeErrors.move | 4 +- ...upgrade_compatibility_tests__all_fail.snap | 23 -- ...atibility_tests__declarations_missing.snap | 30 ++ ...e_compatibility_tests__struct_missing.snap | 26 +- .../unit_tests/upgrade_compatibility_tests.rs | 51 ++- crates/sui/src/upgrade_compatibility.rs | 330 ++++++++---------- .../src/compatibility_mode.rs | 4 +- 11 files changed, 252 insertions(+), 234 deletions(-) rename crates/sui/src/unit_tests/fixtures/upgrade_errors/{struct_missing_v1 => declarations_missing_v1}/Move.toml (100%) rename crates/sui/src/unit_tests/fixtures/upgrade_errors/{struct_missing_v1 => declarations_missing_v1}/sources/UpgradeErrors.move (64%) rename crates/sui/src/unit_tests/fixtures/upgrade_errors/{struct_missing_v2 => declarations_missing_v2}/Move.toml (100%) rename crates/sui/src/unit_tests/fixtures/upgrade_errors/{struct_missing_v2 => declarations_missing_v2}/sources/UpgradeErrors.move (64%) delete mode 100644 crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__all_fail.snap create mode 100644 crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__declarations_missing.snap diff --git a/crates/sui/src/client_commands.rs b/crates/sui/src/client_commands.rs index 59c8df0c7f9a6a..06d51a622a77fa 100644 --- a/crates/sui/src/client_commands.rs +++ b/crates/sui/src/client_commands.rs @@ -929,14 +929,7 @@ impl SuiClientCommands { upgrade_package, ) = upgrade_result?; - check_compatibility( - &client, - package_id, - &compiled_modules, - upgrade_package, - protocol_config, - ) - .await?; + check_compatibility(&client, package_id, upgrade_package, protocol_config).await?; let tx_kind = client .transaction_builder() diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_missing_v1/Move.toml b/crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v1/Move.toml similarity index 100% rename from crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_missing_v1/Move.toml rename to crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v1/Move.toml diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_missing_v1/sources/UpgradeErrors.move b/crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v1/sources/UpgradeErrors.move similarity index 64% rename from crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_missing_v1/sources/UpgradeErrors.move rename to crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v1/sources/UpgradeErrors.move index 70db96559ad503..d585e83f087f41 100644 --- a/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_missing_v1/sources/UpgradeErrors.move +++ b/crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v1/sources/UpgradeErrors.move @@ -9,5 +9,14 @@ module upgrades::upgrades { public struct StructToBeRemoved { b: u64 } + + public enum EnumToBeRemoved { + A, + B + } + + public fun fun_to_be_removed(): u64 { + 0 + } } diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_missing_v2/Move.toml b/crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/Move.toml similarity index 100% rename from crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_missing_v2/Move.toml rename to crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/Move.toml diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_missing_v2/sources/UpgradeErrors.move b/crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move similarity index 64% rename from crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_missing_v2/sources/UpgradeErrors.move rename to crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move index 542e2edca5b603..920b2a69af77bb 100644 --- a/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_missing_v2/sources/UpgradeErrors.move +++ b/crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move @@ -5,7 +5,9 @@ #[allow(unused_field)] module upgrades::upgrades { - // struct missing + // removed declarations // public struct StructToBeRemoved {} + // public enum EnumToBeRemoved {} + // public fun fun_to_be_removed() {} } diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__all_fail.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__all_fail.snap deleted file mode 100644 index f75df286256ed5..00000000000000 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__all_fail.snap +++ /dev/null @@ -1,23 +0,0 @@ ---- -source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs -expression: err.to_string() ---- -Upgrade compatibility check failed with the following errors: -- StructAbilityMismatch { name: Identifier("StructAbilityMismatchAdd"), old_struct: Struct { abilities: [], type_parameters: [], fields: [Field { name: Identifier("dummy_field"), type_: Bool }] }, new_struct: Struct { abilities: [Copy, ], type_parameters: [], fields: [Field { name: Identifier("dummy_field"), type_: Bool }] } } -- StructAbilityMismatch { name: Identifier("StructAbilityMismatchChange"), old_struct: Struct { abilities: [Copy, ], type_parameters: [], fields: [Field { name: Identifier("dummy_field"), type_: Bool }] }, new_struct: Struct { abilities: [Drop, ], type_parameters: [], fields: [Field { name: Identifier("dummy_field"), type_: Bool }] } } -- StructAbilityMismatch { name: Identifier("StructAbilityMismatchRemove"), old_struct: Struct { abilities: [Copy, ], type_parameters: [], fields: [Field { name: Identifier("dummy_field"), type_: Bool }] }, new_struct: Struct { abilities: [], type_parameters: [], fields: [Field { name: Identifier("dummy_field"), type_: Bool }] } } -- StructFieldMismatch { name: Identifier("StructFieldMismatchAdd"), old_struct: Struct { abilities: [], type_parameters: [], fields: [Field { name: Identifier("a"), type_: U64 }, Field { name: Identifier("b"), type_: U64 }] }, new_struct: Struct { abilities: [], type_parameters: [], fields: [Field { name: Identifier("a"), type_: U64 }, Field { name: Identifier("b"), type_: U64 }, Field { name: Identifier("c"), type_: U64 }] } } -- StructFieldMismatch { name: Identifier("StructFieldMismatchChange"), old_struct: Struct { abilities: [], type_parameters: [], fields: [Field { name: Identifier("a"), type_: U64 }, Field { name: Identifier("b"), type_: U64 }] }, new_struct: Struct { abilities: [], type_parameters: [], fields: [Field { name: Identifier("a"), type_: U64 }, Field { name: Identifier("b"), type_: U8 }] } } -- StructFieldMismatch { name: Identifier("StructFieldMismatchRemove"), old_struct: Struct { abilities: [], type_parameters: [], fields: [Field { name: Identifier("a"), type_: U64 }, Field { name: Identifier("b"), type_: U64 }] }, new_struct: Struct { abilities: [], type_parameters: [], fields: [Field { name: Identifier("a"), type_: U64 }] } } -- StructMissing { name: Identifier("StructToBeRemoved"), old_struct: Struct { abilities: [], type_parameters: [], fields: [Field { name: Identifier("b"), type_: U64 }] } } -- StructTypeParamMismatch { name: Identifier("StructTypeParamMismatch"), old_struct: Struct { abilities: [], type_parameters: [DatatypeTyParameter { constraints: [], is_phantom: false }, DatatypeTyParameter { constraints: [], is_phantom: false }], fields: [Field { name: Identifier("a"), type_: TypeParameter(0) }] }, new_struct: Struct { abilities: [], type_parameters: [DatatypeTyParameter { constraints: [], is_phantom: false }], fields: [Field { name: Identifier("a"), type_: TypeParameter(0) }] } } -- EnumAbilityMismatch { name: Identifier("EnumAbilityMismatchAdd"), old_enum: Enum { abilities: [], type_parameters: [], variants: [Variant { name: Identifier("A"), fields: [] }] }, new_enum: Enum { abilities: [Copy, ], type_parameters: [], variants: [Variant { name: Identifier("A"), fields: [] }] } } -- EnumAbilityMismatch { name: Identifier("EnumAbilityMismatchChange"), old_enum: Enum { abilities: [Copy, ], type_parameters: [], variants: [Variant { name: Identifier("A"), fields: [] }] }, new_enum: Enum { abilities: [Drop, ], type_parameters: [], variants: [Variant { name: Identifier("A"), fields: [] }] } } -- EnumAbilityMismatch { name: Identifier("EnumAbilityMismatchRemove"), old_enum: Enum { abilities: [Copy, ], type_parameters: [], variants: [Variant { name: Identifier("A"), fields: [] }] }, new_enum: Enum { abilities: [], type_parameters: [], variants: [Variant { name: Identifier("A"), fields: [] }] } } -- EnumNewVariant { name: Identifier("EnumNewVariant"), old_enum: Enum { abilities: [], type_parameters: [], variants: [Variant { name: Identifier("A"), fields: [] }, Variant { name: Identifier("B"), fields: [] }, Variant { name: Identifier("C"), fields: [] }] }, new_enum: Enum { abilities: [], type_parameters: [], variants: [Variant { name: Identifier("A"), fields: [] }, Variant { name: Identifier("B"), fields: [] }, Variant { name: Identifier("C"), fields: [] }, Variant { name: Identifier("D"), fields: [] }] } } -- EnumMissing { name: Identifier("EnumToBeRemoved"), old_enum: Enum { abilities: [], type_parameters: [], variants: [Variant { name: Identifier("A"), fields: [] }, Variant { name: Identifier("B"), fields: [] }] } } -- EnumVariantMissing { name: Identifier("EnumVariantMissing"), old_enum: Enum { abilities: [], type_parameters: [], variants: [Variant { name: Identifier("A"), fields: [] }, Variant { name: Identifier("B"), fields: [] }] }, tag: 1 } -- FunctionSignatureMismatch { name: Identifier("function_add_arg"), old_function: Function { visibility: Public, is_entry: false, type_parameters: [], parameters: [], return_: [], code: [Ret] }, new_function: Function { visibility: Public, is_entry: false, type_parameters: [], parameters: [U64], return_: [], code: [Ret] } } -- FunctionSignatureMismatch { name: Identifier("function_change_arg"), old_function: Function { visibility: Public, is_entry: false, type_parameters: [], parameters: [U64], return_: [], code: [Ret] }, new_function: Function { visibility: Public, is_entry: false, type_parameters: [], parameters: [U8], return_: [], code: [Ret] } } -- FunctionSignatureMismatch { name: Identifier("function_remove_arg"), old_function: Function { visibility: Public, is_entry: false, type_parameters: [], parameters: [U64], return_: [], code: [Ret] }, new_function: Function { visibility: Public, is_entry: false, type_parameters: [], parameters: [], return_: [], code: [Ret] } } -- FunctionLostPublicVisibility { name: Identifier("function_to_have_public_removed"), old_function: Function { visibility: Public, is_entry: false, type_parameters: [], parameters: [], return_: [], code: [Ret] } } diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__declarations_missing.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__declarations_missing.snap new file mode 100644 index 00000000000000..f0105f449314ba --- /dev/null +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__declarations_missing.snap @@ -0,0 +1,30 @@ +--- +source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs +expression: normalize_path(err.to_string()) +--- +Upgrade compatibility check failed: +error: struct is missing + ┌─ crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move:7:18 + │ +7 │ module upgrades::upgrades { + │ ^^^^^^^^ Module 'upgrades' expected struct 'StructToBeRemoved', but found none + │ + = The struct is missing in the new module, add the previously defined: 'StructToBeRemoved' + +error: enum is missing + ┌─ crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move:7:18 + │ +7 │ module upgrades::upgrades { + │ ^^^^^^^^ Module 'upgrades' expected enum 'EnumToBeRemoved', but found none + │ + = The enum is missing in the new module, add the previously defined: 'EnumToBeRemoved' + +error: public function is missing + ┌─ crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move:7:18 + │ +7 │ module upgrades::upgrades { + │ ^^^^^^^^ Module 'upgrades' expected public function 'fun_to_be_removed', but found none + │ + = The public function is missing in the new module, add the previously defined: 'fun_to_be_removed' + + diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__struct_missing.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__struct_missing.snap index faa586a1414add..bf52e38771b994 100644 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__struct_missing.snap +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__struct_missing.snap @@ -2,5 +2,27 @@ source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs expression: err.to_string() --- -Upgrade compatibility check failed with the following errors: -- StructMissing { name: Identifier("StructToBeRemoved"), old_struct: Struct { abilities: [], type_parameters: [], fields: [Field { name: Identifier("b"), type_: U64 }] } } +Upgrade compatibility check failed: +error: struct is missing + ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move:7:18 + │ +7 │ module upgrades::upgrades { + │ ^^^^^^^^ Module 'upgrades' expected struct 'StructToBeRemoved', but found none + │ + = The struct is missing in the new module, add the previously defined: 'StructToBeRemoved' + +error: enum is missing + ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move:7:18 + │ +7 │ module upgrades::upgrades { + │ ^^^^^^^^ Module 'upgrades' expected enum 'EnumToBeRemoved', but found none + │ + = The enum is missing in the new module, add the previously defined: 'EnumToBeRemoved' + +error: public function is missing + ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move:7:18 + │ +7 │ module upgrades::upgrades { + │ ^^^^^^^^ Module 'upgrades' expected public function 'fun_to_be_removed', but found none + │ + = The public function is missing in the new module, add the previously defined: 'fun_to_be_removed' diff --git a/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs b/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs index 1f0e38eae99950..0acd7dd2dff176 100644 --- a/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs +++ b/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs @@ -6,48 +6,56 @@ use insta::assert_snapshot; use move_binary_format::CompiledModule; use std::path::PathBuf; use sui_move_build::BuildConfig; +use sui_move_build::CompiledPackage; #[test] +#[should_panic] fn test_all_fail() { - let (pkg_v1, pkg_v2) = get_packages("all"); + let (mods_v1, pkg_v2) = get_packages("all"); - let result = compare_packages(pkg_v1, pkg_v2); - assert!(result.is_err()); - let err = result.unwrap_err(); - - assert_snapshot!(err.to_string()); + // panics: Not all errors are implemented yet + compare_packages(mods_v1, pkg_v2, false).unwrap(); } #[test] -fn test_struct_missing() { - let (pkg_v1, pkg_v2) = get_packages("struct_missing"); - let result = compare_packages(pkg_v1, pkg_v2); +fn test_declarations_missing() { + let (pkg_v1, pkg_v2) = get_packages("declarations_missing"); + let result = compare_packages(pkg_v1, pkg_v2, false); + + // find lines that have ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/declarations_missing_v2/sources/UpgradeErrors.move:7:18 + // remove the path before /sui/ + let result = result.map_err(|err| { + let err = err.to_string(); + let err = err.replace("/Users/jordanjennings/code/sui/", ""); + err + }); + assert!(result.is_err()); let err = result.unwrap_err(); - assert_snapshot!(err.to_string()); + assert_snapshot!(normalize_path(err.to_string())); } #[test] fn test_friend_link_ok() { let (pkg_v1, pkg_v2) = get_packages("friend_linking"); // upgrade compatibility ignores friend linking - assert!(compare_packages(pkg_v1, pkg_v2).is_ok()); + assert!(compare_packages(pkg_v1, pkg_v2, false).is_ok()); } #[test] fn test_entry_linking_ok() { let (pkg_v1, pkg_v2) = get_packages("entry_linking"); // upgrade compatibility ignores entry linking - assert!(compare_packages(pkg_v1, pkg_v2).is_ok()); + assert!(compare_packages(pkg_v1, pkg_v2, false).is_ok()); } -fn get_packages(name: &str) -> (Vec, Vec) { +fn get_packages(name: &str) -> (Vec, CompiledPackage) { let mut path: PathBuf = PathBuf::from(env!("CARGO_MANIFEST_DIR")); path.push("src/unit_tests/fixtures/upgrade_errors/"); path.push(format!("{}_v1", name)); - let pkg_v1 = BuildConfig::new_for_testing() + let mods_v1 = BuildConfig::new_for_testing() .build(&path) .unwrap() .into_modules(); @@ -56,10 +64,15 @@ fn get_packages(name: &str) -> (Vec, Vec) { path.push("src/unit_tests/fixtures/upgrade_errors/"); path.push(format!("{}_v2", name)); - let pkg_v2 = BuildConfig::new_for_testing() - .build(&path) - .unwrap() - .into_modules(); + let pkg_v2 = BuildConfig::new_for_testing().build(&path).unwrap(); - (pkg_v1, pkg_v2) + (mods_v1, pkg_v2) } + +/// Snapshots will differ on each machine, normalize to prevent test failures +fn normalize_path(err_string: String) -> String { + // match the part of the path that is before /sui/ could be any home directory + let re = regex::Regex::new(r"(^ ┌─ /[-a-zA-Z0-9_/ .+()]+/sui/)").unwrap(); + let path = re.replace(&err_string, " ┌─ /sui/"); + path.to_string() +} \ No newline at end of file diff --git a/crates/sui/src/upgrade_compatibility.rs b/crates/sui/src/upgrade_compatibility.rs index bac22c8af26d4a..e3ac76730b7712 100644 --- a/crates/sui/src/upgrade_compatibility.rs +++ b/crates/sui/src/upgrade_compatibility.rs @@ -11,7 +11,6 @@ use std::fs; use anyhow::{anyhow, Context, Error}; use codespan_reporting::diagnostic::{Diagnostic, Label}; use codespan_reporting::files::SimpleFiles; -use codespan_reporting::term::termcolor::{ColorChoice, StandardStream}; use move_binary_format::{ compatibility::Compatibility, @@ -36,6 +35,9 @@ use sui_types::{base_types::ObjectID, execution_config_utils::to_binary_config}; #[derive(Debug, Clone)] #[allow(dead_code)] pub(crate) enum UpgradeCompatibilityModeError { + ModuleMissing { + name: Identifier, + }, StructMissing { name: Identifier, old_struct: Struct, @@ -109,129 +111,12 @@ pub(crate) enum UpgradeCompatibilityModeError { }, } -/// A list of errors that can occur during upgrade compatibility checks. -#[derive(Debug, Clone, Default)] -pub struct UpgradeErrorList { - errors: Vec, - source: Option, -} - -impl UpgradeErrorList { - fn push(&mut self, err: UpgradeCompatibilityModeError) { - self.errors.push(err); - } - - /// Only keep the errors that break compatibility with the given [`Compatibility`] - fn retain_incompatible(&mut self, compatibility: &Compatibility) { - self.errors - .retain(|e| e.breaks_compatibility(compatibility)); - } - - /// Print the errors to the console with the relevant source code. - fn print_errors( - &mut self, - compiled_unit_with_source: &CompiledUnitWithSource, - ) -> Result<(), Error> { - for err in self.errors.clone() { - match err { - UpgradeCompatibilityModeError::StructMissing { name, .. } => { - self.print_missing_definition( - "Struct", - name.to_string(), - compiled_unit_with_source, - )?; - } - UpgradeCompatibilityModeError::EnumMissing { name, .. } => { - self.print_missing_definition( - "Enum", - name.to_string(), - compiled_unit_with_source, - )?; - } - UpgradeCompatibilityModeError::FunctionMissingPublic { name, .. } => { - self.print_missing_definition( - "Function", - name.to_string(), - compiled_unit_with_source, - )?; - } - UpgradeCompatibilityModeError::FunctionMissingEntry { name, .. } => { - self.print_missing_definition( - "Function", - name.to_string(), - compiled_unit_with_source, - )?; - } - _ => {} - } - } - Ok(()) - } - - /// retrieve the source, caches the source after the first read - fn source( - &mut self, - compiled_unit_with_source: &CompiledUnitWithSource, - ) -> Result<&String, Error> { - if self.source.is_none() { - let source_path = compiled_unit_with_source.source_path.clone(); - let source_content = fs::read_to_string(&source_path)?; - self.source = Some(source_content); - } - Ok(self.source.as_ref().unwrap()) - } - - /// Print missing definition errors, e.g. struct, enum, function - fn print_missing_definition( - &mut self, - declaration_kind: &str, - identifier_name: String, - compiled_unit_with_source: &CompiledUnitWithSource, - ) -> Result<(), Error> { - let module_name = compiled_unit_with_source.unit.name; - let source_path = compiled_unit_with_source.source_path.to_string_lossy(); - let source = self.source(compiled_unit_with_source)?; - - let start = compiled_unit_with_source - .unit - .source_map - .definition_location - .start() as usize; - - let end = compiled_unit_with_source - .unit - .source_map - .definition_location - .end() as usize; - - let mut files = SimpleFiles::new(); - let file_id = files.add(source_path, &source); - - let diag = Diagnostic::error() - .with_message(format!("{} is missing", declaration_kind)) - .with_labels(vec![Label::primary(file_id, start..end).with_message( - format!( - "Module '{}' expected {} '{}', but found none", - declaration_kind, module_name, identifier_name - ), - )]) - .with_notes(vec![format!( - "The {} is missing in the new module, add the previously defined: '{}'", - declaration_kind, identifier_name - )]); - - let mut writer = StandardStream::stderr(ColorChoice::Always); - let config = codespan_reporting::term::Config::default(); - - codespan_reporting::term::emit(&mut writer, &config, &files, &diag) - .context("Unable to print error") - } -} - impl UpgradeCompatibilityModeError { /// check if the error breaks compatibility for a given [`Compatibility`] fn breaks_compatibility(&self, compatability: &Compatibility) -> bool { match self { + UpgradeCompatibilityModeError::ModuleMissing { .. } => true, + UpgradeCompatibilityModeError::StructAbilityMismatch { .. } | UpgradeCompatibilityModeError::StructTypeParamMismatch { .. } | UpgradeCompatibilityModeError::EnumAbilityMismatch { .. } @@ -273,11 +158,11 @@ impl UpgradeCompatibilityModeError { /// A compatibility mode that collects errors as a vector of enums which describe the error causes #[derive(Default)] pub(crate) struct CliCompatibilityMode { - errors: UpgradeErrorList, + errors: Vec, } impl CompatibilityMode for CliCompatibilityMode { - type Error = UpgradeErrorList; + type Error = Vec; // ignored, address is not populated pre-tx fn module_id_mismatch( &mut self, @@ -451,13 +336,16 @@ impl CompatibilityMode for CliCompatibilityMode { }); } - fn finish(&mut self, compatability: &Compatibility) -> Result<(), Self::Error> { - self.errors.retain_incompatible(compatability); - - if !self.errors.errors.is_empty() { - return Err(self.errors.clone()); + fn finish(&self, compatability: &Compatibility) -> Result<(), Self::Error> { + let errors: Vec = self + .errors + .iter() + .filter(|e| e.breaks_compatibility(compatability)) + .cloned() + .collect(); + if !errors.is_empty() { + return Err(errors); } - Ok(()) } } @@ -466,16 +354,9 @@ impl CompatibilityMode for CliCompatibilityMode { pub(crate) async fn check_compatibility( client: &SuiClient, package_id: ObjectID, - compiled_modules: &[Vec], - upgrade_package: CompiledPackage, + new_package: CompiledPackage, protocol_config: ProtocolConfig, ) -> Result<(), Error> { - let new_modules = compiled_modules - .iter() - .map(|b| CompiledModule::deserialize_with_config(b, &to_binary_config(&protocol_config))) - .collect::, _>>() - .context("Unable to to deserialize compiled module")?; - let existing_obj_read = client .read_api() .get_object_with_options(package_id, SuiObjectDataOptions::new().with_bcs()) @@ -500,68 +381,159 @@ pub(crate) async fn check_compatibility( .collect::, _>>() .context("Unable to get existing package")?; - compare_packages(existing_modules, upgrade_package, new_modules) + compare_packages(existing_modules, new_package, true) } /// Collect all the errors into a single error message. fn compare_packages( existing_modules: Vec, - upgrade_package: CompiledPackage, - new_modules: Vec, + new_package: CompiledPackage, + enable_colors: bool, ) -> Result<(), Error> { // create a map from the new modules - let new_modules_map: HashMap = new_modules - .iter() + let new_modules_map: HashMap = new_package + .get_modules() .map(|m| (m.self_id().name().to_owned(), m.clone())) .collect(); - let errors: Vec = existing_modules + let errors: Vec<(Identifier, UpgradeCompatibilityModeError)> = existing_modules .iter() - .map(|existing_module| { + .flat_map(|existing_module| { let name = existing_module.self_id().name().to_owned(); - let compiled_unit_with_source = upgrade_package - .package - .get_module_by_name_from_root(&name.to_string()) - .context("Unable to get module")?; - // find the new module with the same name match new_modules_map.get(&name) { - Some(new_module) => Compatibility::upgrade_check() - .check_with_mode::( - &Module::new(&existing_module), - &Module::new(new_module), - ) - .map_err(|mut error_list| { - if let Err(print_errors) = - error_list.print_errors(&compiled_unit_with_source) - { - print_errors - } else { - anyhow!("Compatibility check failed for module '{}'", name) - } - }), - None => Err(anyhow!("Module '{}' is missing from the package", name)), + Some(new_module) => { + let compatible = Compatibility::upgrade_check() + .check_with_mode::( + &Module::new(existing_module), + &Module::new(new_module), + ); + if let Err(errors) = compatible { + errors.into_iter().map(|e| (name.to_owned(), e)).collect() + } else { + vec![] + } + } + None => vec![( + name.clone(), + UpgradeCompatibilityModeError::ModuleMissing { name }, + )], } }) - // filter to errors - .filter(|r| r.is_err()) - // collect the errors - .map(|r| r.unwrap_err().to_string()) .collect(); - if errors.len() == 1 { - return Err(anyhow!(errors[0].clone())); - } else if !errors.is_empty() { - return Err(anyhow!( - "Upgrade compatibility check failed with the following errors:\n{}", - errors - .iter() - .map(|e| format!("- {}", e)) - .collect::>() - .join("\n") - )); + if errors.is_empty() { + return Ok(()); + } + + let mut files = SimpleFiles::new(); + let config = codespan_reporting::term::Config::default(); + let mut writer; + if enable_colors { + writer = codespan_reporting::term::termcolor::Buffer::ansi(); + } else { + writer = codespan_reporting::term::termcolor::Buffer::no_color(); } + let mut file_id_map = HashMap::new(); + + for (name, err) in errors { + let compiled_unit_with_source = new_package + .package + .get_module_by_name_from_root(&name.to_string()) + .context("Unable to get module")?; + + let source_path = compiled_unit_with_source.source_path.to_string_lossy(); + let file_id = match file_id_map.get(&source_path) { + Some(file_id) => *file_id, + None => { + let source = fs::read_to_string(&compiled_unit_with_source.source_path) + .context("Unable to read source file")?; + let file_id = files.add(source_path.clone(), source); + file_id_map.insert(source_path.clone(), file_id); + file_id + } + }; + + codespan_reporting::term::emit( + &mut writer, + &config, + &files, + &diag_from_error(err, compiled_unit_with_source, file_id), + )?; + } + + Err(anyhow!( + "Upgrade compatibility check failed:\n{}", + String::from_utf8(writer.into_inner()).context("Unable to convert buffer to string")? + )) +} + +/// Convert an error to a diagnostic using the specific error type's function. +fn diag_from_error( + error: UpgradeCompatibilityModeError, + compiled_unit_with_source: &CompiledUnitWithSource, + file_id: usize, +) -> Diagnostic { + match error { + UpgradeCompatibilityModeError::StructMissing { name, .. } => missing_definition_diag( + "struct", + name.to_string(), + compiled_unit_with_source, + file_id, + ), + UpgradeCompatibilityModeError::EnumMissing { name, .. } => { + missing_definition_diag("enum", name.to_string(), compiled_unit_with_source, file_id) + } + UpgradeCompatibilityModeError::FunctionMissingPublic { name, .. } => { + missing_definition_diag( + "public function", + name.to_string(), + compiled_unit_with_source, + file_id, + ) + } + UpgradeCompatibilityModeError::FunctionMissingEntry { name, .. } => { + missing_definition_diag( + "entry function", + name.to_string(), + compiled_unit_with_source, + file_id, + ) + } + _ => todo!("Implement diag_from_error for {:?}", error), + } +} - Ok(()) +/// Return a diagnostic for a missing definition. +fn missing_definition_diag( + declaration_kind: &str, + identifier_name: String, + compiled_unit_with_source: &CompiledUnitWithSource, + file_id: usize, +) -> Diagnostic { + let module_name = compiled_unit_with_source.unit.name; + + let start = compiled_unit_with_source + .unit + .source_map + .definition_location + .start() as usize; + + let end = compiled_unit_with_source + .unit + .source_map + .definition_location + .end() as usize; + + Diagnostic::error() + .with_message(format!("{declaration_kind} is missing")) + .with_labels(vec![Label::primary(file_id, start..end).with_message( + format!( + "Module '{module_name}' expected {declaration_kind} '{identifier_name}', but found none" + ), + )]) + .with_notes(vec![format!( + "The {declaration_kind} is missing in the new module, add the previously defined: '{identifier_name}'" + )]) } diff --git a/external-crates/move/crates/move-binary-format/src/compatibility_mode.rs b/external-crates/move/crates/move-binary-format/src/compatibility_mode.rs index 8192b92f5e9c61..a0848682ae4058 100644 --- a/external-crates/move/crates/move-binary-format/src/compatibility_mode.rs +++ b/external-crates/move/crates/move-binary-format/src/compatibility_mode.rs @@ -100,7 +100,7 @@ pub trait CompatibilityMode: Default { ); /// Finish the compatibility check and return the error if one has been accumulated from individual errors. - fn finish(&mut self, _: &Compatibility) -> Result<(), Self::Error>; + fn finish(&self, _: &Compatibility) -> Result<(), Self::Error>; } /// Compatibility mode impl for execution compatibility checks. @@ -240,7 +240,7 @@ impl CompatibilityMode for ExecutionCompatibilityMode { } /// Finish by comparing against the compatibility flags. - fn finish(&mut self, compatability: &Compatibility) -> Result<(), ()> { + fn finish(&self, compatability: &Compatibility) -> Result<(), ()> { if !self.datatype_and_function_linking { return Err(()); }