Skip to content

Commit

Permalink
Format missing module errors in upgrade errors (#20541)
Browse files Browse the repository at this point in the history
## Description 

Missing modules errors do not have a file to point to since their file
has been removed during upgrade or otherwise unknown since the
declaration is gone from its previous file if the file still exists.
Align missing modules errors with the behavior of other errors by using
Move.toml as the reference point for these errors and pointing to the
"package" instead.

## Test plan 

snapshots

---

## 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): 
- [ ] Indexer: 
- [ ] JSON-RPC: 
- [ ] GraphQL: 
- [ ] CLI: 
- [ ] Rust SDK:
- [ ] REST API:
  • Loading branch information
jordanjennings-mysten authored Dec 21, 2024
1 parent a8b3ad3 commit f321519
Show file tree
Hide file tree
Showing 15 changed files with 245 additions and 57 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
[addresses]
upgrades = "0x0"

[package]
name = "upgrades"
edition = "2024.beta" # edition = "legacy" to use legacy (pre-2024) Move
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
# Emojis are 4 bytes long, this test ensures proper parsing of UTF-8 bytes and check for improper mixing of byte and character indexes.
😀[package]😀
😀
😀name = "emoji"😀
😀
Empty file.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[package]
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@

[package]
name = "upgrades"
edition = "2024.beta" # edition = "legacy" to use legacy (pre-2024) Move

[addresses]
upgrades = "0x0"
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@


Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs
expression: output
---
error[Compatibility E01007]: 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]
5 │ │ name = "upgrades"
│ ╰─────────────────^ Package is missing module 'identifier'
= Modules which are part package cannot be removed during an upgrade.
= add missing module 'identifier' back to the package.
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,16 @@ error[Compatibility E01001]: missing public declaration
= structs are part of a module's public interface and cannot be removed or changed during an upgrade.
= add missing struct 'StructToBeRemoved' back to the module 'struct_'.

The following modules are missing from the new package: 'missing_module'
error[Compatibility E01007]: module missing
┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/declaration_errors_v2/Move.toml:1:1
1 │ ╭ [package]
2 │ │ name = "upgrades"
3 │ │ edition = "2024.beta" # edition = "legacy" to use legacy (pre-2024) Move
│ ╰────────────────────────────────────────────────────────────────────────^ Package is missing module 'missing_module'
= Modules which are part package cannot be removed during an upgrade.
= add missing module 'missing_module' back to the package.


Upgrade failed, this package requires changes to be compatible with the existing package. Its upgrade policy is set to 'compatible'.
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
---
source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs
expression: output
---
error[Compatibility E01007]: module missing
┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/missing_module_toml/emoji/Move.toml:2:2
2 │ 😀[package]😀
│ ╭───^
3 │ │ 😀
4 │ │ 😀name = "emoji"😀
│ ╰──────────────────^ Package is missing module 'identifier'
= Modules which are part package cannot be removed during an upgrade.
= add missing module 'identifier' back to the package.
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs
expression: output
---
error[Compatibility E01007]: module missing
┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/missing_module_toml/empty/Move.toml:1:1
1
^ Package is missing module 'identifier'
= Modules which are part package cannot be removed during an upgrade.
= add missing module 'identifier' back to the package.
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs
expression: output
---
error[Compatibility E01007]: 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]
^^^^^^^^^ Package is missing module 'identifier'
= Modules which are part package cannot be removed during an upgrade.
= add missing module 'identifier' back to the package.
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs
expression: output
---
error[Compatibility E01007]: 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]
3 │ │ name = "upgrades"
4 │ │ edition = "2024.beta" # edition = "legacy" to use legacy (pre-2024) Move
│ ╰────────────────────────────────────────────────────────────────────────^ Package is missing module 'identifier'
= Modules which are part package cannot be removed during an upgrade.
= add missing module 'identifier' back to the package.
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
source: crates/sui/src/unit_tests/upgrade_compatibility_tests.rs
expression: output
---
error[Compatibility E01007]: module missing
┌─ /Users/jordanjennings/code/sui/crates/sui/src/unit_tests/fixtures/upgrade_errors/missing_module_toml/whitespace/Move.toml:1:1
1 │ ╭
2 │ │
│ ╰──^ Package is missing module 'identifier'
= Modules which are part package cannot be removed during an upgrade.
= add missing module 'identifier' back to the package.
90 changes: 71 additions & 19 deletions crates/sui/src/unit_tests/upgrade_compatibility_tests.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,25 @@
// Copyright (c) Mysten Labs, Inc.
// SPDX-License-Identifier: Apache-2.0

use crate::upgrade_compatibility::compare_packages;
use crate::upgrade_compatibility::{compare_packages, missing_module_diag};
use insta::assert_snapshot;
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;

#[test]
fn test_all() {
let (mods_v1, pkg_v2) = get_packages("all");
let result = compare_packages(mods_v1, pkg_v2, UpgradePolicy::Compatible);
let (mods_v1, pkg_v2, path) = get_packages("all");
let result = compare_packages(mods_v1, pkg_v2, path, UpgradePolicy::Compatible);

assert!(result.is_err());
let err = result.unwrap_err();
Expand All @@ -21,8 +28,8 @@ fn test_all() {

#[test]
fn test_declarations_missing() {
let (pkg_v1, pkg_v2) = get_packages("declaration_errors");
let result = compare_packages(pkg_v1, pkg_v2, UpgradePolicy::Compatible);
let (pkg_v1, pkg_v2, path) = get_packages("declaration_errors");
let result = compare_packages(pkg_v1, pkg_v2, path, UpgradePolicy::Compatible);

assert!(result.is_err());
let err = result.unwrap_err();
Expand All @@ -31,8 +38,8 @@ fn test_declarations_missing() {

#[test]
fn test_function() {
let (pkg_v1, pkg_v2) = get_packages("function_errors");
let result = compare_packages(pkg_v1, pkg_v2, UpgradePolicy::Compatible);
let (pkg_v1, pkg_v2, path) = get_packages("function_errors");
let result = compare_packages(pkg_v1, pkg_v2, path, UpgradePolicy::Compatible);

assert!(result.is_err());
let err = result.unwrap_err();
Expand All @@ -41,8 +48,8 @@ fn test_function() {

#[test]
fn test_struct() {
let (pkg_v1, pkg_v2) = get_packages("struct_errors");
let result = compare_packages(pkg_v1, pkg_v2, UpgradePolicy::Compatible);
let (pkg_v1, pkg_v2, path) = get_packages("struct_errors");
let result = compare_packages(pkg_v1, pkg_v2, path, UpgradePolicy::Compatible);

assert!(result.is_err());
let err = result.unwrap_err();
Expand All @@ -51,8 +58,8 @@ fn test_struct() {

#[test]
fn test_enum() {
let (pkg_v1, pkg_v2) = get_packages("enum_errors");
let result = compare_packages(pkg_v1, pkg_v2, UpgradePolicy::Compatible);
let (pkg_v1, pkg_v2, path) = get_packages("enum_errors");
let result = compare_packages(pkg_v1, pkg_v2, path, UpgradePolicy::Compatible);

assert!(result.is_err());
let err = result.unwrap_err();
Expand All @@ -61,8 +68,8 @@ fn test_enum() {

#[test]
fn test_type_param() {
let (pkg_v1, pkg_v2) = get_packages("type_param_errors");
let result = compare_packages(pkg_v1, pkg_v2, UpgradePolicy::Compatible);
let (pkg_v1, pkg_v2, path) = get_packages("type_param_errors");
let result = compare_packages(pkg_v1, pkg_v2, path, UpgradePolicy::Compatible);

assert!(result.is_err());
let err = result.unwrap_err();
Expand All @@ -71,19 +78,64 @@ fn test_type_param() {

#[test]
fn test_friend_link_ok() {
let (pkg_v1, pkg_v2) = get_packages("friend_linking");
let (pkg_v1, pkg_v2, path) = get_packages("friend_linking");
// upgrade compatibility ignores friend linking
assert!(compare_packages(pkg_v1, pkg_v2, UpgradePolicy::Compatible).is_ok());
assert!(compare_packages(pkg_v1, pkg_v2, path, UpgradePolicy::Compatible).is_ok());
}

#[test]
fn test_entry_linking_ok() {
let (pkg_v1, pkg_v2) = get_packages("entry_linking");
let (pkg_v1, pkg_v2, path) = get_packages("entry_linking");
// upgrade compatibility ignores entry linking
assert!(compare_packages(pkg_v1, pkg_v2, UpgradePolicy::Compatible).is_ok());
assert!(compare_packages(pkg_v1, pkg_v2, path, UpgradePolicy::Compatible).is_ok());
}

fn get_packages(name: &str) -> (Vec<CompiledModule>, CompiledPackage) {
#[test]
fn test_missing_module_toml() {
// note: the first examples empty and whitespace shouldn't occur in practice
// since a Move.toml which is empty will not build
for malformed_pkg in [
"emoji",
"addresses_first",
"starts_second_line",
"package_no_name",
"whitespace",
"empty",
] {
let move_pkg_path = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
.join("src/unit_tests/fixtures/upgrade_errors/missing_module_toml/")
.join(malformed_pkg);

let move_toml_contents: Arc<str> = fs::read_to_string(move_pkg_path.join("Move.toml"))
.unwrap_or_default()
.into();
let move_toml_hash = FileHash::new(&move_toml_contents);

let result = missing_module_diag(
&Identifier::from_str("identifier").unwrap(),
&move_toml_hash,
&move_toml_contents,
);

let move_toml: Arc<str> = fs::read_to_string(move_pkg_path.join("Move.toml"))
.unwrap_or_default()
.into();
let file_hash = FileHash::new(&move_toml);
let mut files = FilesSourceText::new();
let filename = FileName::from(move_pkg_path.join("Move.toml").to_string_lossy());
files.insert(file_hash, (filename, move_toml));

let output = String::from_utf8(report_diagnostics_to_buffer(
&files.into(),
result.unwrap(),
false,
))
.unwrap();
assert_snapshot!(malformed_pkg, output);
}
}

fn get_packages(name: &str) -> (Vec<CompiledModule>, CompiledPackage, PathBuf) {
let mut path: PathBuf = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
path.push("src/unit_tests/fixtures/upgrade_errors/");
path.push(format!("{}_v1", name));
Expand All @@ -99,7 +151,7 @@ fn get_packages(name: &str) -> (Vec<CompiledModule>, CompiledPackage) {

let pkg_v2 = BuildConfig::new_for_testing().build(&path).unwrap();

(mods_v1, pkg_v2)
(mods_v1, pkg_v2, path)
}

/// Snapshots will differ on each machine, normalize to prevent test failures
Expand Down
Loading

0 comments on commit f321519

Please sign in to comment.