From ca155f399df75a25c695c940c5fc210e8c4c725a Mon Sep 17 00:00:00 2001 From: Jort Date: Tue, 14 Jan 2025 23:17:25 -0800 Subject: [PATCH] improve comments, format type params within structs and enums (#20874) ## Description - improves comments - format type params within structs - remove dead code - refactor formatting fields ## 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. - [ ] Protocol: - [ ] Nodes (Validators and Full nodes): - [ ] gRPC: - [ ] JSON-RPC: - [ ] GraphQL: - [ ] CLI: - [ ] Rust SDK: --- .../enum_errors_v1/sources/UpgradeErrors.move | 2 +- .../enum_errors_v2/sources/UpgradeErrors.move | 4 +- .../sources/UpgradeErrors.move | 13 + .../sources/UpgradeErrors.move | 13 + ...upgrade_compatibility_tests__additive.snap | 6 +- ..._compatibility_tests__addresses_first.snap | 2 +- ...atibility_tests__declarations_missing.snap | 2 +- ...y__upgrade_compatibility_tests__emoji.snap | 2 +- ...y__upgrade_compatibility_tests__empty.snap | 2 +- ...ty__upgrade_compatibility_tests__enum.snap | 28 +- ..._compatibility_tests__package_no_name.snap | 2 +- ...mpatibility_tests__starts_second_line.snap | 2 +- ...__upgrade_compatibility_tests__struct.snap | 114 ++++- ...compatibility_tests__version_mismatch.snap | 2 +- ...grade_compatibility_tests__whitespace.snap | 2 +- .../unit_tests/upgrade_compatibility_tests.rs | 26 +- .../src/upgrade_compatibility/formatting.rs | 168 +++++++ .../mod.rs} | 423 ++++++------------ 18 files changed, 497 insertions(+), 316 deletions(-) create mode 100644 crates/sui/src/upgrade_compatibility/formatting.rs rename crates/sui/src/{upgrade_compatibility.rs => upgrade_compatibility/mod.rs} (87%) 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 f0e18832d1e33..bd127d042773b 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 @@ -85,7 +85,7 @@ module upgrades::upgrades { A, // add u8 B(u8), // to be changed to u16 C(u8, u8), // remove u8 - D(u8) // remove last u8 + D(u8) // remove u8 from last variant } public struct ChangeFieldA { 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 887ecdf66d2e6..12d2a04872009 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 @@ -83,9 +83,9 @@ module upgrades::upgrades { public enum EnumChangePositionalType { A(u8), // add u8 - B(u16), // to be changed to u16 + B(u16), // changed to u16 C(u8), // removed u8 - D, // removed last u8 + D, // removed u8 from variant } public struct ChangeFieldA { 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 59862adc7b9c1..7d62c259ddfec 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 @@ -90,4 +90,17 @@ module upgrades::upgrades { public struct ChangeNameNestedStruct { a: ChangeFieldA, // change to ChangeFieldB } + + // nested struct type param field mismatch + public struct NamedBox { x: A } + public struct NamedTwoBox { x: B, y: C } + + public struct NamedBoxInBox { x: NamedBox> } + public struct NamedBoxInTwoBox { x: NamedTwoBox, NamedBox> } + + public struct PositionalBox(G) + public struct PositionalTwoBox(H, I) + + public struct PositionalBoxInBox(PositionalBox>) + public struct PositionalBoxInTwoBox(PositionalTwoBox, PositionalBox>) } 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 9995e75d06701..9502846dcf0e4 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 @@ -90,4 +90,17 @@ module upgrades::upgrades { public struct ChangeNameNestedStruct { a: ChangeFieldB, // changed to ChangeFieldB } + + + public struct NamedBox { x: u32 } + public struct NamedTwoBox { x: u32, y: u32 } + + public struct NamedBoxInBox { x: u32 } + public struct NamedBoxInTwoBox { x: u32 } + + public struct PositionalBox(u32) + public struct PositionalTwoBox(u32, u32) + + public struct PositionalBoxInBox(u32) + public struct PositionalBoxInTwoBox(u32) } diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__additive.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__additive.snap index 6dfd7a009a152..0402b496262cf 100644 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__additive.snap +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__additive.snap @@ -2,7 +2,7 @@ source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs expression: normalize_path(err.to_string()) --- -error[Compatibility E01008]: missing declaration +error[Compatibility E01007]: missing declaration ┌─ /fixtures/upgrade_errors/additive_errors_v2/sources/UpgradeErrors.move:4:18 │ 4 │ module upgrades::upgrades { @@ -11,7 +11,7 @@ error[Compatibility E01008]: missing declaration = enums cannot be removed or changed during an 'additive' or 'dependency only' upgrade. = add missing enum 'EnumToRemove' back to the module 'upgrades'. -error[Compatibility E01008]: missing declaration +error[Compatibility E01007]: missing declaration ┌─ /fixtures/upgrade_errors/additive_errors_v2/sources/UpgradeErrors.move:4:18 │ 4 │ module upgrades::upgrades { @@ -20,7 +20,7 @@ error[Compatibility E01008]: missing declaration = functions cannot be removed or changed during an 'additive' or 'dependency only' upgrade. = add missing function 'function_to_remove' back to the module 'upgrades'. -error[Compatibility E01008]: missing declaration +error[Compatibility E01007]: missing declaration ┌─ /fixtures/upgrade_errors/additive_errors_v2/sources/UpgradeErrors.move:4:18 │ 4 │ module upgrades::upgrades { diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__addresses_first.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__addresses_first.snap index a9745ea1dd86c..9a60d301b7166 100644 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__addresses_first.snap +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__addresses_first.snap @@ -2,7 +2,7 @@ source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs expression: output --- -error[Compatibility E01007]: module missing +error[Compatibility E01006]: module missing ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/missing_module_toml/addresses_first/Move.toml:4:1 │ 4 │ ╭ [package] 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 index 9bfb076c465fb..eb65354488d68 100644 --- 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 @@ -38,7 +38,7 @@ error[Compatibility E01001]: missing public declaration = structs are part of a module's public interface and cannot be removed or changed during a 'compatible' upgrade. = add missing struct 'StructToBeRemoved' back to the module 'struct_'. -error[Compatibility E01007]: module missing +error[Compatibility E01006]: module missing ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/declaration_errors_v2/Move.toml:1:1 │ 1 │ ╭ [package] diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__emoji.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__emoji.snap index a24bfcb9eb8a9..ac2c0a7a1c9fd 100644 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__emoji.snap +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__emoji.snap @@ -2,7 +2,7 @@ source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs expression: output --- -error[Compatibility E01007]: module missing +error[Compatibility E01006]: module missing ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/missing_module_toml/emoji/Move.toml:2:2 │ 2 │ 😀[package]😀 diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__empty.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__empty.snap index 3fcbbedc9638f..1d573f94245bb 100644 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__empty.snap +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__empty.snap @@ -2,7 +2,7 @@ source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs expression: output --- -error[Compatibility E01007]: module missing +error[Compatibility E01006]: module missing ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/missing_module_toml/empty/Move.toml:1:1 │ 1 │ 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 4e5820f7e0a71..1084b0213f057 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 @@ -57,7 +57,7 @@ error[Compatibility E02001]: variant mismatch │ ----------------- Enum definition 32 │ A, 33 │ C, // changed from B - │ ^ Mismatched variant name 'C', expected 'B'. + │ ^ Mismatched variant 'C', expected 'B'. │ = Enums are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original enum's variants for enum 'EnumChangeVariant' including the ordering. @@ -69,7 +69,7 @@ error[Compatibility E02001]: variant mismatch │ ----------------------- Enum definition 37 │ A, 38 │ C, // to be changed to C - │ ^ Mismatched variant name 'C', expected 'B'. + │ ^ Mismatched variant 'C', expected 'B'. │ = Enums are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original enum's variants for enum 'EnumChangeAndAddVariant' including the ordering. @@ -114,7 +114,7 @@ error[Compatibility E02001]: variant mismatch │ -------------------------- Enum definition 43 │ A, 44 │ C, // changed to C - │ ^ Mismatched variant name 'C', expected 'B'. + │ ^ Mismatched variant 'C', expected 'B'. │ = Enums are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original enum's variants for enum 'EnumChangeAndRemoveVariant' including the ordering. @@ -165,7 +165,7 @@ error[Compatibility E02001]: variant mismatch │ -------------------------- Enum definition 68 │ A { a: u8 }, 69 │ C { b: u8 }, // changed to C - │ ^^^^^^^^^^^ Mismatched variant name 'C', expected 'B'. + │ ^^^^^^^^^^^ Mismatched variant 'C', expected 'B'. │ = Enums are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original enum's variants for enum 'EnumChangeVariantWithTypes' including the ordering. @@ -177,7 +177,7 @@ error[Compatibility E02001]: variant mismatch │ -------------------------------- Enum definition 73 │ A { a: u8 }, 74 │ C { b: u8 }, // to be changed to C - │ ^^^^^^^^^^^ Mismatched variant name 'C', expected 'B'. + │ ^^^^^^^^^^^ Mismatched variant 'C', expected 'B'. │ = Enums are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original enum's variants for enum 'EnumChangeAndAddVariantWithTypes' including the ordering. @@ -222,7 +222,7 @@ error[Compatibility E02001]: variant mismatch │ --------------------------------------------- Enum definition 79 │ A(u8), 80 │ C(u8), // changed to C - │ ^^^^^ Mismatched variant name 'C', expected 'B'. + │ ^^^^^ Mismatched variant 'C', expected 'B'. │ = Enums are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original enum's variants for enum 'EnumChangeAndRemoveVariantWithPositionalTypes' including the ordering. @@ -238,14 +238,14 @@ error[Compatibility E01004]: field mismatch = Enums are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original enum's variants for enum 'EnumChangePositionalType' including the ordering. -error[Compatibility E01004]: field mismatch +error[Compatibility E01002]: type 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'. +86 │ B(u16), // changed to u16 + │ ^^^^^^ Mismatched field type 'u16', expected 'u8'. │ = Enums are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original enum's variants for enum 'EnumChangePositionalType' including the ordering. @@ -268,24 +268,24 @@ error[Compatibility E01004]: field mismatch 84 │ public enum EnumChangePositionalType { │ ------------------------ Enum definition · -88 │ D, // removed last u8 +88 │ D, // removed u8 from variant │ ^ Mismatched variant field count, expected 1, found 0. │ = Enums are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original enum's variants for enum 'EnumChangePositionalType' including the ordering. -error[Compatibility E01004]: field mismatch +error[Compatibility E01002]: type 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'. + │ ^^^^^^^^^^^^^^^ Mismatched field type '0x0::upgrades::ChangeFieldB', expected '0x0::upgrades::ChangeFieldA'. │ = Enums are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original enum's variant for enum 'EnumWithPositionalChanged' including the ordering. -error[Compatibility E01004]: field mismatch +error[Compatibility E01002]: type mismatch ┌─ /fixtures/upgrade_errors/enum_errors_v2/sources/UpgradeErrors.move:104:9 │ 103 │ public enum EnumWithNamedChanged { @@ -295,7 +295,7 @@ error[Compatibility E01004]: field mismatch 106 │ │ y: ChangeFieldA, 107 │ │ z: ChangeFieldB, // changed to ChangeFieldB 108 │ │ }, - │ ╰─────────^ Mismatched field 'z: 0x0::upgrades::ChangeFieldA', expected '0x0::upgrades::ChangeFieldB'. + │ ╰─────────^ Mismatched field type '0x0::upgrades::ChangeFieldB', expected '0x0::upgrades::ChangeFieldA'. │ = Enums are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original enum's variant for enum 'EnumWithNamedChanged' including the ordering. diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__package_no_name.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__package_no_name.snap index 03cd875ac4992..000b3a5a550ff 100644 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__package_no_name.snap +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__package_no_name.snap @@ -2,7 +2,7 @@ source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs expression: output --- -error[Compatibility E01007]: module missing +error[Compatibility E01006]: module missing ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/missing_module_toml/package_no_name/Move.toml:1:1 │ 1 │ [package] diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__starts_second_line.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__starts_second_line.snap index 9984efcd08db3..0a8fbb124048b 100644 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__starts_second_line.snap +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__starts_second_line.snap @@ -2,7 +2,7 @@ source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs expression: output --- -error[Compatibility E01007]: module missing +error[Compatibility E01006]: module missing ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/missing_module_toml/starts_second_line/Move.toml:2:1 │ 2 │ ╭ [package] 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 dabb1ac7c25e5..9dd94bfd7df8d 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 @@ -90,7 +90,7 @@ error[Compatibility E01004]: field mismatch │ --------------- Struct definition 41 │ a: u64, 42 │ c: u64, // changed from b to c - │ ^ Mismatched field name 'c', expected 'b'. + │ ^ Mismatched field 'c', expected 'b'. │ = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original struct's fields for struct 'ChangeFieldName' including the ordering. @@ -170,7 +170,7 @@ 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. + │ ----------------- ^ Mismatched field 'a', expected a positional field. │ │ │ Struct definition │ @@ -210,5 +210,115 @@ error[Compatibility E01002]: type mismatch = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. = Restore the original struct's field for struct 'ChangeNameNestedStruct' including the ordering. +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:95:33 + │ +95 │ public struct NamedBox { x: u32 } + │ -------- ^ Mismatched field type 'u32', expected 'A'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. + = Restore the original struct's field for struct 'NamedBox' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:96:39 + │ +96 │ public struct NamedTwoBox { x: u32, y: u32 } + │ ----------- ^ Mismatched field type 'u32', expected 'B'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. + = Restore the original struct's fields for struct 'NamedTwoBox' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:96:47 + │ +96 │ public struct NamedTwoBox { x: u32, y: u32 } + │ ----------- ^ Mismatched field type 'u32', expected 'C'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. + = Restore the original struct's fields for struct 'NamedTwoBox' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:98:38 + │ +98 │ public struct NamedBoxInBox { x: u32 } + │ ------------- ^ Mismatched field type 'u32', expected '0x0::upgrades::NamedBox<0x0::upgrades::NamedBox>'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. + = Restore the original struct's field for struct 'NamedBoxInBox' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:99:44 + │ +99 │ public struct NamedBoxInTwoBox { x: u32 } + │ ---------------- ^ Mismatched field type 'u32', expected '0x0::upgrades::NamedTwoBox<0x0::upgrades::NamedBox, 0x0::upgrades::NamedBox>'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. + = Restore the original struct's field for struct 'NamedBoxInTwoBox' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:101:36 + │ +101 │ public struct PositionalBox(u32) + │ ------------- ^^^ Mismatched field type 'u32', expected 'G'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. + = Restore the original struct's field for struct 'PositionalBox' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:102:42 + │ +102 │ public struct PositionalTwoBox(u32, u32) + │ ---------------- ^^^ Mismatched field type 'u32', expected 'H'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. + = Restore the original struct's fields for struct 'PositionalTwoBox' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:102:47 + │ +102 │ public struct PositionalTwoBox(u32, u32) + │ ---------------- ^^^ Mismatched field type 'u32', expected 'I'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. + = Restore the original struct's fields for struct 'PositionalTwoBox' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:104:41 + │ +104 │ public struct PositionalBoxInBox(u32) + │ ------------------ ^^^ Mismatched field type 'u32', expected '0x0::upgrades::PositionalBox<0x0::upgrades::PositionalBox>'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. + = Restore the original struct's field for struct 'PositionalBoxInBox' including the ordering. + +error[Compatibility E01002]: type mismatch + ┌─ /fixtures/upgrade_errors/struct_errors_v2/sources/UpgradeErrors.move:105:47 + │ +105 │ public struct PositionalBoxInTwoBox(u32) + │ --------------------- ^^^ Mismatched field type 'u32', expected '0x0::upgrades::PositionalTwoBox<0x0::upgrades::PositionalBox, 0x0::upgrades::PositionalBox>'. + │ │ + │ Struct definition + │ + = Structs are part of a module's public interface and cannot be removed or changed during an upgrade. + = Restore the original struct's field for struct 'PositionalBoxInTwoBox' 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__version_mismatch.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__version_mismatch.snap index ad31f8daf1b4f..0f1d6f9ceff1c 100644 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__version_mismatch.snap +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__version_mismatch.snap @@ -2,7 +2,7 @@ source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs expression: normalize_path(result.unwrap_err().to_string()) --- -error[Compatibility E01009]: file format version downgrade +error[Compatibility E01008]: file format version downgrade ┌─ /fixtures/upgrade_errors/deponly_errors_v2/sources/UpgradeErrors.move:4:18 │ 4 │ module upgrades::upgrades { diff --git a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__whitespace.snap b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__whitespace.snap index b76c5aa7047b6..933bf7e129589 100644 --- a/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__whitespace.snap +++ b/crates/sui/src/unit_tests/snapshots/sui__upgrade_compatibility__upgrade_compatibility_tests__whitespace.snap @@ -2,7 +2,7 @@ source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs expression: output --- -error[Compatibility E01007]: module missing +error[Compatibility E01006]: module missing ┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/missing_module_toml/whitespace/Move.toml:1:1 │ 1 │ ╭ diff --git a/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs b/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs index 4e79efa4b50bd..74c9f22b1d4b8 100644 --- a/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs +++ b/crates/sui/src/unit_tests/upgrade_compatibility_tests.rs @@ -1,17 +1,19 @@ // Copyright (c) Mysten Labs, Inc. // SPDX-License-Identifier: Apache-2.0 - -use crate::upgrade_compatibility::{compare_packages, missing_module_diag}; use insta::assert_snapshot; +use std::fs; +use std::path::PathBuf; +use std::str::FromStr; +use std::sync::Arc; + +use crate::upgrade_compatibility::{compare_packages, missing_module_diag, FormattedField}; + +use move_binary_format::normalized::{Field, Type}; use move_binary_format::CompiledModule; use move_command_line_common::files::FileHash; use move_compiler::diagnostics::report_diagnostics_to_buffer; use move_compiler::shared::files::{FileName, FilesSourceText}; use move_core_types::identifier::Identifier; -use std::fs; -use std::path::PathBuf; -use std::str::FromStr; -use std::sync::Arc; use sui_move_build::BuildConfig; use sui_move_build::CompiledPackage; use sui_types::move_package::UpgradePolicy; @@ -167,6 +169,18 @@ fn test_missing_module_toml() { } } +#[test] +fn positional_formatting() { + let name = Identifier::new("pos999").unwrap(); + let field = Field { + name, + type_: Type::Bool, + }; + + let ff = FormattedField::new(&field, &[]); + assert_eq!(format!("{}", ff), "'bool' at position 999"); +} + fn get_packages(name: &str) -> (Vec, CompiledPackage, PathBuf) { let mut path: PathBuf = PathBuf::from(env!("CARGO_MANIFEST_DIR")); path.push("src/unit_tests/fixtures/upgrade_errors/"); diff --git a/crates/sui/src/upgrade_compatibility/formatting.rs b/crates/sui/src/upgrade_compatibility/formatting.rs new file mode 100644 index 0000000000000..de7eb8f0a8599 --- /dev/null +++ b/crates/sui/src/upgrade_compatibility/formatting.rs @@ -0,0 +1,168 @@ +// Copyright (c) Mysten Labs, Inc. +// SPDX-License-Identifier: Apache-2.0 + +use anyhow::{Context, Error}; +use move_binary_format::normalized::{Field, Type}; +use move_bytecode_source_map::source_map::SourceName; +use move_core_types::identifier::Identifier; +use move_ir_types::location::Loc; +use regex::Regex; +use std::fmt; +use std::sync::LazyLock; + +pub(super) struct FormattedType<'f> { + type_: &'f Type, + type_params: &'f [SourceName], +} + +impl<'f> fmt::Display for FormattedType<'f> { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + if f.alternate() { + write!( + f, + "{}", + format_param(self.type_, self.type_params.to_vec(), &mut Vec::new()) + .map_err(|_| fmt::Error)?, + ) + } else { + write!( + f, + "'{}'", + format_param(self.type_, self.type_params.to_vec(), &mut Vec::new()) + .map_err(|_| fmt::Error)?, + ) + } + } +} + +pub(super) enum FormattedIdentifier<'f> { + Positional(usize), + Named(&'f Identifier), +} + +pub(super) struct FormattedField<'f> { + pub(super) identifier: FormattedIdentifier<'f>, + pub(super) type_: FormattedType<'f>, +} + +impl<'f> FormattedField<'f> { + pub(super) fn new(f: &'f Field, type_params: &'f [SourceName]) -> Self { + static RE_POS: LazyLock = LazyLock::new(|| Regex::new(r"^pos(\d+)$").unwrap()); + let identifier = if let Some(ix) = RE_POS + .captures(f.name.as_str()) + .and_then(|c| c.get(1)) + .and_then(|m| m.as_str().parse::().ok()) + { + FormattedIdentifier::Positional(ix) + } else { + FormattedIdentifier::Named(&f.name) + }; + + FormattedField { + identifier, + type_: FormattedType { + type_: &f.type_, + type_params, + }, + } + } +} + +impl<'f> fmt::Display for FormattedField<'f> { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + use FormattedIdentifier as FI; + match self.identifier { + FI::Positional(_) if f.alternate() => write!(f, "a positional field"), + FI::Named(name) if f.alternate() => write!(f, "'{name}'"), + + FI::Positional(ix) => write!(f, "'{:#}' at position {ix}", self.type_), + FI::Named(name) => write!(f, "'{name}: {:#}'", self.type_), + } + } +} + +/// Returns a string representation of a parameter and updates its secondary label to include its location. +pub(super) fn format_param( + param: &Type, + type_params: Vec, + secondary: &mut Vec<(Loc, String)>, +) -> Result { + 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)?) + } + Type::Struct { + address, + module, + name, + type_arguments, + .. + } if type_arguments.len() > 0 => { + format!( + "{}::{}::{}<{}>", + address.to_hex_literal(), + module, + name, + type_arguments + .iter() + .map(|t| format_param(t, type_params.clone(), secondary)) + .collect::, _>>()? + .join(", ") + ) + } + _ => format!("{}", param), + }) +} + +/// Format a list of items into a human-readable string. +pub(super) fn format_list( + items: impl IntoIterator, + noun_singular_plural: Option<(&str, &str)>, +) -> String { + let items: Vec<_> = items.into_iter().map(|i| i.to_string()).collect(); + let items_string = match items.len() { + 0 => "none".to_string(), + 1 => items[0].to_string(), + 2 => format!("{} and {}", items[0], items[1]), + _ => { + let all_but_last = &items[..items.len() - 1].join(", "); + let last = items.last().expect("unexpected empty list"); + format!("{}, and {}", all_but_last, last) + } + }; + if let Some((singular, plural)) = noun_singular_plural { + format!( + "{}: {}", + singular_or_plural(items.len(), singular, plural), + items_string, + ) + } else { + items_string + } +} + +/// Returns a string with the singular or plural form of a word based on a count. +pub(super) fn singular_or_plural(n: usize, singular: &str, plural: &str) -> String { + if n == 1 { + singular.to_string() + } else { + plural.to_string() + } +} diff --git a/crates/sui/src/upgrade_compatibility.rs b/crates/sui/src/upgrade_compatibility/mod.rs similarity index 87% rename from crates/sui/src/upgrade_compatibility.rs rename to crates/sui/src/upgrade_compatibility/mod.rs index 63a534e45f7da..f3a09e490c52a 100644 --- a/crates/sui/src/upgrade_compatibility.rs +++ b/crates/sui/src/upgrade_compatibility/mod.rs @@ -1,12 +1,14 @@ // Copyright (c) Mysten Labs, Inc. // SPDX-License-Identifier: Apache-2.0 -#[path = "unit_tests/upgrade_compatibility_tests.rs"] +mod formatting; +#[path = "../unit_tests/upgrade_compatibility_tests.rs"] #[cfg(test)] mod upgrade_compatibility_tests; +use formatting::{format_list, format_param, singular_or_plural, FormattedField}; + use anyhow::{anyhow, Context, Error}; -use regex::Regex; use std::collections::{BTreeMap, HashMap, HashSet}; use std::fs; use std::path::PathBuf; @@ -47,20 +49,13 @@ use sui_sdk::SuiClient; use sui_types::move_package::UpgradePolicy; use sui_types::{base_types::ObjectID, execution_config_utils::to_binary_config}; -/// Errors that can occur during upgrade compatibility checks. -/// one-to-one related to the underlying trait functions see: [`CompatibilityMode`] -/// Except for the `ModuleMismatch` which is a special case for additive and dependency only policies and `ModuleMissing` +/// Errors that can occur during upgrade compatibility checks, +/// one-to-one related to the underlying trait functions see: [`CompatibilityMode`]. #[derive(Debug, Clone)] pub(crate) enum UpgradeCompatibilityModeError { ModuleMissing { name: Identifier, }, - /// The upgrade is not compatible with the existing package due to the policy. - /// This error is used for additive and dependency only policies where modules - /// are either not allowed to add declarations or change them. - ModuleMismatch { - policy: UpgradePolicy, - }, StructMissing { name: Identifier, }, @@ -164,14 +159,13 @@ pub(crate) enum UpgradeCompatibilityModeError { FriendMissing, } -/// Check if a specifc error breaks 'compatible' upgrades +/// Check if an `UpgradeCompatibilityModeError` variant breaks 'compatible' upgrades. fn breaks_compatibility( error: &UpgradeCompatibilityModeError, compatability: &Compatibility, ) -> bool { match error { - UpgradeCompatibilityModeError::ModuleMissing { .. } - | UpgradeCompatibilityModeError::ModuleMismatch { .. } => true, + UpgradeCompatibilityModeError::ModuleMissing { .. } => true, UpgradeCompatibilityModeError::StructAbilityMismatch { .. } | UpgradeCompatibilityModeError::StructTypeParamMismatch { .. } @@ -219,7 +213,8 @@ fn breaks_compatibility( } } -/// Check if a specifc error breaks inclusion checks for 'additive' (Subset) or 'dependency only' (Equal) upgrades +/// Check if an `UpgradeCompatibilityModeError` variant breaks inclusion checks for 'additive' (Subset) +/// or 'dependency only' (Equal) upgrades. fn breaks_inclusion_check( error: &UpgradeCompatibilityModeError, inclusion_check: &InclusionCheck, @@ -236,7 +231,6 @@ fn breaks_inclusion_check( } UpgradeCompatibilityModeError::ModuleMissing { .. } - | UpgradeCompatibilityModeError::ModuleMismatch { .. } | UpgradeCompatibilityModeError::StructMissing { .. } | UpgradeCompatibilityModeError::StructAbilityMismatch { .. } | UpgradeCompatibilityModeError::StructTypeParamMismatch { .. } @@ -262,7 +256,7 @@ fn breaks_inclusion_check( } } -/// A compatibility mode that collects errors as a vector of enums which describe the error causes +/// Compatibility mode used for 'compatible' upgrades, collects all errors and returns them as a single error. #[derive(Default)] pub(crate) struct CliCompatibilityMode { errors: Vec, @@ -270,7 +264,7 @@ pub(crate) struct CliCompatibilityMode { impl CompatibilityMode for CliCompatibilityMode { type Error = Vec; - // ignored, address is not populated pre-tx + // Ignored, address is not populated pre-tx, `compare_packages` function prevents name mismatches fn module_id_mismatch( &mut self, _old_addr: &AccountAddress, @@ -440,7 +434,8 @@ impl CompatibilityMode for CliCompatibilityMode { } } -/// A Compatibility mode used for checking inclusion checks for 'additive' (Subset) or 'dependency only' (Equal) upgrades +/// Compatibility mode used during inclusion checks: 'additive' (Subset) or 'dependency only' (Equal) upgrades, +/// collects all errors and returns them as a single error. #[derive(Default)] struct CliInclusionCheckMode { errors: Vec, @@ -449,13 +444,13 @@ struct CliInclusionCheckMode { impl InclusionCheckMode for CliInclusionCheckMode { type Error = Vec; - // ignored address is not populated pre-tx + // Ignored, address is not populated pre-tx, `compare_packages` function prevents name mismatches fn module_id_mismatch( &mut self, - old_address: &AccountAddress, - old_name: &IdentStr, - new_address: &AccountAddress, - new_name: &IdentStr, + _old_address: &AccountAddress, + _old_name: &IdentStr, + _new_address: &AccountAddress, + _new_name: &IdentStr, ) { } @@ -483,7 +478,7 @@ impl InclusionCheckMode for CliInclusionCheckMode { }); } - fn struct_missing(&mut self, name: &Identifier, old_struct: &Struct) { + fn struct_missing(&mut self, name: &Identifier, _old_struct: &Struct) { self.errors .push(UpgradeCompatibilityModeError::StructMissing { name: name.clone() }); } @@ -502,7 +497,7 @@ impl InclusionCheckMode for CliInclusionCheckMode { }); } - fn enum_missing(&mut self, name: &Identifier, old_enum: &Enum) { + fn enum_missing(&mut self, name: &Identifier, _old_enum: &Enum) { self.errors .push(UpgradeCompatibilityModeError::EnumMissing { name: name.clone() }); } @@ -524,7 +519,7 @@ impl InclusionCheckMode for CliInclusionCheckMode { }); } - fn function_missing(&mut self, name: &Identifier, old_func: &Function) { + fn function_missing(&mut self, name: &Identifier, _old_func: &Function) { self.errors .push(UpgradeCompatibilityModeError::FunctionMissing { name: name.clone() }); } @@ -559,7 +554,7 @@ struct IdentifierTableLookup { function_identifier_to_index: BTreeMap, } -/// creates an index to allow looking up the table index of a struct, enum, or function by its identifier +/// Creates an index to allow looking up the table index of a struct, enum, or function by its identifier. fn table_index(compiled_module: &CompiledModule) -> IdentifierTableLookup { // for each in compiled module let struct_identifier_to_index: BTreeMap = compiled_module @@ -658,7 +653,6 @@ upgrade_codes!( AbilityMismatch: { msg: "ability mismatch" }, FieldMismatch: { msg: "field mismatch" }, TypeParamMismatch: { msg: "type parameter mismatch" }, - ModuleMismatch: { msg: "module incompatible" }, ModuleMissing: { msg: "module missing" }, Missing: { msg: "missing declaration" }, VersionMismatch: { msg: "file format version downgrade" }, @@ -840,24 +834,13 @@ fn modules_into_diags( Ok(diags) } -fn errors_or_empty_vec( - name: Identifier, - result: Result<(), Vec>, -) -> Vec<(Identifier, UpgradeCompatibilityModeError)> { - match result { - Ok(_) => vec![], - Err(errors) => errors.into_iter().map(|e| (name.clone(), e)).collect(), - } -} - -/// Convert an error to a diagnostic using the specific error type's function. +/// Convert an error to a vector of diagnostics using the error specific function. fn compatibility_diag_from_error( error: &UpgradeCompatibilityModeError, compiled_unit_with_source: &CompiledUnitWithSource, is_compatible: bool, lookup: &IdentifierTableLookup, ) -> Result { - let module_name = compiled_unit_with_source.unit.name.as_str(); match error { UpgradeCompatibilityModeError::StructMissing { name, .. } => { missing_definition_diag("struct", name, is_compatible, compiled_unit_with_source) @@ -993,11 +976,6 @@ fn compatibility_diag_from_error( UpgradeCompatibilityModeError::FunctionEntryCompatibility { name, old_function, .. } => function_entry_mismatch(name, old_function, compiled_unit_with_source, lookup), - // Specifically handles additive and dep only policies where modules - // are either not allowed to add declarations or change them. - UpgradeCompatibilityModeError::ModuleMismatch { policy } => { - module_compatibility_error_diag(*policy, compiled_unit_with_source) - } UpgradeCompatibilityModeError::ModuleMissing { .. } => { unreachable!("Module Missing should be handled by outer function") } @@ -1048,45 +1026,7 @@ fn compatibility_diag_from_error( } } -// give specifics about the declarations which do not match -fn module_compatibility_error_diag( - policy: UpgradePolicy, - compiled_unit_with_source: &CompiledUnitWithSource, -) -> Result { - let mut diags = Diagnostics::new(); - - let loc = compiled_unit_with_source - .unit - .source_map - .definition_location; - - diags.add(Diagnostic::new( - Declarations::ModuleMismatch, - ( - loc, - format!( - "The upgrade is not compatible with the existing package due to {} policy.", - match policy { - UpgradePolicy::Additive => "additive", - UpgradePolicy::DepOnly => "dependency only", - _ => unreachable!("Invalid upgrade policy for this error type"), - } - ), - ), - Vec::<(Loc, String)>::new(), - vec![ - "The upgrade is not compatible with the existing package.".to_string(), - format!( - "The upgrade policy is set to '{}'.", - policy.to_string().to_lowercase() - ), - ], - )); - - Ok(diags) -} - -/// Return a diagnostic when a module is missing from a package. +/// Returns a diagnostic when a module is missing from a package. fn missing_module_diag( module_name: &Identifier, move_toml_hash: &FileHash, @@ -1120,7 +1060,7 @@ fn missing_module_diag( Ok(diags) } -/// Return a diagnostic for a missing definition. +/// Returns a diagnostic for a missing definition. fn missing_definition_diag( declaration_kind: &str, identifier_name: &Identifier, @@ -1182,7 +1122,7 @@ fn missing_definition_diag( Ok(diags) } -/// Return a diagnostic for a function which has lost its public visibility +/// Returns a diagnostic for a function which has lost its public visibility. fn function_lost_public( function_name: &Identifier, compiled_unit_with_source: &CompiledUnitWithSource, @@ -1224,39 +1164,9 @@ fn function_lost_public( Ok(diags) } -fn format_param( - param: &Type, - type_params: Vec, - secondary: &mut Vec<(Loc, String)>, -) -> Result { - 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), - }) -} - -/// 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. +/// Returns diagnostics for each signature mismatch in the given function. +/// Start by checking the lengths of the parameters and returns and add a diagnostic if they are different. +/// If the lengths are the same check each parameter piece wise and add a diagnostic for each mismatch. fn function_signature_mismatch_diag( function_name: &Identifier, old_function: &Function, @@ -1571,7 +1481,7 @@ fn function_entry_mismatch( Ok(diags) } -/// Return a label string for an ability mismatch. +/// Returns a label string for an ability mismatch. fn ability_mismatch_label( old_abilities: AbilitySet, new_abilities: AbilitySet, @@ -1613,7 +1523,7 @@ fn ability_mismatch_label( } } -/// Return a diagnostic for an ability mismatch. +/// Returns a diagnostic for a given struct's ability mismatch. fn struct_ability_mismatch_diag( struct_name: &Identifier, old_struct: &Struct, @@ -1682,63 +1592,43 @@ 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 +/// Returns an error code and label for the given field. +fn field_mismatch_message( + old_field: &Field, + new_field: &Field, + type_params: Vec, +) -> Result<(Declarations, String), Error> { + let old_ff = FormattedField::new(old_field, &type_params); + let new_ff = FormattedField::new(new_field, &type_params); + + Ok( + match ( + old_field.name != new_field.name, + old_field.type_ != new_field.type_, + ) { + (true, true) => ( + Declarations::FieldMismatch, + format!("Mismatched field {new_ff}, expected {old_ff}."), ), - ), - (true, false) => ( - Declarations::FieldMismatch, - format!( - "Mismatched field name {}, expected {}.", - new_field_name, old_field_name + (true, false) => ( + Declarations::FieldMismatch, + format!("Mismatched field {new_ff:#}, expected {old_ff:#}."), ), - ), - (false, true) => ( - Declarations::TypeMismatch, - format!( - "Mismatched field type {}, expected {}.", - new_field_type, old_field_type + (false, true) => ( + Declarations::TypeMismatch, + format!( + "Mismatched field type {}, expected {}.", + new_ff.type_, old_ff.type_ + ), ), - ), - (false, false) => unreachable!("Fields should not be the same"), - } + (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 +/// Returns diagnostics for each field mismatch in the given struct. +/// 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, @@ -1762,16 +1652,21 @@ fn struct_field_mismatch_diag( let def_loc = struct_sourcemap.definition_location; + let dummy_field = Field { + name: Identifier::new("dummy_field") + .context("unexpected error with identifier constructor")?, + type_: Type::Bool, + }; let old_fields: Vec<&Field> = old_struct .fields .iter() - .filter(|f| f.name != Identifier::new("dummy_field").unwrap() && f.type_ != Type::Bool) + .filter(|f| f != &&dummy_field) .collect(); let new_fields: Vec<&Field> = new_struct .fields .iter() - .filter(|f| f.name != Identifier::new("dummy_field").unwrap() && f.type_ != Type::Bool) + .filter(|f| f != &&dummy_field) .collect(); let reason = if public_visibility_related_error { @@ -1811,7 +1706,11 @@ fn struct_field_mismatch_diag( .get(i) .context("Unable to get field location")?; - let (code, label) = field_mismatch_message(old_field, new_field); + let (code, label) = field_mismatch_message( + old_field, + new_field, + struct_sourcemap.type_parameters.clone(), + )?; diags.add(Diagnostic::new( code, @@ -1833,9 +1732,9 @@ fn struct_field_mismatch_diag( Ok(diags) } -/// 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 +/// Returns diagnostics for each type parameter mismatch in the given struct. +/// 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. fn struct_type_param_mismatch_diag( name: &Identifier, old_struct: &Struct, @@ -1868,7 +1767,7 @@ fn struct_type_param_mismatch_diag( ) } -/// Return a diagnostic for a new variant in an enum +/// Returns a diagnostic for enum ability mismatches. fn enum_ability_mismatch_diag( enum_name: &Identifier, old_enum: &Enum, @@ -1940,69 +1839,64 @@ fn enum_ability_mismatch_diag( Ok(diags) } -/// Returns the error code and label for mismatched, missing, or unexpected variant -fn enum_variant_field_error( +/// Returns the error code and label for mismatched, missing, or unexpected variants. +fn enum_variant_field_message( old_variant: &Variant, new_variant: &Variant, - variant_loc: Loc, - def_loc: Loc, -) -> (DiagnosticInfo, Vec) { +) -> Result, Error> { if old_variant.fields.len() != new_variant.fields.len() { - return ( + return Ok(vec![( Declarations::FieldMismatch.into(), - vec![format!( + 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 + Ok( + match ( + old_variant.name != new_variant.name, + old_variant.fields != new_variant.fields, + ) { + (true, true) => vec![( + Enums::VariantMismatch.into(), + 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 + (true, false) => vec![( + Enums::VariantMismatch.into(), + format!( + "Mismatched variant '{}', 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 - )); + (false, true) => { + let mut errors: Vec<(DiagnosticInfo, String)> = vec![]; + + for (old_field, new_field) in + old_variant.fields.iter().zip(new_variant.fields.iter()) + { + if old_field != new_field { + let (code, label) = + field_mismatch_message(old_field, new_field, Vec::new())?; + errors.push((code.into(), label)); + } } - } - (Declarations::FieldMismatch.into(), errors) - } - (false, false) => unreachable!("Variants should not be the same"), - } + 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 +/// Returns diagnostics for each variant mismatch in the given enum. +/// Start by checking the lengths of the variants 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. fn enum_variant_mismatch_diag( enum_name: &Identifier, old_enum: &Enum, @@ -2040,12 +1934,11 @@ fn enum_variant_mismatch_diag( .0 .1; - let (code, labels) = - enum_variant_field_error(old_variant, new_variant, variant_loc, def_loc); + let messages = enum_variant_field_message(old_variant, new_variant)?; - for label in labels { + for (code, label) in messages { diags.add(Diagnostic::new( - code.clone(), + code, (variant_loc, label), vec![(def_loc, "Enum definition".to_string())], vec![ @@ -2072,7 +1965,7 @@ fn enum_variant_mismatch_diag( Ok(diags) } -/// Return a diagnostic for a new variant in an enum +/// Returns diagnostics for each new variant in an enum. fn enum_new_variant_diag( enum_name: &Identifier, old_enum: &Enum, @@ -2134,7 +2027,7 @@ fn enum_new_variant_diag( Ok(diags) } -/// Return a diagnostic for a missing variant in an enum +/// Returns diagnostics for each missing variant in an enum. fn enum_variant_missing_diag( enum_name: &Identifier, old_enum: &Enum, @@ -2182,6 +2075,7 @@ fn enum_variant_missing_diag( Ok(diags) } +/// Returns a diagnostic for an unexpected struct. fn struct_new_diag( struct_name: &Identifier, compiled_unit_with_source: &CompiledUnitWithSource, @@ -2211,6 +2105,7 @@ fn struct_new_diag( Ok(diags) } +/// Returns a diagnostic for an unexpected struct changed. fn struct_changed_diag( struct_name: &Identifier, old_struct: &Struct, @@ -2256,6 +2151,7 @@ fn struct_changed_diag( Ok(diags) } +/// Returns a diagnostic for an unexpected new enum. fn enum_new_diag( enum_name: &Identifier, _new_enum: &Enum, @@ -2286,6 +2182,7 @@ fn enum_new_diag( Ok(diags) } +/// Returns a diagnostic for an unexpected enum change. fn enum_changed_diag( enum_name: &Identifier, old_enum: &Enum, @@ -2331,6 +2228,7 @@ fn enum_changed_diag( Ok(diags) } +/// Returns a diagnostic for an unexpected new function. fn function_new_diag( function_name: &Identifier, compiled_unit_with_source: &CompiledUnitWithSource, @@ -2363,6 +2261,7 @@ fn function_new_diag( Ok(diags) } +/// Returns a diagnostic for an unexpected function changed. fn function_changed_diag( function_name: &Identifier, old_function: &Function, @@ -2386,7 +2285,7 @@ fn function_changed_diag( Ok(diags) } -/// Return a diagnostic for a type parameter mismatch +/// Returns a diagnostic for an enum type parameter mismatch. fn enum_type_param_mismatch( enum_name: &Identifier, old_enum: &Enum, @@ -2419,7 +2318,7 @@ fn enum_type_param_mismatch( ) } -/// Return a diagnostic for a type parameter mismatch +/// Returns a diagnostic for a type parameter mismatch. fn type_parameter_diag( declaration_kind: &str, name: &Identifier, @@ -2437,7 +2336,7 @@ fn type_parameter_diag( .enumerate() .map(|(i, c)| { if i == 0 { - c.to_uppercase().next().unwrap() + c.to_uppercase().next().unwrap_or(c) } else { c } @@ -2523,7 +2422,7 @@ fn type_parameter_diag( Ok(diags) } -/// Return a diagnostic for a type parameter constrant mismatch +/// Returns a label for a type parameter constraint mismatch. fn type_param_constraint_labels( old_constraints: AbilitySet, new_constraints: AbilitySet, @@ -2551,7 +2450,7 @@ fn type_param_constraint_labels( )) } -/// Return a diagnostic for a type parameter phantom mismatch +/// Returns a label 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; @@ -2570,7 +2469,7 @@ fn type_param_phantom_labels(old_phantom: bool, new_phantom: bool) -> Option<(St }) } -/// Return a diagnostic for package file format version mismatch +/// Returns a diagnostic for package file format version mismatch. fn file_format_version_downgrade_diag( old_version: &u32, new_version: &u32, @@ -2602,7 +2501,7 @@ fn file_format_version_downgrade_diag( Ok(diags) } -/// Return a diagnostic for a friend link mismatch +/// Returns a diagnostic for a friend link mismatch. fn friend_link_diag( compiled_unit_with_source: &CompiledUnitWithSource, ) -> Result { @@ -2623,44 +2522,8 @@ fn friend_link_diag( Ok(diags) } -/// Format a list of items into a human-readable string. -fn format_list( - items: impl IntoIterator, - noun_singular_plural: Option<(&str, &str)>, -) -> String { - let items: Vec<_> = items.into_iter().map(|i| i.to_string()).collect(); - let items_string = match items.len() { - 0 => "none".to_string(), - 1 => items[0].to_string(), - 2 => format!("{} and {}", items[0], items[1]), - _ => { - let all_but_last = &items[..items.len() - 1].join(", "); - let last = items.last().unwrap(); - format!("{}, and {}", all_but_last, last) - } - }; - if let Some((singular, plural)) = noun_singular_plural { - format!( - "{}: {}", - singular_or_plural(items.len(), singular, plural), - items_string, - ) - } else { - items_string - } -} - -/// 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() - } else { - plural.to_string() - } -} - -/// Helper function to determine if colors should be used in the output. -/// disables colors in tests +/// Helper function to determine if colors should be used in the output and +/// disables colors in tests. fn use_colors() -> bool { #[cfg(test)] {