[move-compiler] Added preliminary support for writing linters #12650

awelc · 2023-06-23T11:10:26Z

Description

Added support for writing linters:

integration with Sui CLI
a simple testing harness
an example linter to signal sharing of potentially owned objects

Test Plan

Additional testing framework being part of this PR can be used to test individual linters.

If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.

Type of Change (Check all that apply)

protocol change
user-visible impact
breaking change for a client SDKs
breaking change for FNs (FN binary must upgrade)
breaking change for validators or node operators (must upgrade binaries)
breaking change for on-chain data layout
necessitate either a data wipe or data migration

Release notes

Developers may now opt into running linters when building/testing/publishing/upgrading packages via CLI by specifying the --lint flag.

vercel · 2023-06-23T11:10:33Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

6 Ignored Deployments

Name	Status	Updated (UTC)
explorer	⬜️ Ignored (Inspect)	Jun 27, 2023 2:39pm
explorer-storybook	⬜️ Ignored (Inspect)	Jun 27, 2023 2:39pm
multisig-toolkit	⬜️ Ignored (Inspect)	Jun 27, 2023 2:39pm
sui-kiosk	⬜️ Ignored (Inspect)	Jun 27, 2023 2:39pm
sui-wallet-kit	⬜️ Ignored (Inspect)	Jun 27, 2023 2:39pm
wallet-adapter	⬜️ Ignored (Inspect)	Jun 27, 2023 2:39pm

tnowacki

Outstanding! Thanks!

Just a few minor things, and mostly looking for your thoughts/feedback on a few things

tnowacki · 2023-06-27T00:08:15Z

crates/sui-move-build/src/lib.rs

+        self.build_and_lint(path, false)
+    }
+
+    pub fn build_and_lint(self, path: PathBuf, lint: bool) -> SuiResult<CompiledPackage> {


Should lint here be a flag on self: BuildConfig?

Makes sense. Adding a function felt less-invasive but considering that in most cases BuildConfig is created with a single function, it does not matter much, and having a flag in BuildConfig does look cleaner. Thanks!

crates/sui-move-build/src/linters/share_owned.rs

tnowacki · 2023-06-27T00:18:51Z

crates/sui-move-build/src/linters/share_owned.rs

+            let msg = "You may be trying to share an owned object.";
+            let uid_msg = "This argument should in most cases come from an object created within the same function.";


I think this is fine? Maybe?
Just brainstorming:

Suggested change

let msg = "You may be trying to share an owned object.";

let uid_msg = "This argument should in most cases come from an object created within the same function.";

let msg = "Potential abort from sharing an object not created this transaction.";

let uid_msg = "Creating a fresh object in this same function will ensure this does not abort.";

I went for yet another variant combining (sort of) the two proposals.

tnowacki · 2023-06-27T00:19:13Z

crates/sui-move-build/src/linters/share_owned.rs

+                (f.arguments.exp.loc, uid_msg)
+            );
+            if let Value::NotFreshObj(l) = args[0] {
+                d.add_secondary_label((l, "A potentially shared object coming from here"))


Suggested change

d.add_secondary_label((l, "A potentially shared object coming from here"))

d.add_secondary_label((l, "A potentially owned object coming from here"))

Is this flipped?

tnowacki · 2023-06-27T00:20:07Z

crates/sui-move-build/src/linters/share_owned.rs

+        if let L::Unpack(_, _, fields) = l_ {
+            for (f, l) in fields {
+                let v = if is_obj(l) {
+                    <Self::State as SimpleDomain>::Value::NotFreshObj(f.loc())


Strange, does just Value::NotFreshObj error here?

As it happens, it does... Probably copypasta and/or some artifact of an earlier implementation based off a branch.

tnowacki · 2023-06-27T01:49:05Z

crates/sui-move-build/src/linters/share_owned.rs

+                };
+                self.lvalue(context, state, l, v)
+            }
+            return true;


thoughts on the bool thing? (where true is skip and false is continue)

I came up with it and I find it a bit confusing reading it lol

I didn't give it that much thought but I found it pretty sensible to work with.

tnowacki · 2023-06-27T01:49:14Z

crates/sui-move-build/tests/linter_tests.rs

+    Ok(())
+}
+
+fn run_tests(path: &Path) -> anyhow::Result<()> {


tnowacki · 2023-06-27T01:51:53Z

external-crates/move/move-compiler/src/command_line/compiler.rs

@@ -743,6 +743,36 @@ fn has_compiled_module_magic_number(path: &str) -> bool {
    num_bytes_read == BinaryConstants::MOVE_MAGIC_SIZE && magic == BinaryConstants::MOVE_MAGIC
 }

+pub fn move_check_for_errors(


Not really relevant for this PR just a comment:
We might want to rename it and flesh out this API (as it also does warnings now... this function is super old fwiw)

tnowacki · 2023-06-27T01:53:29Z

crates/sui-move-build/tests/linter_tests.rs

+    if update_baseline {
+        if has_diags {
+            fs::write(exp_path, rendered_diags)?;
+        } else if exp_path.is_file() {
+            fs::remove_file(exp_path)?;
+        }
+        return Ok(());
+    }
+
+    let exp_exists = exp_path.is_file();
+    if exp_exists {
+        let expected_diags = fs::read_to_string(exp_path)?;
+        if rendered_diags != expected_diags {
+            let msg = format!(
+                "Expected output differ from the actual one:\n{}",
+                format_diff(expected_diags, rendered_diags),
+            );
+            anyhow::bail!(add_update_baseline_fix(msg));
+        }
+    } else {
+        let msg = format!("Unexpected output :\n{}", rendered_diags);
+        anyhow::bail!(add_update_baseline_fix(msg));
+    }
+
+    Ok(())
+}


Also not relevant for this PR, but I've copy-pasted this so many times. We should maybe make a library for this or something idk

tnowacki · 2023-06-27T01:54:03Z

crates/sui-move-build/src/linters/share_owned.rs

+const SHARE_OWNED_DIAG: DiagnosticInfo = custom(
+    "Lint ",
+    Severity::Warning,
+    /* category */ 1,
+    /* code */ 1,
+    "possible owned object share",
+);


Any feedback you have for making this easier to manage? (specifically on the category/code side of things)

Not at the moment, but I will think about it when writing more lints!

…ith example linter

## Description Added support for writing linters: - integration with Sui CLI - a simple testing harness - an example linter to signal sharing of potentially owned objects ## Test Plan Additional testing framework being part of this PR can be used to test individual linters. --- If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process. ### Type of Change (Check all that apply) - [ ] protocol change - [x] user-visible impact - [ ] breaking change for a client SDKs - [ ] breaking change for FNs (FN binary must upgrade) - [ ] breaking change for validators or node operators (must upgrade binaries) - [ ] breaking change for on-chain data layout - [ ] necessitate either a data wipe or data migration ### Release notes Developers may now opt into running linters when building/testing/publishing/upgrading packages via CLI by specifying the `--lint` flag.

awelc requested a review from tnowacki June 23, 2023 11:10

awelc self-assigned this Jun 23, 2023

vercel bot had a problem deploying to Preview – multisig-toolkit June 23, 2023 11:18 Failure

vercel bot had a problem deploying to Preview – multisig-toolkit June 23, 2023 14:14 Failure

tnowacki approved these changes Jun 27, 2023

View reviewed changes

vercel bot had a problem deploying to Preview – multisig-toolkit June 27, 2023 14:24 Failure

awelc added 4 commits June 27, 2023 16:36

[move-compiler] Added preliminary support for writing linters along w…

079e810

…ith example linter

Added license header

e7b45b0

Fixed tests

8c9bbd2

Addressed review comments

79013e9

awelc force-pushed the aw/linter-framework branch from 769481f to 79013e9 Compare June 27, 2023 14:37

Added a missing lint field

68e11a3

awelc merged commit bbf4187 into main Jun 27, 2023

awelc deleted the aw/linter-framework branch June 27, 2023 16:26

awelc mentioned this pull request Jul 10, 2023

[move build] Example linters #11212

Closed

6 tasks

randall-Mysten mentioned this pull request Jul 26, 2023

[move-compiler] Suppressed linter warnings in Sui stdlib #13127

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move-compiler] Added preliminary support for writing linters #12650

[move-compiler] Added preliminary support for writing linters #12650

awelc commented Jun 23, 2023

vercel bot commented Jun 23, 2023 •

edited

Loading

tnowacki left a comment

tnowacki Jun 27, 2023

awelc Jun 27, 2023

tnowacki Jun 27, 2023

awelc Jun 27, 2023

tnowacki Jun 27, 2023

awelc Jun 27, 2023

tnowacki Jun 27, 2023

awelc Jun 27, 2023

tnowacki Jun 27, 2023

awelc Jun 27, 2023

tnowacki Jun 27, 2023

tnowacki Jun 27, 2023

tnowacki Jun 27, 2023

tnowacki Jun 27, 2023

awelc Jun 27, 2023

		let msg = "You may be trying to share an owned object.";
		let uid_msg = "This argument should in most cases come from an object created within the same function.";

	d.add_secondary_label((l, "A potentially shared object coming from here"))
	d.add_secondary_label((l, "A potentially owned object coming from here"))

[move-compiler] Added preliminary support for writing linters #12650

[move-compiler] Added preliminary support for writing linters #12650

Conversation

awelc commented Jun 23, 2023

Description

Test Plan

Type of Change (Check all that apply)

Release notes

vercel bot commented Jun 23, 2023 • edited Loading

tnowacki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Jun 23, 2023 •

edited

Loading