[move compiler] Report unnecessary mutable references #14216
Merged
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
tnowacki
requested review from
amnn,
cgswords,
awelc,
tzakian,
sblackshear and
dariorussi
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
tnowacki
requested review from
randall-Mysten and
ronny-mysten
as code owners
tzakian
approved these changes
Oct 26, 2023
| } = info; | ||
| if *is_mut && !*used_mutably { | ||
| let msg = "Mutable reference is never used mutably, \ | ||
| consider switching to an imutable reference '&' instead"; |
There was a problem hiding this comment.
nit:
Suggested change
| consider switching to an imutable reference '&' instead"; | |
| consider switching to an immutable reference '&' instead"; |
Comment on lines
219
to
226
| assert_eq!(exps.len(), 1); | ||
| let infos: &mut BTreeMap<ExpBasedID, RefExpInfo> = | ||
| &mut RefCell::borrow_mut(&self.mutably_used); | ||
| for e in exps { | ||
| let info = infos.get_mut(e).unwrap(); | ||
| assert!(info.param_name.is_none()); | ||
| info.param_name = Some(name) | ||
| } |
There was a problem hiding this comment.
Nit: Either there shouldn't be an assert here, or assert remains and the for loop should be rewritten to just directly access the zero-th exp so something like
let info = infos.get_mut(&exps[0]).unwrap();
...
jonas-lj
pushed a commit
to jonas-lj/sui
that referenced
this pull request
Nov 2, 2023
## Description - Report unused mutable references - The system is quite exhaustive and reports even locally created mutable references. It piggy-backs on the borrow checker but globally collects usages instead of keeping it in the abstract state. This is done mostly for simplicity - A mutable reference is considered unused if: - It is mutated - It is passed to a function (since freeze removes any subtyping issues) - It is returned - Copies share the same abstract reference ID ## Test Plan - Added new tests --- 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 With this change, Move will now suggest switching mutable references `&mut` to immutable ones `&` when the reference is not modified, or passed somewhere where the mutability is required. This can be suppressed for parameters by prefixing the parameter name with an underscore `_`.
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
Test Plan
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
With this change, Move will now suggest switching mutable references
&mutto immutable ones&when the reference is not modified, or passed somewhere where the mutability is required.This can be suppressed for parameters by prefixing the parameter name with an underscore
_.