upgrade errors advanced function signatures #20663
Conversation
jordanjennings-mysten
requested review from
amnn,
tnowacki and
stefan-mysten
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
jordanjennings-mysten
temporarily deployed
to
sui-typescript-aws-kms-test-env
GitHub Actions
Inactive
| ┌─ /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' |
There was a problem hiding this comment.
maybe?
| │ ^ Unexpected parameter '0x0::upgrades::StructB', expected '0x0::upgrades::StructA' | |
| │ ^ Unexpected parameter with type '0x0::upgrades::StructB', expected '0x0::upgrades::StructA' |
There was a problem hiding this comment.
+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.
There was a problem hiding this comment.
Sorry to leave post-landing notes -- I think we still need to handle the following cases:
- generic structs and enums in function signatures
- type parameters in struct and variant fields.
I've also left a suggestion to clean-up the allocation pattern for format_params. Not a big deal in this setting, but something we generally want to be careful about when dealing with recursive structures like types.
| ┌─ /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' |
There was a problem hiding this comment.
+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.
| Type::Reference(t) => { | ||
| format!("&{}", format_param(t, type_params, secondary)?) | ||
| } | ||
| _ => format!("{}", param), |
There was a problem hiding this comment.
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!
| @@ -895,6 +896,36 @@ fn function_lost_public( | |||
| Ok(diags) | |||
| } | |||
|
|
|||
| fn format_param( | |||
There was a problem hiding this comment.
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.
Description
function signature upgrade errors test and improve formatting for:
Test plan
How did you test the new or updated feature?
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.