[move-compiler] Added linter for accidentally freezing wrapped objects #13194

awelc · 2023-07-27T21:57:39Z

Description

Added a linter signaling a possibility of inadvertently freezing an object that (directly or indirectly) contains other (wrapped) objects. Freezing such object will prevent unwrapping of inner objects.

Added an additional visitor at the hlir level to have easy access to resolved variable types.

Test Plan

A new test has been added.

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

When building Move code, there are now additional linter warnings related to freezing an object containing (directly or indirectly) other (wrapped) object. Freezing such an object prevents unwrapping of inner objects.

vercel · 2023-07-27T21:57:46Z

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

Name	Status	Preview	Comments	Updated (UTC)
sui-typescript-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Aug 11, 2023 4:49pm

5 Ignored Deployments

Name	Status	Preview	Updated (UTC)
explorer	⬜️ Ignored (Inspect)	Visit Preview	Aug 11, 2023 4:49pm
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Aug 11, 2023 4:49pm
mysten-ui	⬜️ Ignored (Inspect)	Visit Preview	Aug 11, 2023 4:49pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Aug 11, 2023 4:49pm
sui-wallet-kit	⬜️ Ignored (Inspect)	Visit Preview	Aug 11, 2023 4:49pm

tzakian · 2023-07-27T23:33:14Z

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

+    ("sui", "transfer", "public_freeze_object"),
+    ("sui", "transfer", "freeze_object"),
+];


Some/all of these names are defined as constants now I believe, so probably best to use those here.

Fixed! An artifact of developing this in parallel with another PR that introduced these constants...

tzakian

Overall looks good to me -- a couple nits.

Main concern I have is if generic arguments with ability key are handled correctly, but I don't think that should be hard to double-check/fix.

tzakian · 2023-07-28T21:48:26Z

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

+            {
+                // single argument passed by value
+                let H::Type_::Single(st) = &fun.arguments.ty.value else {
+                    return;


Just to make sure I understand here and below: this is returning instead of ICE-ing to let typechecking handle this error?

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

tzakian · 2023-07-28T22:02:55Z

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

+    };
+
+    let sp!(_, tname) = tname;
+    if let H::TypeName_::ModuleType(mident, sname) = tname {


Does this cover the case where you try to freeze an object like

struct Wrapper<K: key> has key { id: UID, field: K, }

+1 for this, you should just be able to check that the field has key directly?

Good point and I clearly did miss it. Fixed.

crates/sui-move-build/tests/linter/freeze_wrapped.move

tnowacki · 2023-08-01T21:37:01Z

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

+    };
+
+    let sp!(_, tname) = tname;
+    if let H::TypeName_::ModuleType(mident, sname) = tname {


+1 for this, you should just be able to check that the field has key directly?

external-crates/move/move-compiler/src/hlir/visitor.rs

tnowacki · 2023-08-01T21:41:57Z

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

+                    if let Some((sfields, sloc)) = struct_fields(&sname, &mident, modules) {
+                        for (f, t) in sfields {
+                            if let Some((nested_sname, nested)) =
+                                contains_key(t, modules, /*field_depth*/ 0)


This feels a bit wasteful to check everytime? I feel like you should be able to pre-populate all of this information for every time before visiting expressions. In other words:

Visit all structs across all modules

You will need to do some sort of saturating pass for this I think? Which is maybe why you took this approach you have?

Record which have fields with key

Visit exps for freeze

I would advocate for the approach I laid out over some sort of memoized layer on top of what has been done here in that I would hope what I have suggested is more parallelizable in the future
(it might not look it, but other than a few annoying spots with Context, the pass structure of the compiler is designed to be highly amenable to parallelization (which we need to do some day))

tnowacki · 2023-08-01T21:43:05Z

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

@@ -0,0 +1,259 @@
+// Copyright (c) Mysten Labs, Inc.


I think this is fine to do over HLIR, but I think it will also work over typing?

The typing visitors are run after expansion, so all ability information should be present.
What issue were you anticipating with the typing visitor that caused you to reach for the HLIR?

Thanks for explaining the subtlety here. Reverted to using typed AST.

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

tnowacki

LGTM! Thanks for the improvements!

tnowacki · 2023-08-07T23:35:25Z

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

+                        return;
+                    };
+                    // see if info about fields of a give struct was already found
+                    let (key_field_info, info_inserted) = self.get_key_field(&mident, &sname);


It's fine, but I feel like I would try to encapsulate all of this logic inside of struct_fields somehow? Just feels weird that this area of the visitor knows about the memoization elsewhere

It was a little more complicated than I thought, but I think it does look better now. Thank you for the suggestion! I also did some renaming (from "key_field" to "wrapping_field" which I think is more intuitive, especially considering code comment explanations that were already there). Please take another look if you'd like!

## Description The original implementation of the freeze wrapped linter (#13194) had a bug - arguments to the transfer function were not processed to look for other cases where the linter warning should kick in. The case would have to look similarly to the example below, which makes it pretty unlikely, but still... ``` public fun freeze_arg(w1: Wrapper, w2: Wrapper) { transfer::public_freeze_object({ transfer::public_freeze_object(w1) ; w2}); } ``` ## Test Plan Added a test capturing the missing bit (same as example above) --- 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

#13194) ## Description Added a linter signaling a possibility of inadvertently freezing an object that (directly or indirectly) contains other (wrapped) objects. Freezing such object will prevent unwrapping of inner objects. Added an additional visitor at the hlir level to have easy access to resolved variable types. ## Test Plan A new test has been added. --- 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 When building Move code, additional linter warnings related to freezing an object containing (directly or indirectly) other (wrapped) object. Freezing such object will prevent unwrapping of inner objects.

## Description The original implementation of the freeze wrapped linter (#13194) had a bug - arguments to the transfer function were not processed to look for other cases where the linter warning should kick in. The case would have to look similarly to the example below, which makes it pretty unlikely, but still... ``` public fun freeze_arg(w1: Wrapper, w2: Wrapper) { transfer::public_freeze_object({ transfer::public_freeze_object(w1) ; w2}); } ``` ## Test Plan Added a test capturing the missing bit (same as example above) --- 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

## Description Adding protocol change highlight to release notes script ## Test Plan ``` eugene@mac-studio ~/code/sui (eugene/add_protocol_highlight_to_release_notes) $ ./scripts/generate-release-notes.sh releases/sui-v1.7.0-release releases/sui-v1.8.0-release Sui Protocol Version in this release: XX #13124: Add protocol config feature flags for zkLogin to enable testing in Devnet, use updated proof verification logics for zkLogin signature verification. #13417: When building Move code, there are now additional linter warnings related to comparing collections from Sui framework code (`Bag`, `Table`, and `TableVec`). Note that this comparison is not a structural one based on the collection content, which is what one might expect, so Sui now indicates this via a linter warning. #12989: All transaction execution errors from `execute_transaction_block` of `client-fault` now return a -32002 error code. If you encounter this error code, there is most likely an issue in your transaction inputs. Previously, when executing a transaction failed on the RPC, you would receive a, "Transaction has non recoverable errors from at least 1/3 of validators" after the transaction failed to execute. You now receive an improved error message, "Transaction execution failed due to issues with transaction inputs, please review the errors and try again: {errors}", where `{errors}` is a string list of actionable errors. After you resolve the errors indicated, your transaction should succeed. #13194: When building Move code, there are now additional linter warnings related to freezing an object containing (directly or indirectly) other (wrapped) object. Freezing such an object prevents unwrapping of inner objects. #12575: The details included in error messages returned during dependency graph construction might differ from the previous error messages, but they still include similar details and information. #12933: Error code designation is updated to support a more cohesive error reporting structure. Internal errors that arise while reading from authority return a `-32603` error code. Client-fault errors that arise while reading from authority return a `-32602` error code. Error strings are not modified. #13312: Removes the `--legacy-digest` flag from the `sui client upgrade` and `sui move build` CLI commands, as Sui networks no longer require package digests to be calculated using the legacy algorithm. ```

awelc self-assigned this Jul 27, 2023

vercel bot deployed to Preview – sui-typescript-docs July 27, 2023 22:03 View deployment

awelc requested a review from tzakian July 27, 2023 23:16

tzakian reviewed Jul 27, 2023

View reviewed changes

vercel bot deployed to Preview – sui-typescript-docs July 27, 2023 23:37 View deployment

awelc force-pushed the aw/linter-freeze-key branch from d6d854a to 2419fbf Compare July 28, 2023 01:02

vercel bot deployed to Preview – sui-typescript-docs July 28, 2023 01:02 View deployment

vercel bot deployed to Preview – mysten-ui July 28, 2023 01:03 View deployment

tzakian reviewed Jul 28, 2023

View reviewed changes

tnowacki reviewed Aug 1, 2023

View reviewed changes

vercel bot deployed to Preview – sui-typescript-docs August 2, 2023 02:07 View deployment

vercel bot deployed to Preview – sui-typescript-docs August 2, 2023 17:34 View deployment

vercel bot deployed to Preview – sui-typescript-docs August 2, 2023 22:11 View deployment

vercel bot deployed to Preview – sui-typescript-docs August 3, 2023 23:15 View deployment

awelc force-pushed the aw/linter-freeze-key branch from 37a41ac to 6292d4e Compare August 7, 2023 20:37

vercel bot deployed to Preview – mysten-ui August 7, 2023 20:37 View deployment

vercel bot deployed to Preview – sui-typescript-docs August 7, 2023 20:38 View deployment

vercel bot deployed to Preview – sui-typescript-docs August 7, 2023 20:39 View deployment

tnowacki approved these changes Aug 7, 2023

View reviewed changes

vercel bot deployed to Preview – sui-typescript-docs August 11, 2023 01:12 View deployment

awelc added 9 commits August 10, 2023 18:37

[move-compiler] Added linter for accidentally frezing wrapped objects

a44f479

Fixed formatting

9c73c94

Added an extra test

18d89b9

Added constants

3f3df70

Redid the basic implementation in typed AST and removed HLIR visitor

5f6ad26

Added type param fields handling

6669793

Further finessed error messages

e4377c3

Addressed review comments

d7879a5

Regenerated test output

5544155

Refactoring to keep memoization in one place plus some renamings

386bdaa

awelc force-pushed the aw/linter-freeze-key branch from 86dd247 to 386bdaa Compare August 11, 2023 01:38

vercel bot deployed to Preview – sui-typescript-docs August 11, 2023 01:38 View deployment

vercel bot deployed to Preview – mysten-ui August 11, 2023 01:38 View deployment

Regenerated tests

ea900c5

vercel bot deployed to Preview – sui-typescript-docs August 11, 2023 01:41 View deployment

Fiexd a compilation error

1bc2511

vercel bot deployed to Preview – sui-typescript-docs August 11, 2023 16:49 View deployment

awelc merged commit 1e1edb7 into main Aug 11, 2023
38 checks passed

awelc deleted the aw/linter-freeze-key branch August 11, 2023 17:06

awelc mentioned this pull request Aug 17, 2023

[move-compiler] A fix to the freeze wrapped linter #13455

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[move-compiler] Added linter for accidentally freezing wrapped objects #13194

[move-compiler] Added linter for accidentally freezing wrapped objects #13194

awelc commented Jul 27, 2023 •

edited by randall-Mysten

Loading

vercel bot commented Jul 27, 2023 •

edited

Loading

tzakian Jul 27, 2023

awelc Jul 27, 2023

tzakian left a comment

tzakian Jul 28, 2023

awelc Aug 2, 2023

tzakian Jul 28, 2023

tnowacki Aug 1, 2023

awelc Aug 2, 2023

tnowacki Aug 1, 2023

tnowacki Aug 1, 2023

tnowacki Aug 1, 2023

awelc Aug 3, 2023

tnowacki Aug 1, 2023

awelc Aug 2, 2023

tnowacki left a comment

tnowacki Aug 7, 2023

awelc Aug 11, 2023

[move-compiler] Added linter for accidentally freezing wrapped objects #13194

[move-compiler] Added linter for accidentally freezing wrapped objects #13194

Conversation

awelc commented Jul 27, 2023 • edited by randall-Mysten Loading

Description

Test Plan

Type of Change (Check all that apply)

Release notes

vercel bot commented Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzakian 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

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

awelc commented Jul 27, 2023 •

edited by randall-Mysten

Loading

vercel bot commented Jul 27, 2023 •

edited

Loading