diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v1/Move.toml b/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v1/Move.toml new file mode 100644 index 0000000000000..39cc2752ab46d --- /dev/null +++ b/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v1/Move.toml @@ -0,0 +1,6 @@ +[package] +name = "upgrades" +edition = "2024.beta" # edition = "legacy" to use legacy (pre-2024) Move + +[addresses] +upgrades = "0x0" diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v1/sources/UpgradeErrors.move b/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v1/sources/UpgradeErrors.move new file mode 100644 index 0000000000000..904a1ec98fae0 --- /dev/null +++ b/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v1/sources/UpgradeErrors.move @@ -0,0 +1,29 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +/// Module: UpgradeErrors + +#[allow(unused_field)] +module upgrades::upgrades { + public fun func_with_wrong_param(a: u64): u64 { + 0 + } + + public fun func_with_wrong_return(): u64 { + 0 + } + + public fun func_with_wrong_param_and_return(a: u64): u64 { + 0 + } + + public fun func_with_wrong_param_length(a: u64, b: u64): u64 { + 0 + } + + public fun func_with_wrong_return_length(): (u64, u64) { + (0,0) + } + +} + diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/Move.toml b/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/Move.toml new file mode 100644 index 0000000000000..39cc2752ab46d --- /dev/null +++ b/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/Move.toml @@ -0,0 +1,6 @@ +[package] +name = "upgrades" +edition = "2024.beta" # edition = "legacy" to use legacy (pre-2024) Move + +[addresses] +upgrades = "0x0" diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/sources/UpgradeErrors.move b/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/sources/UpgradeErrors.move new file mode 100644 index 0000000000000..1968cfdbf6878 --- /dev/null +++ b/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/sources/UpgradeErrors.move @@ -0,0 +1,30 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +/// Module: UpgradeErrors + +#[allow(unused_field)] +module upgrades::upgrades { + // struct missing + public fun func_with_wrong_param(a: u32): u64 { + 0 + } + + public fun func_with_wrong_return(): u32 { + 0 + } + + public fun func_with_wrong_param_and_return(a: u32): u32 { + 0 + } + + public fun func_with_wrong_param_length(a: u64): u64 { + 0 + } + + public fun func_with_wrong_return_length(): u64 { + 0 + } + +} + diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__function_signature.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__function_signature.snap new file mode 100644 index 0000000000000..f96ae74d0105c --- /dev/null +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__function_signature.snap @@ -0,0 +1,54 @@ +--- +source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs +expression: err.to_string() +--- +Upgrade compatibility check failed: +error: Function signature mismatch + ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/sources/UpgradeErrors.move:9:38 + │ +9 │ public fun func_with_wrong_param(a: u32): u64 { + │ ^ Function 'func_with_wrong_param' unexpected parameter u32 at position 0, expected u64 + │ + = Functions are part of a module's public interface and cannot be changed during an upgrade, restore the original function's parameters for function 'func_with_wrong_param'. + +error: Function signature mismatch + ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/sources/UpgradeErrors.move:17:49 + │ +17 │ public fun func_with_wrong_param_and_return(a: u32): u32 { + │ ^ Function 'func_with_wrong_param_and_return' unexpected parameter u32 at position 0, expected u64 + │ + = Functions are part of a module's public interface and cannot be changed during an upgrade, restore the original function's parameters for function 'func_with_wrong_param_and_return'. + +error: Function signature mismatch + ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/sources/UpgradeErrors.move:17:58 + │ +17 │ public fun func_with_wrong_param_and_return(a: u32): u32 { + │ ^^^ Module 'upgrades' function 'func_with_wrong_param_and_return' has an unexpected return type u32, expected u64 + │ + = Functions are part of a module's public interface and cannot be changed during an upgrade, restore the original function's return types for function 'func_with_wrong_param_and_return'. + +error: Function signature mismatch + ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/sources/UpgradeErrors.move:21:16 + │ +21 │ public fun func_with_wrong_param_length(a: u64): u64 { + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Function 'func_with_wrong_param_length' expected 2 parameters, have 1 + │ + = Functions are part of a module's public interface and cannot be changed during an upgrade, restore the original function's parameters for function 'func_with_wrong_param_length', expected 2 parameters. + +error: Function signature mismatch + ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/sources/UpgradeErrors.move:13:42 + │ +13 │ public fun func_with_wrong_return(): u32 { + │ ^^^ Module 'upgrades' function 'func_with_wrong_return' has an unexpected return type u32, expected u64 + │ + = Functions are part of a module's public interface and cannot be changed during an upgrade, restore the original function's return types for function 'func_with_wrong_return'. + +error: Function signature mismatch + ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/function_signature_v2/sources/UpgradeErrors.move:25:16 + │ +25 │ public fun func_with_wrong_return_length(): u64 { + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Function 'func_with_wrong_return_length' expected to have 2 return type(s), have 1 + │ + = Functions are part of a module's public interface and cannot be changed during an upgrade, restore the original function's return types for function 'func_with_wrong_return_length'. + + diff --git a/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs b/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs index 8e6d002c712e0..22ec020735437 100644 --- a/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs +++ b/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs @@ -27,6 +27,16 @@ fn test_declarations_missing() { assert_snapshot!(normalize_path(err.to_string())); } +#[test] +fn test_function_signature() { + let (pkg_v1, pkg_v2) = get_packages("function_signature"); + let result = compare_packages(pkg_v1, pkg_v2); + + assert!(result.is_err()); + let err = result.unwrap_err(); + assert_snapshot!(err.to_string()); +} + #[test] fn test_friend_link_ok() { let (pkg_v1, pkg_v2) = get_packages("friend_linking"); diff --git a/crates/sui/src/upgrade_compatibility.rs b/crates/sui/src/upgrade_compatibility.rs index 9d41b8d19a51a..d9ef8423f64d4 100644 --- a/crates/sui/src/upgrade_compatibility.rs +++ b/crates/sui/src/upgrade_compatibility.rs @@ -6,7 +6,7 @@ mod upgrade_compatibility_tests; use std::collections::hash_map::Entry::{Occupied, Vacant}; -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use std::fs; use std::io::{stdout, IsTerminal}; @@ -15,6 +15,7 @@ use codespan_reporting::diagnostic::{Diagnostic, Label}; use codespan_reporting::files::SimpleFiles; use codespan_reporting::term; +use move_binary_format::file_format::{FunctionDefinitionIndex, TableIndex}; use move_binary_format::{ compatibility::Compatibility, compatibility_mode::CompatibilityMode, @@ -114,125 +115,6 @@ 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 { @@ -472,6 +354,56 @@ impl CompatibilityMode for CliCompatibilityMode { } } +#[allow(dead_code)] +struct IdentifierTableLookup { + struct_identifier_to_index: BTreeMap, + enum_identifier_to_index: BTreeMap, + function_identifier_to_index: BTreeMap, +} + +fn table_index(compiled_module: &CompiledModule) -> IdentifierTableLookup { + // for each in compiled module + let struct_identifier_to_index: BTreeMap = compiled_module + .struct_defs() + .iter() + .enumerate() + .map(|(i, d)| { + // get the identifier of the struct + let s_id = compiled_module + .identifier_at(compiled_module.datatype_handle_at(d.struct_handle).name); + (s_id.to_owned(), i as TableIndex) + }) + .collect(); + + let enum_identifier_to_index: BTreeMap = compiled_module + .enum_defs() + .iter() + .enumerate() + .map(|(i, d)| { + let e_id = compiled_module + .identifier_at(compiled_module.datatype_handle_at(d.enum_handle).name); + (e_id.to_owned(), i as TableIndex) + }) + .collect(); + + let function_identifier_to_index: BTreeMap = compiled_module + .function_defs() + .iter() + .enumerate() + .map(|(i, d)| { + let f_id = + compiled_module.identifier_at(compiled_module.function_handle_at(d.function).name); + (f_id.to_owned(), i as TableIndex) + }) + .collect(); + + IdentifierTableLookup { + struct_identifier_to_index, + enum_identifier_to_index, + function_identifier_to_index, + } +} + /// Check the upgrade compatibility of a new package with an existing on-chain package. pub(crate) async fn check_compatibility( client: &SuiClient, @@ -517,6 +449,11 @@ fn compare_packages( .map(|m| (m.self_id().name().to_owned(), m.clone())) .collect(); + let lookup: HashMap = existing_modules + .iter() + .map(|m| (m.self_id().name().to_owned(), table_index(m))) + .collect(); + let errors: Vec<(Identifier, UpgradeCompatibilityModeError)> = existing_modules .iter() .flat_map(|existing_module| { @@ -549,7 +486,7 @@ fn compare_packages( } let mut files = SimpleFiles::new(); - let config = codespan_reporting::term::Config::default(); + let config = term::Config::default(); let mut writer; if stdout().is_terminal() { writer = term::termcolor::Buffer::ansi(); @@ -558,7 +495,6 @@ fn compare_packages( } let mut file_id_map = HashMap::new(); - for (name, err) in errors { let compiled_unit_with_source = new_package .package @@ -575,52 +511,53 @@ fn compare_packages( } }; - term::emit( - &mut writer, - &config, - &files, - &diag_from_error(err, compiled_unit_with_source, file_id), - )?; + for diag in diag_from_error(&err, compiled_unit_with_source, file_id, &lookup[&name]) + .iter() + .flatten() + { + term::emit(&mut writer, &config, &files, diag).context("Unable to emit error")?; + } } Err(anyhow!( - "Upgrade compatibility check failed:\n{}", + "{}\nUpgrade failed, this package requires changes to be compatible with the existing package. It's upgrade policy is set to 'Compatible'.", 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, + error: &UpgradeCompatibilityModeError, compiled_unit_with_source: &CompiledUnitWithSource, file_id: usize, -) -> Diagnostic { + lookup: &IdentifierTableLookup, +) -> Result>, Error> { match error { - UpgradeCompatibilityModeError::StructMissing { name, .. } => missing_definition_diag( - "struct", - name.to_string(), - compiled_unit_with_source, - file_id, - ), + UpgradeCompatibilityModeError::StructMissing { name, .. } => { + missing_definition_diag("struct", &name, compiled_unit_with_source, file_id) + } UpgradeCompatibilityModeError::EnumMissing { name, .. } => { - missing_definition_diag("enum", name.to_string(), compiled_unit_with_source, file_id) + missing_definition_diag("enum", &name, compiled_unit_with_source, file_id) } UpgradeCompatibilityModeError::FunctionMissingPublic { name, .. } => { - missing_definition_diag( - "public function", - name.to_string(), - compiled_unit_with_source, - file_id, - ) + missing_definition_diag("public function", &name, compiled_unit_with_source, file_id) } UpgradeCompatibilityModeError::FunctionMissingEntry { name, .. } => { - missing_definition_diag( - "entry function", - name.to_string(), - compiled_unit_with_source, - file_id, - ) + missing_definition_diag("entry function", &name, compiled_unit_with_source, file_id) } + + UpgradeCompatibilityModeError::FunctionSignatureMismatch { + name, + old_function, + new_function, + } => function_signature_mismatch_diag( + &name, + old_function, + new_function, + compiled_unit_with_source, + file_id, + lookup, + ), _ => todo!("Implement diag_from_error for {:?}", error), } } @@ -631,7 +568,7 @@ fn missing_definition_diag( identifier_name: &Identifier, compiled_unit_with_source: &CompiledUnitWithSource, file_id: usize, -) -> Diagnostic { +) -> Result>, Error> { let module_name = compiled_unit_with_source.unit.name; let start = compiled_unit_with_source @@ -646,7 +583,7 @@ fn missing_definition_diag( .definition_location .end() as usize; - Diagnostic::error() + Ok(vec![Diagnostic::error() .with_message(format!("{declaration_kind} is missing")) .with_labels(vec![Label::primary(file_id, start..end).with_message( format!( @@ -655,39 +592,146 @@ fn missing_definition_diag( )]) .with_notes(vec![format!( "{declaration_kind}s are part of a module's public interface and cannot be removed or changed during an upgrade, add back the {declaration_kind} '{identifier_name}'." - )]) + )])]) } -fn diag_from_error( - error: UpgradeCompatibilityModeError, + +/// Return a diagnostic for a function signature mismatch. +/// start by checking the lengths of the parameters and returns and return a diagnostic if they are different +/// if the lengths are the same check each parameter piece wise and return a diagnostic for each mismatch +fn function_signature_mismatch_diag( + function_name: &Identifier, + old_function: &Function, + new_function: &Function, 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, - ) + lookup: &IdentifierTableLookup, +) -> Result>, Error> { + let module_name = compiled_unit_with_source.unit.name; + let old_func_index = lookup + .function_identifier_to_index + .get(function_name) + .context("Unable to get function index")?; + + let new_func_sourcemap = compiled_unit_with_source + .unit + .source_map + .get_function_source_map(FunctionDefinitionIndex::new(*old_func_index)) + .context("Unable to get function source map")?; + + let identifier_start = new_func_sourcemap.definition_location.start() as usize; + let identifier_end = new_func_sourcemap.definition_location.end() as usize; + + let mut diags = vec![]; + + // handle function arguments + if old_function.parameters.len() != new_function.parameters.len() { + diags.push( + Diagnostic::error() + .with_message("Function signature mismatch") + .with_labels(vec![Label::primary( + file_id, + identifier_start..identifier_end, + ) + .with_message(format!( + "Function '{function_name}' expected {} parameters, have {}", + old_function.parameters.len(), + new_function.parameters.len() + ))]) + .with_notes(vec![format!( + "Functions are part of a module's public interface and cannot be changed during an upgrade, restore the original function's parameters for function '{function_name}', expected {} parameters.", + old_function.parameters.len() + )]), + ); + } else if old_function.parameters != new_function.parameters { + for ((i, old_param), new_param) in old_function + .parameters + .iter() + .enumerate() + .zip(new_function.parameters.iter()) + { + if old_param != new_param { + let start = new_func_sourcemap + .parameters + .get(i) + .context("Unable to get parameter location")? + .1 + .start() as usize; + + let end = new_func_sourcemap + .parameters + .get(i) + .context("Unable to get parameter location")? + .1 + .end() as usize; + + diags.push( + Diagnostic::error() + .with_message("Function signature mismatch") + .with_labels(vec![Label::primary(file_id, start..end).with_message( + format!("Function '{function_name}' unexpected parameter {new_param} at position {i}, expected {old_param}"), + )]) + .with_notes(vec![format!( + "Functions are part of a module's public interface and cannot be changed during an upgrade, restore the original function's parameters for function '{function_name}'." + )]), + ); + } } - UpgradeCompatibilityModeError::FunctionMissingEntry { name, .. } => { - missing_definition_diag( - "entry function", - name.to_string(), - compiled_unit_with_source, - file_id, - ) + } + + // handle return + if old_function.return_.len() != new_function.return_.len() { + diags.push( + Diagnostic::error() + .with_message("Function signature mismatch") + .with_labels(vec![Label::primary( + file_id, + identifier_start..identifier_end, + ) + .with_message(format!( + "Function '{function_name}' expected to have {} return type(s), have {}", + old_function.return_.len(), + new_function.return_.len() + ))]) + .with_notes(vec![format!( + "Functions are part of a module's public interface and cannot be changed during an upgrade, restore the original function's return types for function '{function_name}'." + )]), + ); + } else if old_function.return_ != new_function.return_ { + for ((i, old_return), new_return) in old_function + .return_ + .iter() + .enumerate() + .zip(new_function.return_.iter()) + { + let returns = new_func_sourcemap + .returns + .get(i) + .context("Unable to get return location")?; + let start = returns.start() as usize; + let end = returns.end() as usize; + + if old_return != new_return { + diags.push( + Diagnostic::error() + .with_message("Function signature mismatch") + .with_labels(vec![Label::primary( + file_id, + start..end + ) + .with_message( + if new_function.return_.len() == 1 { + format!("Module '{module_name}' function '{function_name}' has an unexpected return type {new_return}, expected {old_return}") + } else { + format!("Module '{module_name}' function '{function_name}' unexpected return type {new_return} at position {i}, expected {old_return}") + }, + )]) + .with_notes(vec![format!( + "Functions are part of a module's public interface and cannot be changed during an upgrade, restore the original function's return types for function '{function_name}'." + )]), + ); + } } - _ => todo!("Implement diag_from_error for {:?}", error), } + + Ok(diags) }