Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Suggest dereferencing non-lval mutable reference on assignment #94639

Merged
merged 3 commits into from
May 18, 2022
Merged

Suggest dereferencing non-lval mutable reference on assignment #94639

merged 3 commits into from
May 18, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Mar 5, 2022 •
edited
Loading

  1. Adds deref suggestions for LHS of assignment (or assign-binop) when it implements DerefMut
  2. Fixes missing deref suggestions for LHS when it isn't a place expr

Fixes #46276
Fixes #93980

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 5, 2022
@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2022
@compiler-errors
Copy link
Member Author

r? rust-lang/compiler

wesleywiser
wesleywiser reviewed Apr 7, 2022
View reviewed changes
compiler/rustc_typeck/src/check/coercion.rs Outdated
@@ -963,6 +964,23 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
.find_map(|(ty, steps)| self.probe(|_| coerce.unify(ty, target)).ok().map(|_| steps))
}

/// Given a type, this function will calculate and return the type given
/// for `<Ty as Deref>::Target` only if `Ty` also implements `DerefMut`.
/// This function is for diagnostics, since it does not register sub-obligations.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit concerned this statement is easy to miss. Is it possible we can move this method to somewhere in the diagnostics code? Or perhaps naming this method so that it includes some mention of this being for diagnostics only.

src/test/ui/typeck/issue-93486.stderr Outdated
Comment on lines 9 to 12
help: consider dereferencing here to assign to the mutable borrowed piece of memory
|
LL | *vec![].last_mut().unwrap() = 3_u8;
| +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know why we lost this message? (It seems like it is correct unlike the one in the previous commit so I'm surprised it's missing.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's bc i added must_apply_modulo_regions when checking if the type implements the trait, instead of may_apply, and the fact that we have an infer var on the lhs. i'll see if it causes too many false errors here.

@compiler-errors
Copy link
Member Author

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 14, 2022
@compiler-errors
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 21, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
compiler-errors
compiler-errors commented Apr 21, 2022
View reviewed changes
src/test/ui/issues/issue-5239-1.stderr
|
help: `+=` can be used on `isize`, you can dereference `x`
|
LL | let x = |ref x: isize| { *x += 1; };
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is kinda a regression, but not actually... &isize doesn't have a +=. Perhaps we could suggest changing the ref to ref mut, but that seems out of scope for this PR.

wesleywiser
wesleywiser approved these changes Apr 26, 2022
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with or without nit. Nice work! 💯

compiler/rustc_typeck/src/check/expr.rs Outdated
Comment on lines 1066 to 1067
"consider dereferencing here to assign to the mutable \
borrowed piece of memory",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "piece of" just reads a bit awkwardly to me. Maybe "consider dereferencing here to assign to the mutably borrowed memory"?

@wesleywiser wesleywiser added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2022
@compiler-errors
Copy link
Member Author

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented May 18, 2022

📌 Commit d50d3fc has been approved by wesleywiser

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 18, 2022
@Dylan-DPC Dylan-DPC mentioned this pull request May 18, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request May 18, 2022
@bors
Auto merge of rust-lang#97135 - Dylan-DPC:rollup-06u9pqn, r=Dylan-DPC
a084b7a 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#94639 (Suggest dereferencing non-lval mutable reference on assignment)
 - rust-lang#95979 (update coherence docs, fix generator + opaque type ICE)
 - rust-lang#96378 (Mention traits and types involved in unstable trait upcasting)
 - rust-lang#96917 (Make HashMap fall back to RtlGenRandom if BCryptGenRandom fails)
 - rust-lang#97101 (Add tracking issue for ExitCode::exit_process)
 - rust-lang#97123 (Clean fix for rust-lang#96223)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 64c58a1 into rust-lang:master May 18, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 18, 2022
estebank
estebank reviewed May 18, 2022
View reviewed changes
compiler/rustc_typeck/src/check/demand.rs
Comment on lines +704 to +705
// Suppressing this diagnostic, we'll properly print it in `check_expr_assign`
return None;
Copy link
Contributor

@estebank estebank May 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@compiler-errors For future reference, adding a delay_span_bug here might be a good idea to protect from miscompilations in the face of refactors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shoot, yeah. i can add one in a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this just suppresses a suggestion, not the error itself. And the other case I actually did suppress a real error, I downgraded it to a delayed bug. See: https://github.com/rust-lang/rust/pull/94639/files#diff-c46757032f463a1170b40e5c41d569ce058668cfe259774fb8a7936e3fdd82f4R57

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@compiler-errors perfect!

@steffahn steffahn mentioned this pull request Sep 3, 2022
Closed
@compiler-errors compiler-errors deleted the rval-mutref branch August 11, 2023 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank left review comments

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.63.0
8 participants
@compiler-errors @rust-highfive @rust-log-analyzer @bors @wesleywiser @estebank @matthewjasper @rustbot