&mut &T coerced to &T suggests &mut mut x
#57431
Comments
|
(note that this isn't match_ergonomics specific-- the issue reproduces if the match is changed to |
|
So the issue here is with the coercion, not with match ergonomics. (play) struct X;
impl X {
fn mutate(&mut self) {}
}
pub fn foo(x: bool) {
let mut term = X;
let ref_term = if x {
&mut term
} else {
&X
};
ref_term.mutate();
}Also, plz no parsing Rust with regular expressions: struct X;
impl X {
fn mutate(&mut self) {}
}
pub fn foo(x: bool) {
let mut term = X;
let ref_term = if x {
& /* c/*om*/ment */ mut term
} else {
&X
};
ref_term.mutate();
} |
|
@spastorino I would like to help with this. |
|
@saleemjaffer I know more or less how to fix the issue but never found time to tackle it. So yeah, go ahead. |
|
NLL triage. Assigning P-high priority, because actively misleading the user can impede the learning process about the language as a whole |
|
@spastorino would you be willing to provide mentorship for @saleemjaffer, outlining the approach you were thinking of for resolving it? |
|
@pnkfelix @saleemjaffer sure, ping me on Zulip. Until March 31 I'm very busy with Rust Latam though, still ping me and we can see how I can help. |
|
Sure, I'll be picking this up in a day. |
|
Just to be clear: struct X;
impl X {
fn mutate(&mut self) {}
}
pub fn foo(x: bool) {
let mut term = X;
let ref_term = if x {
&mut term
} else {
&X
};
ref_term.mutate();
}Instead of this error: what exactly do we want to show? Do we want to not show this: This way the user understands that |
|
@saleemjaffer exactly, we want to avoid the help suggestion. |
|
rust/src/librustc_mir/borrow_check/mutability_errors.rs Lines 374 to 424 in dd363d1 In particular this is where the issue occurs: rust/src/librustc_mir/borrow_check/mutability_errors.rs Lines 385 to 395 in dd363d1 |
|
hey @saleemjaffer how are you going with this?. Sorry for the absense but I was running Rust Latam and I had zero time. Let me know your progress, feel free to create a Zulip thread and start talking about this. |
|
@rustbot release-assignment Just figured that I was still assigned to this. |
|
Error: Cannot release unassigned issue Please let |
|
The problem as shown in @arielb1's reduction no longer reproduces. Click for info about when the behavior on the reduction changedThat change happened between rustc 1.37.0-nightly (0dc9e9c 2019-06-15) and rustc 1.37.0-nightly (4edff84 2019-06-16). The following PR's landed in that time period: And thanks to rustup-toolchain-install-master, I was able to quickly install the most relevant of the above commits, in order to determine that yes, this bug was resolved by #60730: we now issue the following diagnostic here: However, I believe this is merely a masking of a still present bug; going back to @cramertj 's original example, we continue to see the following diagnostic: |
|
Ah, here is a way to fix @arielb1 's reduction (and reduce it even further!) (play): struct X;
impl X {
fn mutate(&mut self) {}
}
pub fn foo() {
let mut term = X;
let ref_term: &X = &mut term;
ref_term.mutate();
}
fn main() { }which currently emits the diagnostic: |
|
As hinted at in discussions on PR #59808, there are two tacks we might take with this:
|
|
@pnkfelix I was actually working on #59808. I have some time now and can pick this up again!
I actually figured out where in the code the complier inferred a |
|
Hi @saleemjaffer ; I was on leave, but I am back now. Sometimes we decide a given short-term solution is not worth the effort, and jump straight to a long-term solution. I.e. if initial investigation of the short-term solution indicates that it is nearly as much effort as the long-term one would be. My guess is that this is not one of those cases. I believe we could have people independently work on the short- and long-term solutions being suggested here. (That's my way of saying: Feel free to work on whichever aspect you prefer. If you think you can make progress on the long-term fix, feel free.) |
|
Yeah I can pick this up in a few days!
…On Wed, 11 Sep 2019, 15:09 Felix S Klock II, ***@***.***> wrote:
Hi @saleemjaffer <https://github.com/saleemjaffer> ; I was on leave, but
I am back now.
Sometimes we decide a given short-term solution is not worth the effort,
and jump straight to a long-term solution. I.e. if initial investigation of
the short-term solution indicates that it is nearly as much effort as the
long-term one would be.
My guess is that this is not one of those cases. I believe we could have
people independently work on the short- and long-term solutions being
suggested here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#57431?email_source=notifications&email_token=AESUZVDC65O7JBL3HXUQHHLQJC4ETA5CNFSM4GORVH42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6N4YAQ#issuecomment-530304002>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AESUZVAJ5VPOB6UIQFBEYXLQJC4ETANCNFSM4GORVH4Q>
.
|
|
Looking into this one |
|
Here's what I'm thinking: Step 1: Remove the help dialog entirely - it's point the user in the wrong direction entirely. Step 2: Somehow identify that a coercion has taken place and indicate that to the user to give them a clue as to how to fix the issue. The fix for: struct X;
impl X {
fn mutate(&mut self) {}
}
pub fn foo() {
let mut term = X;
let ref_term: &X = &mut term;
ref_term.mutate();
}
fn main() { }is struct X;
impl X {
fn mutate(&mut self) {}
}
pub fn foo() {
let mut term = X;
let ref_term: &mut X = &mut term;
ref_term.mutate();
}
fn main() { }Identifying every type of coercion would be quite difficult - but by indicating to the user that a coercion has likely occurred, we can hopefully point them in the correct direction to fix their code. |
|
I have a failing test in place on my fork: |
|
On initial investigation, I think the place to fix this may be in the check_access_permissions method of the borrow checker. |
|
I did a pretty thorough debug trace through the borrow checker, trying to figure out what borrow rule I should focus on editing. I first added some additional debug statements to the borrow checker here to allow me to better track the process with which the code in my test is executed by the borrow checker. I then built the stage 1 compiler with ./x.py build --stage 1Then I ran my test in debug mode with: RUSTC_LOG=debug /home/nell/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc /home/nell/rust/src/test/ui/borrowck/issue-57431-coerced-mut-reference.rsThis produced A LOT of debug output. In my test, this line is the one that fails the borrow check, so I focused on the debug statements for when the borrow checker was processing line 11. They are: Based on this, I think that I need to modify one of these read or write kinds.
And my instinct is to look further into Write(Mutate) first. Edit: |
|
@nellshamrell you can use more granular log filtering like |
|
Removing the NLL-diagnostics label, since the output is the same both with and without |
|
The diagnostic is currently pointing you in the right direction, but we still don't correctly look at the |
|
Current output: |
|
Current output, aesthetic changes only: |
This code (play):
Produces these errors:
Note that the second error suggests changing
&mut termto&mut mut term, which obviously won't fix the issue.The text was updated successfully, but these errors were encountered: