Remove "consider specifying this binding's type" when reference differs in mutability #115648
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
|
r? @jackh726 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
jmintb
changed the title
|
☔ The latest upstream changes (presumably #115308) made this pull request unmergeable. Please resolve the merge conflicts. |
issue: rust-lang#113767 This commit disables the "consider changing this bindings type" suggestion in cases where the initial value of a binding is not mutable and the suggestion is therefore invalid.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
jackh726
requested changes
Oct 7, 2023
There was a problem hiding this comment.
It's times like this that I think that actually just "applying" suggested fix and checking if the new code would compile would be super helpful.
I think this is just an improvement, even if not perfect. Please bless the broken tests, address the review comments, and fix formatting. Should move the added test to a proper directory with a useful name - and add a comment on what the test is testing. (Alternatively, could move the test case into an existing test if there's one that fits.
Comment on lines
1220
to
1221
| if let Some(hir_id) = hir_id | ||
| && let Some(hir::Node::Local(local)) = hir_map.find(hir_id) |
There was a problem hiding this comment.
So, the added checks here don't cover this else case. Which can have the same problems?
| Some(hir::ExprKind::Path(hir::QPath::Resolved(_, hir::Path{res: Res::Local(id), ..} ))) => hir_map.find(*id), | ||
| Some(hir::ExprKind::Call(Expr { kind: ExprKind::Path(QPath::Resolved(_, Path {res: Res::Def(DefKind::Fn, defid ) , ..} ), ..), ..}, .. )) => { | ||
| let Some(local_id) = defid.as_local() else { | ||
| todo!("What do we do here?") |
There was a problem hiding this comment.
We could have a fallback case that just has a more lax "you may be able to change the mutability here". I'm not sure if the binding info is available cross-crates.
| ), | ||
| .. | ||
| })) => mut_ty.mutbl, | ||
| _ => Mutability::Mut, // TODO: this probably is not correct handling of this case. |
There was a problem hiding this comment.
Eh, it's okay. Just leave a note that other cases might not be covered.
|
|
||
| // If the inital value of an expression is not mutable then suggesting that the user | ||
| // changes the type of the expression to be mutable is incorrect is it will not help. | ||
| // TODO: Could we provide an alternative suggestions around altering the mutability |
There was a problem hiding this comment.
This is fine to leave as a FIXME, or just remove.
| @@ -1224,6 +1278,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> { | |||
| format!(": {message}"), | |||
| ), | |||
| }; | |||
| // FOUND IT | |||
jackh726
added
S-waiting-on-author
|
☔ The latest upstream changes (presumably #118076) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@jmintb any updates on this |
Sorry I forgot to come back to this. Will get on it soon. |
|
@jmintb any updates on this? thanks |
|
Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks |
Dylan-DPC
added
S-inactive
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.
This is a draft PR for #113767. It introduces a new test case and a mitigation which seems to work. I am submitting this now to get feedback before refining it further. I am pretty sure I am not covering all cases yet and I need to cleanup the rather insane destructuring haha.
The idea is to check for cases where the original value of a binding is immutable and then avoid suggesting that the type be specified in those cases. As that would not help.
To do this we first find the initial expression's kind HIR ID from the information in
local.init.kind, then use a map of HIR IDs to find aNode. Finally we determine it's mutability by destructing theNodeto find it's type information.I have a few questions:
This change causes three existing tests to fail
tests/ui/borrowck/borrowck-borrow-mut-base-ptr-in-aliasable-loc.rs,tests/ui/issues/issue-51515.rsandtests/ui/process/nofile-limit.rs, As far as I can tell the code is working correctly and the test needs to be updated.We might be able to provide a better suggestion instead of just removing the existing one. Like "consider changing the mutability of the function/value used to initaliase your variable". Formulated more elegantly 😅