Add detailed error explanation for E0389 #33412

cramertj · 2016-05-04T21:01:20Z

rust-highfive · 2016-05-04T21:01:36Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Aatch (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

GuillaumeGomez · 2016-05-04T22:27:38Z

src/librustc_borrowck/diagnostics.rs

@@ -286,6 +286,54 @@ You can read more about cell types in the API documentation:
 https://doc.rust-lang.org/std/cell/
 "##,

+E0389: r##"
+An attempt was made to assign to data in a non-mutable reference. Example:


Not very clear. Also, you have to write "Example of erroneous code" and not just "Example" as stated by the RFC. It's to keep error explanations homogeneous.

Ah, okay. The RFC doesn't seem particularly consistent-- it uses "Example of erroneous code:", "Erroneous code example:", and other errors currently in the same file as E0389 & co. use "For example:", and "Example:". It makes sense to have a standard, though, so perhaps there should be a PR changing the old ones. I can take a look at that after this one if you'd like.

Sure, but before I'd prefer this RFC to be definitely accepted. Once done, you can do it if you want (it'd be very appreciated actually!).

Okay. In that case, do you want to modify the RFC to change line 86 to say "Example of erroneous code:"?

GuillaumeGomez · 2016-05-04T22:29:56Z

Thanks for your work! Please fix the errors and then it should be good to get merged.

cramertj · 2016-05-04T23:02:32Z

@GuillaumeGomez Done. Squash?

GuillaumeGomez · 2016-05-04T23:07:49Z

src/librustc_borrowck/diagnostics.rs

@@ -286,6 +286,66 @@ You can read more about cell types in the API documentation:
 https://doc.rust-lang.org/std/cell/
 "##,

+E0389: r##"
+An attempt was made to mutate data using a non-mutable reference. This
+commonly occurs when attempting to assign to a reference of type


I think your second sentence is a bit unwieldly. What do you think of: "This commonly occurs when attempting to assign to a non-mutable reference of a mutable reference"?

Haha that sounds crazy to me-- that's why I just wrote the type out directly ("a non-mutable reference of a mutable reference" vs. &(&mut T)). But if you prefer, I'm happy to change it as I don't think it's going to greatly confuse anyone either way.

Typecit out and keep the code in a parenthetical 😄

I love it when we can get the best of both worlds 😄

You're demons! 😨

GuillaumeGomez · 2016-05-04T23:08:06Z

Yes please.

Manishearth · 2016-05-05T10:25:11Z

src/librustc_borrowck/diagnostics.rs

+}
+```
+
+Here, `&mut fancy` is mutable, but `&(&mut fancy)` is not. To fix this, either


Could you provide a more in-depth explanation as to why &&mut is immutable? This can trip up newcomers who come across it, since it looks like the inner value should be mutable.

(It isn't because &&mut can be aliased, which means that &T transitively makes all mutable references inside it immutable)

How about, "Creating an immutable reference to a value borrows that value immutably, so neither it nor its contents can be modified." (Should I mention interior mutability?)

"Creating an immutable reference to a value borrows it immutably, so neither it nor its contents can be modified." (a little better from my point of view 😉 )

GuillaumeGomez · 2016-05-06T11:26:38Z

@Manishearth: Seems good for me now.

Manishearth · 2016-05-06T11:32:54Z

@GuillaumeGomez where's the in-depth explanation?

Manishearth · 2016-05-06T11:34:45Z

The point here is that &&mut at first glance for many people sounds like it should be possible to mutate the inner value, just not change the reference itself.

The reason that it's not allowed is a bit nuanced. &T can alias, so &&mut T can also alias. If it can be aliased, we should not allow mutation, so &T freezes the inner value from mutation even via internal &mut references. We should explain this reason.

GuillaumeGomez · 2016-05-06T14:28:07Z

@Manishearth: I thought "Here, &mut fancy is mutable, but &(&mut fancy) is not. Creating an immutable reference to a value borrows it immutably, so neither it nor its contents can be modified. To fix this, either remove the outer reference:" explained it. Kinda tricky :-/

cramertj · 2016-05-06T17:20:46Z

@Manishearth I don't think the "alias" explanation is particularly transparent. I personally prefer the explanation that I provided (the one @GuillaumeGomez referenced). If anything, I think a further clarification might include that there can be multiple references of type &(&mut T), so it must be immutable to prevent multiple mutable references to the same value (equivalent to aliasing, but less confusing terminology IMO).

Manishearth · 2016-05-06T17:34:55Z

Yes, that further clarification is what I was looking for. I used alias whilst explaining the concept I felt should be included to you, not intending it to be included in the text 😄rust-lang/rust wrote:@Manishearth I don't think the "alias" explanation is particularly transparent. I personally prefer the explanation that I provided (the one @GuillaumeGomez referenced). If anything, I think a further clarification might include that there can be multiple references of type &(&mut T), so it must be immutable to prevent multiple mutable references to the same value (equivalent to aliasing, but less confusing terminology IMO).

—You are receiving this because you were mentioned.Reply to this email directly or view it on GitHub

cramertj · 2016-05-06T17:38:20Z

Ah, okay. I'll add that then.

Manishearth · 2016-05-06T17:41:41Z

@GuillaumeGomez that doesn't explain why *(&&mut x) is still immutable ;)

Cleanup of E0389 Added type-d out version of type in E0389 description

cramertj · 2016-05-06T17:48:24Z

Change made.

Manishearth · 2016-05-07T11:46:43Z

@bors r+ rollup

bors · 2016-05-07T11:46:45Z

📌 Commit f25cbe6 has been approved by Manishearth

Add detailed error explanation for E0389 Part of rust-lang#32777

Rollup of 20 pull requests - Successful merges: #33256, #33283, #33313, #33314, #33326, #33336, #33382, #33384, #33402, #33409, #33410, #33412, #33428, #33430, #33437, #33438, #33439, #33442, #33456, #33459 - Failed merges:

rust-highfive assigned Aatch May 4, 2016

GuillaumeGomez reviewed May 4, 2016
View reviewed changes

GuillaumeGomez assigned GuillaumeGomez and unassigned Aatch May 4, 2016

GuillaumeGomez reviewed May 4, 2016
View reviewed changes

cramertj force-pushed the E0389 branch 2 times, most recently from 868dfe7 to 04b45ad Compare May 4, 2016 23:46

Manishearth reviewed May 5, 2016
View reviewed changes

cramertj force-pushed the E0389 branch from 04b45ad to b379c3e Compare May 5, 2016 21:09

alexcrichton force-pushed the master branch from 8514dd9 to 413bafd Compare May 5, 2016 22:04

Add detailed error explanation for E0389

f25cbe6

Cleanup of E0389 Added type-d out version of type in E0389 description

cramertj force-pushed the E0389 branch from b379c3e to f25cbe6 Compare May 6, 2016 17:48

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 7, 2016

Rollup merge of rust-lang#33412 - cramertj:E0389, r=Manishearth

a6f6888

Add detailed error explanation for E0389 Part of rust-lang#32777

GuillaumeGomez mentioned this pull request May 7, 2016

Rollup of 21 pull requests #33482

Closed

steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 7, 2016

Rollup merge of rust-lang#33412 - cramertj:E0389, r=Manishearth

6ae1ac8

Add detailed error explanation for E0389 Part of rust-lang#32777

steveklabnik mentioned this pull request May 7, 2016

Rollup of 21 pull requests #33486

Closed

steveklabnik added a commit to steveklabnik/rust that referenced this pull request May 7, 2016

Rollup merge of rust-lang#33412 - cramertj:E0389, r=Manishearth

0b8a3d5

Add detailed error explanation for E0389 Part of rust-lang#32777

steveklabnik mentioned this pull request May 7, 2016

Rollup of 20 pull requests #33487

Merged

bors merged commit f25cbe6 into rust-lang:master May 7, 2016

cramertj deleted the E0389 branch May 25, 2016 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add detailed error explanation for E0389 #33412

Add detailed error explanation for E0389 #33412

cramertj commented May 4, 2016

rust-highfive commented May 4, 2016

GuillaumeGomez May 4, 2016

cramertj May 4, 2016

GuillaumeGomez May 4, 2016

cramertj May 4, 2016

GuillaumeGomez commented May 4, 2016

cramertj commented May 4, 2016

GuillaumeGomez May 4, 2016

cramertj May 4, 2016

Manishearth May 4, 2016

cramertj May 4, 2016

GuillaumeGomez May 4, 2016

GuillaumeGomez commented May 4, 2016

Manishearth May 5, 2016

cramertj May 5, 2016

GuillaumeGomez May 5, 2016

cramertj May 5, 2016

GuillaumeGomez commented May 6, 2016 •

edited

Loading

Manishearth commented May 6, 2016

Manishearth commented May 6, 2016 •

edited

Loading

GuillaumeGomez commented May 6, 2016

cramertj commented May 6, 2016

Manishearth commented May 6, 2016

cramertj commented May 6, 2016

Manishearth commented May 6, 2016

cramertj commented May 6, 2016

Manishearth commented May 7, 2016

bors commented May 7, 2016

Add detailed error explanation for E0389 #33412

Add detailed error explanation for E0389 #33412

Conversation

cramertj commented May 4, 2016

rust-highfive commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented May 4, 2016

cramertj commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented May 6, 2016 • edited Loading

Manishearth commented May 6, 2016

Manishearth commented May 6, 2016 • edited Loading

GuillaumeGomez commented May 6, 2016

cramertj commented May 6, 2016

Manishearth commented May 6, 2016

cramertj commented May 6, 2016

Manishearth commented May 6, 2016

cramertj commented May 6, 2016

Manishearth commented May 7, 2016

bors commented May 7, 2016

GuillaumeGomez commented May 6, 2016 •

edited

Loading

Manishearth commented May 6, 2016 •

edited

Loading