[move-compiler] Added the collection equality check linter #13417
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Ignored Deployments
|
tnowacki
requested changes
Aug 16, 2023
| | E::Copy { .. } | ||
| | E::Use(_) | ||
| | E::Constant(..) | ||
| | E::ModuleCall(_) |
There was a problem hiding this comment.
Missing the recursive case for the args in ModuleCall
tnowacki
reviewed
Aug 16, 2023
| }) | ||
| { | ||
| let msg = format!( | ||
| "Comparing collections of type '{caddr}::{cmodule}::{cname}' may yield unexpected result." |
There was a problem hiding this comment.
I am worried the generality of this language might cause folks to think this is also true for vector (which it is not)
| "Comparing collections of type '{caddr}::{cmodule}::{cname}' may yield unexpected result." | ||
| ); | ||
| let note_msg = | ||
| "Equality for collections IS NOT a structural check based on content"; |
There was a problem hiding this comment.
Maybe we can just remove the word collections and just refer to the type? Not sure
There was a problem hiding this comment.
I changed this to "Equality for collections of type '{caddr}::{cmodule}::{cname}' IS NOT a structural check based on content" to re-emphasize that it's not about all collections but rather about collections of a given type.
tnowacki
reviewed
Aug 16, 2023
Comment on lines
+124
to
127
| let N::Type_::Apply(_,tname, _) = &bt.value else { | ||
| // not a struct type | ||
| return; | ||
| return true; | ||
| }; |
There was a problem hiding this comment.
Wait, shouldn't these be return false?
You want to keep exploring the sub arguments
For example
freeze<T>({ freeze<MyObjWithWrappedObjects>(obj); x })
While silly, it is still a possible case
There was a problem hiding this comment.
Yeah, it did not occur to me that someone could use a "void" function in any meaningful way in an argument, but better be safe than sorry.
I guess we have to process arguments in ALL cases, though, even if the diag gets generated, so visit_exp_custom should return false no matter what.
Will fix it once #13335 lands in case you need to make some more visitor changes so that I don't have to deal with conflicts.
tnowacki
approved these changes
Aug 16, 2023
damirka
pushed a commit
that referenced
this pull request
Aug 23, 2023
## Description Added a linter to signal a potentially useless comparison of two collections which the developer may expect to be based on comparing the collection content, but which is not implemented this way. ## 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, there are now additional linter warnings related to comparing collections from Sui framework code (`Bag`, `Table` and `TableVec`). The comparison IS NOT a structural one based on the collection content which is what the developers may be expecting so we now signal it to them via a linter warning.
ebmifa
added a commit
that referenced
this pull request
Aug 29, 2023
## 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. ```
randall-Mysten
pushed a commit
that referenced
this pull request
Sep 6, 2023
## 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. ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Added a linter to signal a potentially useless comparison of two collections which the developer may expect to be based on comparing the collection content, but which is not implemented this way.
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)
Release notes
When building Move code, there are now additional linter warnings related to comparing collections from Sui framework code (
Bag,Table, andTableVec). 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.