Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upgrade errors advanced function signatures #20663

Merged
merged 2 commits into from
Dec 19, 2024
Merged

upgrade errors advanced function signatures #20663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d464730
upgrade errors improve function type param handling
jordanjennings-mysten Dec 17, 2024
ac920af
add vector, mutable and immutable references
jordanjennings-mysten Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion .../sui/src/unit_tests/fixtures/upgrade_errors/function_errors_v1/sources/UpgradeErrors.move
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,37 @@ module upgrades::upgrades {
public fun func_with_wrong_return_length(): (u64, u64) {
(0,0)
}
}

public struct StructA has drop {
x: u64
}

public struct StructB has drop {
x: u32
}

// change argument from A to B
public fun func_with_wrong_struct_param(a: StructA): u64 {
0
}

// change from reference to value
public fun ref_to_value(a: &u32): u64 {
0
}

// value to ref u32
public fun value_to_ref(a: u32): u64 {
0
}

// mutable to immutable reference
public fun mutable_to_immutable_ref(a: &mut u32): u64 {
0
}

// immutable to mutable reference
public fun immutable_to_mutable_ref(a: &u32): u64 {
0
}
}
33 changes: 33 additions & 0 deletions .../sui/src/unit_tests/fixtures/upgrade_errors/function_errors_v2/sources/UpgradeErrors.move
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,38 @@ module upgrades::upgrades {
public fun func_with_wrong_return_length(): u64 {
0
}

public struct StructA has drop {
x: u64
}

public struct StructB has drop {
x: u32
}

// changed argument from A to B
public fun func_with_wrong_struct_param(a: StructB): u64 {
0
}

// changed from reference to value
public fun ref_to_value(a: u32): u64 {
0
}

// u32 as ref
public fun value_to_ref(a: &u32): u64 {
0
}

// mutable to immutable reference
public fun mutable_to_immutable_ref(a: &u32): u64 {
0
}

// immutable to mutable reference
public fun immutable_to_mutable_ref(a: &mut u32): u64 {
0
}
}

13 changes: 12 additions & 1 deletion ...ui/src/unit_tests/fixtures/upgrade_errors/type_param_errors_v1/sources/UpgradeErrors.move
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,16 @@ module upgrades::upgrades {

// remove constraint (no effect)
public fun remove_constraint<T: copy>(a: T): T { return a }
}

// swap type params
public fun swap_type_params<T: drop, U: drop + copy>(a: T, b: U): T { return a }

// swap return type params
public fun swap_type_params_return<T: drop, U: drop + copy>(a: T, b: U): T { return a }

// change type on vector
public fun vec_changed(_: vector<u32>) {}

// change type param on vector
public fun vec_changed_type_param<T: drop, U: drop + copy>(_: vector<T>) {}
}
12 changes: 12 additions & 0 deletions ...ui/src/unit_tests/fixtures/upgrade_errors/type_param_errors_v2/sources/UpgradeErrors.move
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,17 @@ module upgrades::upgrades {

// constraint removed (no effect)
public fun remove_constraint<T>(a: T): T { return a }

// type params swapped
public fun swap_type_params<T: drop, U: drop + copy>(a: U, b: T): T { return b }

// return type params swapped U and T
public fun swap_type_params_return<T: drop, U: drop + copy>(a: T, b: U): U { return b }

// change type on vector
public fun vec_changed(_: vector<u32>) {}

// change type param on vector
public fun vec_changed_type_param<T: drop, U: drop + copy>(_: vector<U>) {}
}

2 changes: 1 addition & 1 deletion ...rc/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__all.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/all_v2/sources/UpgradeErrors.move:88:36
│
88 │ public fun function_change_arg(a: u8) {} // now has u8 instead of u64
│ ^ Unexpected parameter u8, expected u64
│ ^ Unexpected parameter 'u8', expected 'u64'
│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's parameter for function 'function_change_arg'.
Expand Down
53 changes: 49 additions & 4 deletions ...it_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__function.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/function_errors_v2/sources/UpgradeErrors.move:9:38
│
9 │ public fun func_with_wrong_param(a: u32): u64 {
│ ^ Unexpected parameter u32, expected u64
│ ^ Unexpected parameter 'u32', expected 'u64'
│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's parameter for function 'func_with_wrong_param'.
Expand All @@ -15,7 +15,7 @@ error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/function_errors_v2/sources/UpgradeErrors.move:14:42
│
14 │ public fun func_with_wrong_return(): u32 {
│ ^^^ Unexpected return type u32, expected u64
│ ^^^ 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 type for function 'func_with_wrong_return'.
Expand All @@ -24,7 +24,7 @@ error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/function_errors_v2/sources/UpgradeErrors.move:19:49
│
19 │ public fun func_with_wrong_param_and_return(a: u32): u32 {
│ ^ Unexpected parameter u32, expected u64
│ ^ Unexpected parameter 'u32', expected 'u64'
│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's parameter for function 'func_with_wrong_param_and_return'.
Expand All @@ -33,7 +33,7 @@ error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/function_errors_v2/sources/UpgradeErrors.move:19:58
│
19 │ public fun func_with_wrong_param_and_return(a: u32): u32 {
│ ^^^ Unexpected return type u32, expected u64
│ ^^^ 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 type for function 'func_with_wrong_param_and_return'.
Expand All @@ -56,5 +56,50 @@ error[Compatibility E03001]: function signature mismatch
= 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'.

error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/function_errors_v2/sources/UpgradeErrors.move:42:45
│
42 │ public fun func_with_wrong_struct_param(a: StructB): u64 {
│ ^ Unexpected parameter '0x0::upgrades::StructB', expected '0x0::upgrades::StructA'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe?

Suggested change
│ ^ Unexpected parameter '0x0::upgrades::StructB', expected '0x0::upgrades::StructA'
│ ^ Unexpected parameter with type '0x0::upgrades::StructB', expected '0x0::upgrades::StructA'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to this -- in the binary representation, all we see is a type, but we should clarify for the sake of people reading source code.

│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's parameter for function 'func_with_wrong_struct_param'.

error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/function_errors_v2/sources/UpgradeErrors.move:47:29
│
47 │ public fun ref_to_value(a: u32): u64 {
│ ^ Unexpected parameter 'u32', expected '&u32'
│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's parameter for function 'ref_to_value'.

error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/function_errors_v2/sources/UpgradeErrors.move:52:29
│
52 │ public fun value_to_ref(a: &u32): u64 {
│ ^ Unexpected parameter '&u32', expected 'u32'
│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's parameter for function 'value_to_ref'.

error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/function_errors_v2/sources/UpgradeErrors.move:57:41
│
57 │ public fun mutable_to_immutable_ref(a: &u32): u64 {
│ ^ Unexpected parameter '&u32', expected '&mut u32'
│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's parameter for function 'mutable_to_immutable_ref'.

error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/function_errors_v2/sources/UpgradeErrors.move:62:41
│
62 │ public fun immutable_to_mutable_ref(a: &mut u32): u64 {
│ ^ Unexpected parameter '&mut u32', expected '&u32'
│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's parameter for function 'immutable_to_mutable_ref'.


Upgrade failed, this package requires changes to be compatible with the existing package. Its upgrade policy is set to 'compatible'.
44 changes: 44 additions & 0 deletions ..._tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__type_param.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,5 +111,49 @@ error[Compatibility E01005]: type parameter mismatch
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's type parameter for function 'add_constraint'.

error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/type_param_errors_v2/sources/UpgradeErrors.move:58:58
│
58 │ public fun swap_type_params<T: drop, U: drop + copy>(a: U, b: T): T { return b }
│ - ^ Unexpected parameter 'U', expected 'T'
│ │
│ Type parameter 'T' is defined here
│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's parameters for function 'swap_type_params'.

error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/type_param_errors_v2/sources/UpgradeErrors.move:58:64
│
58 │ public fun swap_type_params<T: drop, U: drop + copy>(a: U, b: T): T { return b }
│ - ^ Unexpected parameter 'T', expected 'U'
│ │
│ Type parameter 'U' is defined here
│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's parameters for function 'swap_type_params'.

error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/type_param_errors_v2/sources/UpgradeErrors.move:61:78
│
61 │ public fun swap_type_params_return<T: drop, U: drop + copy>(a: T, b: U): U { return b }
│ - ^ Unexpected return type 'U', expected 'T'
│ │
│ Type parameter 'T' is defined here
│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's return type for function 'swap_type_params_return'.

error[Compatibility E03001]: function signature mismatch
┌─ /fixtures/upgrade_errors/type_param_errors_v2/sources/UpgradeErrors.move:67:64
│
67 │ public fun vec_changed_type_param<T: drop, U: drop + copy>(_: vector<U>) {}
│ - ^ Unexpected parameter 'vector<U>', expected 'vector<T>'
│ │
│ Type parameter 'T' is defined here
│
= Functions are part of a module's public interface and cannot be changed during an upgrade.
= Restore the original function's parameter for function 'vec_changed_type_param'.


Upgrade failed, this package requires changes to be compatible with the existing package. Its upgrade policy is set to 'compatible'.
91 changes: 71 additions & 20 deletions crates/sui/src/upgrade_compatibility.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ use move_binary_format::file_format::{
AbilitySet, DatatypeTyParameter, EnumDefinitionIndex, FunctionDefinitionIndex,
StructDefinitionIndex, TableIndex,
};
use move_binary_format::normalized::Type;
use move_binary_format::{
compatibility::Compatibility,
compatibility_mode::CompatibilityMode,
Expand Down Expand Up @@ -895,6 +896,36 @@ fn function_lost_public(
Ok(diags)
}

fn format_param(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, this function will behave more nicely when it comes to allocations if you structure it to accept a buffer to write into, rather than return a string:

fn format_params(
    param: &Type,
    type_params: Vec<SourceName>,
    output: &mut String,
    secondary: &mut Vec<(Loc, String)>,
) -> Result<(), Error>;

Instead of format!(...) you can use write!(output, ...).unwrap() -- it is safe to unwrap because writing to a string doesn't panic. With this pattern, you don't end up allocating small temporary strings and then adding them onto the larger main output string.

param: &Type,
type_params: Vec<SourceName>,
secondary: &mut Vec<(Loc, String)>,
) -> Result<String, Error> {
Ok(match param {
Type::TypeParameter(t) => {
let type_param = type_params
.get(*t as usize)
.context("Unable to get type param location")?;

secondary.push((
type_param.1,
format!("Type parameter '{}' is defined here", &type_param.0),
));
type_param.0.to_string()
}
Type::Vector(t) => {
format!("vector<{}>", format_param(t, type_params, secondary)?)
}
Type::MutableReference(t) => {
format!("&mut {}", format_param(t, type_params, secondary)?)
}
Type::Reference(t) => {
format!("&{}", format_param(t, type_params, secondary)?)
}
_ => format!("{}", param),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this does not handle generic structs and enums (if they contain type parameters, you won't find them and replace them with their type param name). Sorry in advance if I'm pointing something out that you addressed in some later PR, I know this is something we talked about on Wednesday!

})
}

/// 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
Expand Down Expand Up @@ -961,13 +992,25 @@ fn function_signature_mismatch_diag(
.context("Unable to get parameter location")?
.1;

let mut secondary = Vec::new();

let old_param = format_param(
old_param,
func_sourcemap.type_parameters.clone(),
&mut secondary,
)?;
let new_param = format_param(
new_param,
func_sourcemap.type_parameters.clone(),
&mut Vec::new(),
)?;

let label = format!("Unexpected parameter '{new_param}', expected '{old_param}'");

diags.add(Diagnostic::new(
Functions_::SignatureMismatch,
(
param_loc,
format!("Unexpected parameter {new_param}, expected {old_param}"),
),
Vec::<(Loc, String)>::new(),
(param_loc, label),
secondary,
vec![
"Functions are part of a module's public interface \
and cannot be changed during an upgrade."
Expand Down Expand Up @@ -1117,23 +1160,31 @@ fn function_signature_mismatch_diag(
.context("Unable to get return location")?;

if old_return != new_return {
let mut secondary = Vec::new();

let old_return = format_param(
old_return,
func_sourcemap.type_parameters.clone(),
&mut secondary,
)?;
let new_return = format_param(
new_return,
func_sourcemap.type_parameters.clone(),
&mut Vec::new(),
)?;

let label = if new_function.return_.len() == 1 {
format!("Unexpected return type '{new_return}', expected '{old_return}'")
} else {
format!(
"Unexpected return type '{new_return}' at position {i}, expected '{old_return}'"
)
};

diags.add(Diagnostic::new(
Functions_::SignatureMismatch,
(
*return_,
if new_function.return_.len() == 1 {
format!(
"Unexpected return type {new_return}, \
expected {old_return}"
)
} else {
format!(
"Unexpected return type {new_return} at \
position {i}, expected {old_return}"
)
},
),
Vec::<(Loc, String)>::new(),
(*return_, label),
secondary,
vec![
"Functions are part of a module's public interface \
and cannot be changed during an upgrade."
Expand Down
Loading