From 7718359b8b982706c722bb87b977b1da6be5c42c Mon Sep 17 00:00:00 2001 From: Jort Date: Thu, 12 Dec 2024 13:18:44 -0800 Subject: [PATCH 1/2] pull out field error formatting, generalize to positional fields --- .../enum_errors_v1/sources/UpgradeErrors.move | 38 ++- .../enum_errors_v2/sources/UpgradeErrors.move | 35 ++- .../sources/UpgradeErrors.move | 64 ++++- .../sources/UpgradeErrors.move | 64 ++++- ...ty__upgrade_compatibility_tests__enum.snap | 96 ++++++- ...__upgrade_compatibility_tests__struct.snap | 145 +++++++++- crates/sui/src/upgrade_compatibility.rs | 260 +++++++++++------- 7 files changed, 579 insertions(+), 123 deletions(-) diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/enum_errors_v1/sources/UpgradeErrors.move b/crates/sui/src/unit_tests/fixtures/upgrade_errors/enum_errors_v1/sources/UpgradeErrors.move index 6c0c47b58b78b..f0e18832d1e33 100644 --- a/crates/sui/src/unit_tests/fixtures/upgrade_errors/enum_errors_v1/sources/UpgradeErrors.move +++ b/crates/sui/src/unit_tests/fixtures/upgrade_errors/enum_errors_v1/sources/UpgradeErrors.move @@ -10,11 +10,11 @@ module upgrades::upgrades { A, } - public enum EnumRemoveAbility has copy, drop { + public enum EnumRemoveAbility has copy, drop { // remove drop A, } - public enum EnumAddAndRemoveAbility has copy, drop { + public enum EnumAddAndRemoveAbility has copy, drop { // change drop to store A, } @@ -45,6 +45,7 @@ module upgrades::upgrades { C, // to be removed } + // with types public enum EnumAddAbilityWithTypes has copy { // add drop A { a: u8 }, } @@ -73,5 +74,38 @@ module upgrades::upgrades { B { b: u8 }, // to be changed to C // D { d: u8 }, to be added } + + public enum EnumChangeAndRemoveVariantWithPositionalTypes { + A(u8), + B(u8), // to be changed to C + C(u8), // to be removed + } + + public enum EnumChangePositionalType { + A, // add u8 + B(u8), // to be changed to u16 + C(u8, u8), // remove u8 + D(u8) // remove last u8 + } + + public struct ChangeFieldA { + a: u32, + } + + public struct ChangeFieldB { + b: u32, + } + + public enum EnumWithPositionalChanged { + A(ChangeFieldA), // change to ChangeFieldB + } + + public enum EnumWithNamedChanged { + A { + x: ChangeFieldA, + y: ChangeFieldA, + z: ChangeFieldA, // change to ChangeFieldB + }, + } } diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move b/crates/sui/src/unit_tests/fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move index a6a9d2ff8bbd8..887ecdf66d2e6 100644 --- a/crates/sui/src/unit_tests/fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move +++ b/crates/sui/src/unit_tests/fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move @@ -14,7 +14,7 @@ module upgrades::upgrades { A, } - public enum EnumAddAndRemoveAbility has copy, store { + public enum EnumAddAndRemoveAbility has copy, store { // change drop to store A, } @@ -75,5 +75,38 @@ module upgrades::upgrades { D { d: u8 }, // added } + public enum EnumChangeAndRemoveVariantWithPositionalTypes { + A(u8), + C(u8), // changed to C + // C(u8) removed + } + + public enum EnumChangePositionalType { + A(u8), // add u8 + B(u16), // to be changed to u16 + C(u8), // removed u8 + D, // removed last u8 + } + + public struct ChangeFieldA { + a: u32, + } + + public struct ChangeFieldB { + b: u32, + } + + public enum EnumWithPositionalChanged { + A(ChangeFieldB), // changed to ChangeFieldB + } + + public enum EnumWithNamedChanged { + A { + x: ChangeFieldA, + y: ChangeFieldA, + z: ChangeFieldB, // changed to ChangeFieldB + }, + } + } diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_errors_v1/sources/UpgradeErrors.move b/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_errors_v1/sources/UpgradeErrors.move index eecbceeacfa24..59862adc7b9c1 100644 --- a/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_errors_v1/sources/UpgradeErrors.move +++ b/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_errors_v1/sources/UpgradeErrors.move @@ -14,9 +14,23 @@ module upgrades::upgrades { public struct AddMultipleAbilities {} - // field mismatch - public struct AddField {} - // remove field + // add field to empty struct + public struct AddFieldToEmpty { + // add a, + } + + // add fields + public struct AddField { + a: u32 + // b + } + + // remove field from struct with one field + public struct RemoveOnlyField { + a: u64, + } + + // remove field from struct with multiple fields public struct RemoveField { a: u64, b: u64, // remove this field @@ -33,5 +47,47 @@ module upgrades::upgrades { a: u64, b: u64, // change this field type to u32 } -} + // change field name and type + public struct ChangeFieldNameAndType { + a: u64, + b: u64, // change field name to c and type to u32 + } + + // add positional to empty positional struct + public struct EmptyPositionalAdd() // add u64 + + // struct new positional + public struct PositionalAdd(u64, u64) // add u64 + + // struct field missing + public struct PositionalRemove(u64, u64, u64) // remove u64 + + // struct field mismatch + public struct PositionalChange(u32, u32) // change second u32 to u64 + + // add named to empty positional struct + public struct PositionalAddNamed() // change to named { a: u64 } + + // change positional to named + public struct PositionalToNamed(u64) // change to named { a: u64 } + + // change positional to named and change type + public struct PositionalToNamedAndChangeType(u32) // change to named { a: u64 } + + public struct ChangeFieldA { + a: u32, + } + + public struct ChangeFieldB { + b: u32, + } + + // change positional nested struct + public struct ChangePositionalStruct (ChangeFieldA) // change to ChangeFieldB + + // change named nested struct + public struct ChangeNameNestedStruct { + a: ChangeFieldA, // change to ChangeFieldB + } +} diff --git a/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move b/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move index 084bc0997bd2d..9995e75d06701 100644 --- a/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move +++ b/crates/sui/src/unit_tests/fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move @@ -13,15 +13,27 @@ module upgrades::upgrades { public struct RemoveMultipleAbilities has drop {} // remove copy, store public struct AddMultipleAbilities has drop, copy {} - // field mismatch + + // add field to empty struct + public struct AddFieldToEmpty { + a: u64, + } + + // add field public struct AddField { a: u64, - b: u64, + b: u64, // added b } - // remove field + + // remove field from struct with one field + public struct RemoveOnlyField { + // removed a: u64, + } + + // remove field from struct with multiple fields public struct RemoveField { a: u64, - // b removed here + // removed b: u64, } // change field name @@ -35,5 +47,47 @@ module upgrades::upgrades { a: u64, b: u32, // changed to u32 } -} + // change field name and type + public struct ChangeFieldNameAndType { + a: u64, + c: u32, // changed from b to c and u64 to u32 + } + + // add positional to empty positional struct + public struct EmptyPositionalAdd(u64) // removed the u64 + + // struct new positional + public struct PositionalAdd(u64, u64, u64) // added a u64 + + // struct field missing + public struct PositionalRemove(u64, u64) // removed a u64 + + // struct field mismatch + public struct PositionalChange(u32, u64) // changed second u32 to u64 + + // add named to empty positional struct + public struct PositionalAddNamed{ a: u64 } // changed to named from empty positional + + // positional to named + public struct PositionalToNamed{ a: u64 } // changed to named from positional + + // change positional to named and change type + public struct PositionalToNamedAndChangeType{ a: u64 } // changed to named from positional and changed type to u64 + + public struct ChangeFieldA { + a: u32, + } + + public struct ChangeFieldB { + b: u32, + } + + // change positional nested struct + public struct ChangePositionalStruct (ChangeFieldB) // changed to ChangeFieldB + + // change named nested struct + public struct ChangeNameNestedStruct { + a: ChangeFieldB, // changed to ChangeFieldB + } +} diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__enum.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__enum.snap index 305e7a44f555e..b4db9c2859672 100644 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__enum.snap +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__enum.snap @@ -23,7 +23,7 @@ error[Compatibility E01003]: ability mismatch error[Compatibility E01003]: ability mismatch ┌─ /fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move:17:17 │ -17 │ public enum EnumAddAndRemoveAbility has copy, store { +17 │ public enum EnumAddAndRemoveAbility has copy, store { // change drop to store │ ^^^^^^^^^^^^^^^^^^^^^^^ Mismatched abilities: missing 'drop', unexpected 'store' │ = Enums are part of a module's public interface and cannot be changed during an upgrade. @@ -206,5 +206,99 @@ error[Compatibility E02001]: variant mismatch = Enums are part of a module's public interface and cannot be changed during an upgrade. = Restore the original enum's variants for enum 'EnumChangeAndAddVariantWithTypes' including the ordering. +error[Compatibility E02001]: variant mismatch + ┌─ /fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move:78:17 + │ +78 │ public enum EnumChangeAndRemoveVariantWithPositionalTypes { + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Missing variant 'C'. + │ + = Enums are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original enum's variant 'C' for enum 'EnumChangeAndRemoveVariantWithPositionalTypes' including the ordering. + +error[Compatibility E02001]: variant mismatch + ┌─ /fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move:80:9 + │ +78 │ public enum EnumChangeAndRemoveVariantWithPositionalTypes { + │ --------------------------------------------- Enum definition +79 │ A(u8), +80 │ C(u8), // changed to C + │ ^^^^^ Mismatched variant name 'C', expected 'B'. + │ + = Enums are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original enum's variants for enum 'EnumChangeAndRemoveVariantWithPositionalTypes' including the ordering. + +error[Compatibility E01004]: field mismatch + ┌─ /fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move:85:9 + │ +84 │ public enum EnumChangePositionalType { + │ ------------------------ Enum definition +85 │ A(u8), // add u8 + │ ^^^^^ Mismatched variant field count, expected 0, found 1. + │ + = Enums are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original enum's variants for enum 'EnumChangePositionalType' including the ordering. + +error[Compatibility E01004]: field mismatch + ┌─ /fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move:86:9 + │ +84 │ public enum EnumChangePositionalType { + │ ------------------------ Enum definition +85 │ A(u8), // add u8 +86 │ B(u16), // to be changed to u16 + │ ^^^^^^ Mismatched field 'u8' at position 0, expected 'u16'. + │ + = Enums are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original enum's variants for enum 'EnumChangePositionalType' including the ordering. + +error[Compatibility E01004]: field mismatch + ┌─ /fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move:87:9 + │ +84 │ public enum EnumChangePositionalType { + │ ------------------------ Enum definition + · +87 │ C(u8), // removed u8 + │ ^^^^^ Mismatched variant field count, expected 2, found 1. + │ + = Enums are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original enum's variants for enum 'EnumChangePositionalType' including the ordering. + +error[Compatibility E01004]: field mismatch + ┌─ /fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move:88:9 + │ +84 │ public enum EnumChangePositionalType { + │ ------------------------ Enum definition + · +88 │ D, // removed last u8 + │ ^ Mismatched variant field count, expected 1, found 0. + │ + = Enums are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original enum's variants for enum 'EnumChangePositionalType' including the ordering. + +error[Compatibility E01004]: field mismatch + ┌─ /fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move:100:9 + │ + 99 │ public enum EnumWithPositionalChanged { + │ ------------------------- Enum definition +100 │ A(ChangeFieldB), // changed to ChangeFieldB + │ ^^^^^^^^^^^^^^^ Mismatched field '0x0::upgrades::ChangeFieldA' at position 0, expected '0x0::upgrades::ChangeFieldB'. + │ + = Enums are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original enum's variant for enum 'EnumWithPositionalChanged' including the ordering. + +error[Compatibility E01004]: field mismatch + ┌─ /fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move:104:9 + │ +103 │ public enum EnumWithNamedChanged { + │ -------------------- Enum definition +104 │ ╭ A { +105 │ │ x: ChangeFieldA, +106 │ │ y: ChangeFieldA, +107 │ │ z: ChangeFieldB, // changed to ChangeFieldB +108 │ │ }, + │ ╰─────────^ Mismatched field 'z: 0x0::upgrades::ChangeFieldA', expected '0x0::upgrades::ChangeFieldB'. + │ + = Enums are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original enum's variant for enum 'EnumWithNamedChanged' including the ordering. + Upgrade failed, this package requires changes to be compatible with the existing package. Its upgrade policy is set to 'compatible'. diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__struct.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__struct.snap index 5e42dceed67e8..e8fd48dee6562 100644 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__struct.snap +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__struct.snap @@ -48,46 +48,167 @@ error[Compatibility E01003]: ability mismatch = Restore the original abilities of struct 'AddMultipleAbilities': none. error[Compatibility E01002]: type mismatch - ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:17:19 + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:18:19 │ -17 │ public struct AddField { +18 │ public struct AddFieldToEmpty { + │ ^^^^^^^^^^^^^^^ Incorrect number of fields: expected 0, found 1 + │ + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's fields for struct 'AddFieldToEmpty' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:23:19 + │ +23 │ public struct AddField { │ ^^^^^^^^ Incorrect number of fields: expected 1, found 2 │ = Structs are part of a module's public interface and cannot be changed during an upgrade. = Restore the original struct's field for struct 'AddField' including the ordering. error[Compatibility E01002]: type mismatch - ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:22:19 + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:29:19 + │ +29 │ public struct RemoveOnlyField { + │ ^^^^^^^^^^^^^^^ Incorrect number of fields: expected 1, found 0 │ -22 │ public struct RemoveField { + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's field for struct 'RemoveOnlyField' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:34:19 + │ +34 │ public struct RemoveField { │ ^^^^^^^^^^^ Incorrect number of fields: expected 2, found 1 │ = Structs are part of a module's public interface and cannot be changed during an upgrade. = Restore the original struct's fields for struct 'RemoveField' including the ordering. error[Compatibility E01004]: field mismatch - ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:30:9 + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:42:9 │ -28 │ public struct ChangeFieldName { +40 │ public struct ChangeFieldName { │ --------------- Struct definition -29 │ a: u64, -30 │ c: u64, // changed from b to c +41 │ a: u64, +42 │ c: u64, // changed from b to c │ ^ Mismatched field name 'c', expected 'b'. │ = Structs are part of a module's public interface and cannot be changed during an upgrade. = Restore the original struct's fields for struct 'ChangeFieldName' including the ordering. error[Compatibility E01002]: type mismatch - ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:36:9 + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:48:9 │ -34 │ public struct ChangeFieldType { +46 │ public struct ChangeFieldType { │ --------------- Struct definition -35 │ a: u64, -36 │ b: u32, // changed to u32 +47 │ a: u64, +48 │ b: u32, // changed to u32 │ ^ Mismatched field type 'u32', expected 'u64'. │ = Structs are part of a module's public interface and cannot be changed during an upgrade. = Restore the original struct's fields for struct 'ChangeFieldType' including the ordering. +error[Compatibility E01004]: field mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:54:9 + │ +52 │ public struct ChangeFieldNameAndType { + │ ---------------------- Struct definition +53 │ a: u64, +54 │ c: u32, // changed from b to c and u64 to u32 + │ ^ Mismatched field 'c: u32', expected 'b: u64'. + │ + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's fields for struct 'ChangeFieldNameAndType' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:58:19 + │ +58 │ public struct EmptyPositionalAdd(u64) // removed the u64 + │ ^^^^^^^^^^^^^^^^^^ Incorrect number of fields: expected 0, found 1 + │ + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's fields for struct 'EmptyPositionalAdd' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:61:19 + │ +61 │ public struct PositionalAdd(u64, u64, u64) // added a u64 + │ ^^^^^^^^^^^^^ Incorrect number of fields: expected 2, found 3 + │ + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's fields for struct 'PositionalAdd' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:64:19 + │ +64 │ public struct PositionalRemove(u64, u64) // removed a u64 + │ ^^^^^^^^^^^^^^^^ Incorrect number of fields: expected 3, found 2 + │ + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's fields for struct 'PositionalRemove' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:67:41 + │ +67 │ public struct PositionalChange(u32, u64) // changed second u32 to u64 + │ ---------------- ^^^ Mismatched field type 'u64', expected 'u32'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's fields for struct 'PositionalChange' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:70:19 + │ +70 │ public struct PositionalAddNamed{ a: u64 } // changed to named from empty positional + │ ^^^^^^^^^^^^^^^^^^ Incorrect number of fields: expected 0, found 1 + │ + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's fields for struct 'PositionalAddNamed' including the ordering. + +error[Compatibility E01004]: field mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:73:38 + │ +73 │ public struct PositionalToNamed{ a: u64 } // changed to named from positional + │ ----------------- ^ Mismatched field name 'a', expected at position 0. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's field for struct 'PositionalToNamed' including the ordering. + +error[Compatibility E01004]: field mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:76:51 + │ +76 │ public struct PositionalToNamedAndChangeType{ a: u64 } // changed to named from positional and changed type to u64 + │ ------------------------------ ^ Mismatched field 'a: u64', expected 'u32' at position 0. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's field for struct 'PositionalToNamedAndChangeType' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:87:43 + │ +87 │ public struct ChangePositionalStruct (ChangeFieldB) // changed to ChangeFieldB + │ ---------------------- ^^^^^^^^^^^^ Mismatched field type '0x0::upgrades::ChangeFieldB', expected '0x0::upgrades::ChangeFieldA'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's field for struct 'ChangePositionalStruct' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:91:9 + │ +90 │ public struct ChangeNameNestedStruct { + │ ---------------------- Struct definition +91 │ a: ChangeFieldB, // changed to ChangeFieldB + │ ^ Mismatched field type '0x0::upgrades::ChangeFieldB', expected '0x0::upgrades::ChangeFieldA'. + │ + = Structs are part of a module's public interface and cannot be changed during an upgrade. + = Restore the original struct's field for struct 'ChangeNameNestedStruct' including the ordering. + Upgrade failed, this package requires changes to be compatible with the existing package. Its upgrade policy is set to 'compatible'. diff --git a/crates/sui/src/upgrade_compatibility.rs b/crates/sui/src/upgrade_compatibility.rs index 9484188c49e1d..af21adfa72f55 100644 --- a/crates/sui/src/upgrade_compatibility.rs +++ b/crates/sui/src/upgrade_compatibility.rs @@ -1,11 +1,15 @@ // Copyright (c) Mysten Labs, Inc. // SPDX-License-Identifier: Apache-2.0 +#![allow(dead_code)] +#![allow(unused_variables)] + #[path = "unit_tests/upgrade_compatibility_tests.rs"] #[cfg(test)] mod upgrade_compatibility_tests; use anyhow::{anyhow, Context, Error}; +use regex::Regex; use std::collections::{BTreeMap, HashMap, HashSet}; use std::fs; use std::sync::Arc; @@ -20,7 +24,7 @@ use move_binary_format::{ compatibility::Compatibility, compatibility_mode::CompatibilityMode, file_format::Visibility, - normalized::{Enum, Function, Module, Struct}, + normalized::{Enum, Field, Function, Module, Struct, Type, Variant}, CompiledModule, }; use move_bytecode_source_map::source_map::SourceName; @@ -1247,6 +1251,7 @@ fn function_entry_mismatch( Ok(diags) } +/// Return a label string for an ability mismatch. fn ability_mismatch_label( old_abilities: AbilitySet, new_abilities: AbilitySet, @@ -1352,9 +1357,62 @@ fn struct_ability_mismatch_diag( Ok(diags) } +/// Return a diagnostic for an ability mismatch. returns (full version, name, type) +fn field_to_string(field: &Field) -> (String, String, String) { + let mut field_full = format!("'{}: {}'", field.name, field.type_); + let mut field_name = format!("'{}'", field.name); + let field_type = format!("'{}'", field.type_); + + if let Some(pos_num) = Regex::new(r"^pos(\d)+$") + .ok() + .and_then(|r| r.captures(field.name.as_str())) + .and_then(|c| c.get(1)) + .and_then(|m| m.as_str().parse::().ok()) + { + field_name = format!("at position {}", pos_num); + field_full = format!("{} {}", field_type, field_name); + } + + (field_full, field_name, field_type) +} + +/// returns a message for the given field +fn field_mismatch_message(old_field: &Field, new_field: &Field) -> (Declarations, String) { + let (old_field_full, old_field_name, old_field_type) = field_to_string(old_field); + let (new_field_full, new_field_name, new_field_type) = field_to_string(new_field); + + match ( + old_field.name != new_field.name, + old_field.type_ != new_field.type_, + ) { + (true, true) => ( + Declarations::FieldMismatch, + format!( + "Mismatched field {}, expected {}.", + new_field_full, old_field_full + ), + ), + (true, false) => ( + Declarations::FieldMismatch, + format!( + "Mismatched field name {}, expected {}.", + new_field_name, old_field_name + ), + ), + (false, true) => ( + Declarations::TypeMismatch, + format!( + "Mismatched field type {}, expected {}.", + new_field_type, old_field_type + ), + ), + (false, false) => unreachable!("Fields should not be the same"), + } +} + /// Return a diagnostic for a field mismatch -/// start by checking the lengths of the fields and return a diagnostic if they are different -/// if the lengths are the same check each field piece wise and return a diagnostic for each mismatch +/// Start by checking the lengths of the fields and return a diagnostic if they are different. +/// If the lengths are the same check each field piece wise and return a diagnostic for each mismatch. fn struct_field_mismatch_diag( struct_name: &Identifier, old_struct: &Struct, @@ -1377,15 +1435,27 @@ fn struct_field_mismatch_diag( let def_loc = struct_sourcemap.definition_location; - if old_struct.fields.len() != new_struct.fields.len() { + let old_fields: Vec<&Field> = old_struct + .fields + .iter() + .filter(|f| f.name != Identifier::new("dummy_field").unwrap() && f.type_ != Type::Bool) + .collect(); + + let new_fields: Vec<&Field> = new_struct + .fields + .iter() + .filter(|f| f.name != Identifier::new("dummy_field").unwrap() && f.type_ != Type::Bool) + .collect(); + + if old_fields.len() != new_fields.len() { diags.add(Diagnostic::new( Declarations::TypeMismatch, ( def_loc, format!( "Incorrect number of fields: expected {}, found {}", - old_struct.fields.len(), - new_struct.fields.len() + old_fields.len(), + new_fields.len() ), ), Vec::<(Loc, String)>::new(), @@ -1396,50 +1466,19 @@ fn struct_field_mismatch_diag( format!( "Restore the original struct's {} \ for struct '{struct_name}' including the ordering.", - singular_or_plural(old_struct.fields.len(), "field", "fields") + singular_or_plural(old_fields.len(), "field", "fields") ), ], )); - } else if old_struct.fields != new_struct.fields { - for (i, (old_field, new_field)) in old_struct - .fields - .iter() - .zip(new_struct.fields.iter()) - .enumerate() - { + } else if old_fields != new_fields { + for (i, (old_field, new_field)) in old_fields.iter().zip(new_fields.iter()).enumerate() { if old_field != new_field { let field_loc = struct_sourcemap .fields .get(i) .context("Unable to get field location")?; - let (code, label) = match ( - old_field.name != new_field.name, - old_field.type_ != new_field.type_, - ) { - (true, true) => ( - Declarations::FieldMismatch, - format!( - "Mismatched field '{}: {}' expected '{}: {}'.", - new_field.name, new_field.type_, old_field.name, old_field.type_ - ), - ), - (true, false) => ( - Declarations::FieldMismatch, - format!( - "Mismatched field name '{}', expected '{}'.", - new_field.name, old_field.name - ), - ), - (false, true) => ( - Declarations::TypeMismatch, - format!( - "Mismatched field type '{}', expected '{}'.", - new_field.type_, old_field.type_ - ), - ), - (false, false) => unreachable!("Fields should not be the same"), - }; + let (code, label) = field_mismatch_message(old_field, new_field); diags.add(Diagnostic::new( code, @@ -1452,7 +1491,7 @@ fn struct_field_mismatch_diag( format!( "Restore the original struct's {} for \ struct '{struct_name}' including the ordering.", - singular_or_plural(old_struct.fields.len(), "field", "fields") + singular_or_plural(old_fields.len(), "field", "fields") ), ], )); @@ -1560,6 +1599,66 @@ fn enum_ability_mismatch_diag( Ok(diags) } +/// Returns the error code and label for mismatched, missing, or unexpected variant +fn enum_variant_field_error( + old_variant: &Variant, + new_variant: &Variant, + variant_loc: Loc, + def_loc: Loc, +) -> (DiagnosticInfo, Vec) { + if old_variant.fields.len() != new_variant.fields.len() { + return ( + Declarations::FieldMismatch.into(), + vec![format!( + "Mismatched variant field count, expected {}, found {}.", + old_variant.fields.len(), + new_variant.fields.len() + )], + ); + } + + match ( + old_variant.name != new_variant.name, + old_variant.fields != new_variant.fields, + ) { + (true, true) => ( + Enums::VariantMismatch.into(), + vec![format!( + "Mismatched variant '{}', expected '{}'.", + new_variant.name, old_variant.name + )], + ), + (true, false) => ( + Enums::VariantMismatch.into(), + vec![format!( + "Mismatched variant name '{}', expected '{}'.", + new_variant.name, old_variant.name + )], + ), + (false, true) => { + let mut errors: Vec = vec![]; + + for (i, (old_field, new_field)) in old_variant + .fields + .iter() + .zip(new_variant.fields.iter()) + .enumerate() + { + if old_field != new_field { + errors.push(format!( + "Mismatched field {}, expected {}.", + field_to_string(old_field).0, + field_to_string(new_field).2 + )); + } + } + + (Declarations::FieldMismatch.into(), errors) + } + (false, false) => unreachable!("Variants should not be the same"), + } +} + /// Return a diagnostic for a type parameter mismatch /// start by checking the lengths of the type parameters and return a diagnostic if they are different /// if the lengths are the same check each type parameter piece wise and return a diagnostic for each mismatch @@ -1599,65 +1698,26 @@ fn enum_variant_mismatch_diag( .0 .1; - let (code, label): (DiagnosticInfo, String) = match ( - old_variant.name != new_variant.name, - old_variant.fields != new_variant.fields, - ) { - (true, true) => ( - Enums::VariantMismatch.into(), - format!( - "Mismatched variant '{}', expected '{}'.", - new_variant.name, old_variant.name - ), - ), - (true, false) => ( - Enums::VariantMismatch.into(), - format!( - "Mismatched variant name '{}', expected '{}'.", - new_variant.name, old_variant.name - ), - ), - (false, true) => { - let new_variant_fields = new_variant - .fields - .iter() - .map(|f| format!("{:?}", f)) - .collect::>() - .join(", "); - - let old_variant_fields = old_variant - .fields - .iter() - .map(|f| format!("{:?}", f)) - .collect::>() - .join(", "); + let (code, labels) = + enum_variant_field_error(old_variant, new_variant, variant_loc, def_loc); - ( - Declarations::FieldMismatch.into(), - format!( - "Mismatched variant field '{}', expected '{}'.", - new_variant_fields, old_variant_fields - ), - ) - } - (false, false) => unreachable!("Variants should not be the same"), - }; - - diags.add(Diagnostic::new( - code, - (variant_loc, label), - vec![(def_loc, "Enum definition".to_string())], - vec![ - "Enums are part of a module's public interface \ + for label in labels { + diags.add(Diagnostic::new( + code.clone(), + (variant_loc, label), + vec![(def_loc, "Enum definition".to_string())], + vec![ + "Enums are part of a module's public interface \ and cannot be changed during an upgrade." - .to_string(), - format!( - "Restore the original enum's {} for \ + .to_string(), + format!( + "Restore the original enum's {} for \ enum '{enum_name}' including the ordering.", - singular_or_plural(old_enum.variants.len(), "variant", "variants") - ), - ], - )); + singular_or_plural(old_enum.variants.len(), "variant", "variants") + ), + ], + )); + } } } @@ -1901,6 +1961,7 @@ fn type_parameter_diag( Ok(diags) } +/// Return a diagnostic for a type parameter constrant mismatch fn type_param_constraint_labels( old_constraints: AbilitySet, new_constraints: AbilitySet, @@ -1928,6 +1989,7 @@ fn type_param_constraint_labels( )) } +/// Return a diagnostic for a type parameter phantom mismatch fn type_param_phantom_labels(old_phantom: bool, new_phantom: bool) -> Option<(String, String)> { if old_phantom == new_phantom { return None; @@ -1946,6 +2008,7 @@ fn type_param_phantom_labels(old_phantom: bool, new_phantom: bool) -> Option<(St }) } +/// Format a list of items into a human-readable string. fn format_list( items: impl IntoIterator, noun_singular_plural: Option<(&str, &str)>, @@ -1972,6 +2035,7 @@ fn format_list( } } +/// Return a string with the singular or plural form of a word. fn singular_or_plural(n: usize, singular: &str, plural: &str) -> String { if n == 1 { singular.to_string() From 67e93900a095558ef6204c099bea3d1f85ab80f6 Mon Sep 17 00:00:00 2001 From: Jort Date: Fri, 20 Dec 2024 13:34:20 -0800 Subject: [PATCH 2/2] remove allow --- crates/sui/src/upgrade_compatibility.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/crates/sui/src/upgrade_compatibility.rs b/crates/sui/src/upgrade_compatibility.rs index af21adfa72f55..d275ab65cdd60 100644 --- a/crates/sui/src/upgrade_compatibility.rs +++ b/crates/sui/src/upgrade_compatibility.rs @@ -1,9 +1,6 @@ // Copyright (c) Mysten Labs, Inc. // SPDX-License-Identifier: Apache-2.0 -#![allow(dead_code)] -#![allow(unused_variables)] - #[path = "unit_tests/upgrade_compatibility_tests.rs"] #[cfg(test)] mod upgrade_compatibility_tests;