Point to immutable arg/fields when trying to use as &mut by estebank · Pull Request #39139 · rust-lang/rust

estebank · 2017-01-17T22:17:35Z

Present the following output when trying to access an immutable borrow's
field as mutable:

error[E0389]: cannot borrow data mutably in a `&` reference
--> $DIR/issue-38147-1.rs:27:9
|
26 | fn f(&self) {
| ----- use `&mut self` here to make mutable
27 | f.s.push('x');
| ^^^ assignment into an immutable reference

And the following when trying to access an immutable struct field as mutable:

error: cannot borrow immutable borrowed content `*self.s` as mutable
--> $DIR/issue-38147-3.rs:17:9
|
12 | s: &'a String
| ------------- use `&'a mut String` here to make mutable
...|
16 | fn f(&self) {
| ----- use `&mut self` here to make mutable
17 | self.s.push('x');
| ^^^^^^ cannot borrow as mutable

Fixes#38147.

rust-highfive · 2017-01-17T22:17:47Z

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

estebank · 2017-01-17T22:39:33Z

CC @jonathandturner @GuillaumeGomez @nikomatsakis

GuillaumeGomez · 2017-01-17T23:35:40Z

Why suddenly cloning them?

Leftover from previous exploration.

GuillaumeGomez · 2017-01-17T23:36:41Z

This is great! 👍

nrc · 2017-01-18T01:23:12Z

r? @nikomatsakis (I'm not familiar enough with these bits of the compiler any more).

I would prefer less assertive language in the error messages - rather than "X should be Y", I prefer, "if you want X, then you can change X to Y" or something similar. Also, could these be suggestions rather than labels? One day I would like to be able to auto-apply suggestions like this and it makes it easier if they are declared as such.

estebank · 2017-01-18T02:43:49Z

Introduced an error in two tests (removed two hints). Will try to fix it tonight.

estebank · 2017-01-18T07:21:11Z

I would prefer less assertive language in the error messages - rather than "X should be Y", I prefer, "if you want X, then you can change X to Y" or something similar.

@nrc, I'm ok with doing so. For now, I copied the message already being used for the same kind of suggestion:

error: cannot borrow immutable borrowed content `*self.s` as mutable
--> $DIR/issue-38147-3.rs:17:9
|
12 | s: &'a String
| ------------- this field should be `&mut`
...|
16 | fn f(&self) {
| ----- use `&mut self` here to make mutable
17 | self.s.push('x');
| ^^^^^^

Also, could these be suggestions rather than labels? One day I would like to be able to auto-apply suggestions like this and it makes it easier if they are declared as such.

The only things that makes me uneasy about using suggestions are that the actual output turns out to be slightly longer, and because of the out of order and lack of line number, less comprehensible:

error: cannot borrow immutable borrowed content `*self.s` as mutable
--> $DIR/issue-38147-3.rs:17:9
|
12 | s: &'a String
| ------------- this field should be `&mut`
...
17 | self.s.push('x');
| ^^^^^^
|
help: try making this a mutable borrow
| fn f(&mut self) {

I just filed #39152 so it can be improved going forward, but as it is now I really prefer the span_label presentation.

sophiajt · 2017-01-19T02:28:44Z

I'm definitely liking this revision. One question, is this part missing something?

17 | self.s.push('x');
| ^^^^^^

Seems like it needs a label there

estebank · 2017-01-19T02:33:27Z

@jonathandturner the current output doesn't set a label. To add one, I could definitely use some help with the wording. Something along the lines of:

error: cannot borrow immutable borrowed content `*self.s` as mutable
--> $DIR/issue-38147-3.rs:17:9
17 | self.s.push('x');
| ^^^^^^ cannot borrow as mutable

or

error: cannot borrow immutable borrowed content `*self.s` as mutable
--> $DIR/issue-38147-3.rs:17:9
17 | self.s.push('x');
| ^^^^^^ cannot borrow immutable borrowed content as mutable

seems reasonable.

sophiajt · 2017-01-19T02:43:13Z

@estebank choice #1 sounds good :)

estebank · 2017-01-19T06:58:53Z

@jonathandturner done.

nikomatsakis · 2017-01-19T07:28:49Z

👍 to shorter wording on the "primary label". In fact, though I think this is a separate thing, I have long wanted to aggressively shorten the borrowck messages, e.g. removing the phrase "borrowed as mutable" and just say "write" or "mutate", and definitely not talking about distracting but complex things like "immutable borrowed content".

Also 👍 to this general pull request. =)

estebank · 2017-01-20T04:31:25Z

@nikomatsakis is there anything else that should be part of this PR?

nikomatsakis

Looks like it's close but not quite there. We should make some tests targeting this specific case (the existing tests seem to have caught this overenthusiastic error message "by accident", no?)

nikomatsakis · 2017-01-23T21:16:41Z

So...this fn is pretty hard to read. =) Maybe we can use some temporaries or something? I just find it hard to follow, but maybe it's ok, if there was at least a comment. Basically when I read it I just see "walk a bunch of data structures to pull out data", but my eyes slide over the details...

This is moved code from bellow so that I could use it twice without copying code around :)
I will see how I can make it easier to follow.

nikomatsakis · 2017-01-23T21:17:12Z

nikomatsakis · 2017-01-23T21:17:49Z

maybe we can adjust the wording to make it clearer that BOTH parts need to be &mut self? That's a bit tricky, particularly since we don't control the relative order of things here

I could look at the span's line number to identify order and change this to something like
error: cannot borrow immutable borrowed content `*self.s` as mutable --> $DIR/issue-38147-3.rs:17:9 | 12 | s: &'a String | ------------- this field should be `&mut`... ... 16 | fn f(&self) { | ----- ...and use `&mut self` here to make mutable 17 | self.s.push('x'); | ^^^^^^ cannot borrow as mutable
But I don't think it's unreasonable to show both labels, and if the developer only changes one of them, to still get the same error with the remaining label.

nikomatsakis · 2017-01-23T21:18:30Z

Hmm, this seems wrong, no?

Certainly there is nothing that would easily be converted to &mut here

What should be the appropriate message for this case? Nothing if the type is not a ref?

nikomatsakis · 2017-01-23T21:27:52Z

seems like we have to check here that the type of the field is some sort of reference, right? (hence the "over fire" case in the ui tests below)

estebank

We should make some tests targeting this specific case

issue-38147-1, issue-38147-2, issue-38147-3 & issue-38147-4 are new tests targeting this case.

estebank · 2017-01-23T21:38:04Z

What should be the appropriate message for this case? Nothing if the type is not a ref?

estebank · 2017-01-23T21:43:08Z

This is moved code from bellow so that I could use it twice without copying code around :)
I will see how I can make it easier to follow.

estebank · 2017-01-23T21:46:59Z

I could look at the span's line number to identify order and change this to something like
error: cannot borrow immutable borrowed content `*self.s` as mutable --> $DIR/issue-38147-3.rs:17:9 | 12 | s: &'a String | ------------- this field should be `&mut`... ... 16 | fn f(&self) { | ----- ...and use `&mut self` here to make mutable 17 | self.s.push('x'); | ^^^^^^ cannot borrow as mutable
But I don't think it's unreasonable to show both labels, and if the developer only changes one of them, to still get the same error with the remaining label.

nikomatsakis · 2017-01-23T21:55:26Z

@estebank

What should be the appropriate message for this case? Nothing if the type is not a ref?

Yeah, no change is needed in this case.

nikomatsakis · 2017-01-25T16:32:50Z

@estebank

Travis is failing because a UI test needs to be updated:

error: cannot borrow immutable borrowed content `*self.s` as mutable
--> /checkout/src/test/ui/did_you_mean/issue-38147-3.rs:17:9
|
12 | s: &'a String
| ------------- use `&'a mut String` here to make mutable
...
16 | fn f(&self) {
| ----- use `&mut self` here to make mutable
17 | self.s.push('x');
| ^^^^^^ cannot borrow as mutable
error: aborting due to previous error

nikomatsakis · 2017-01-25T16:32:57Z

but the new output looks correct =)

estebank · 2017-01-25T19:50:15Z

@nikomatsakis tests are fixed. Can you check wether the method immutable_argument_should_be_mut is now more readable (along with the new method suggest_mut_for_immutable )?

Also, please verify that using bug! for a case that shouldn't ever happen is correct. If it is not, it can be changed to return a generic comment use '&mut' to make mutable.

bors · 2017-01-26T17:47:01Z

☔ The latest upstream changes (presumably #39309) made this pull request unmergeable. Please resolve the merge conflicts.

Point to immutable borrow arguments and fields when trying to use them as mutable borrows. Add label to primary span on "cannot borrow as mutable" errors. Present the following output when trying to access an immutable borrow's field as mutable: ``` error[E0389]: cannot borrow data mutably in a `&` reference --> $DIR/issue-38147-1.rs:27:9 | 26 | fn f(&self) { | ----- use `&mut self` here to make mutable 27 | f.s.push('x'); | ^^^ assignment into an immutable reference ``` And the following when trying to access an immutable struct field as mutable: ``` error: cannot borrow immutable borrowed content `*self.s` as mutable --> $DIR/issue-38147-3.rs:17:9 | 12 | s: &'a String | ------------- use `&'a mut String` here to make mutable ...| 16 | fn f(&self) { | ----- use `&mut self` here to make mutable 17 | self.s.push('x'); | ^^^^^^ cannot borrow as mutable ```

nikomatsakis · 2017-01-26T20:21:24Z

@estebank

Can you check wether the method immutable_argument_should_be_mut is now more readable (along with the new method suggest_mut_for_immutable)?

Yes; it's a wonder what some comments will do! :) Thanks.

Also, please verify that using bug! for a case that shouldn't ever happen is correct. If it is not, it can be changed to return a generic comment use '&mut' to make mutable.

Yes that is correct.

nikomatsakis · 2017-01-26T20:21:26Z

@bors r+

bors · 2017-01-26T20:21:28Z

📌 Commit e1280d8 has been approved by nikomatsakis

bors · 2017-01-26T20:33:02Z

⌛ Testing commit e1280d8 with merge f453929...

bors · 2017-01-26T21:47:24Z

💔 Test failed - status-travis

alexcrichton · 2017-01-26T22:34:08Z

@bors: retry * osx linker segfaulted #38878

…

On Thu, Jan 26, 2017 at 1:47 PM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/195662190> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#39139 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95NgcEoXZgz9hNDlNtfdjiKWfXLNQks5rWRRugaJpZM4LmO9M> .

bors · 2017-01-26T23:26:32Z

⌛ Testing commit e1280d8 with merge 4805ad6...

bors · 2017-01-27T00:39:06Z

💔 Test failed - status-travis

alexcrichton · 2017-01-27T02:43:19Z

@bors: retry * osx linker segfaulted #38878

…

On Thu, Jan 26, 2017 at 4:39 PM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/195714707> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#39139 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95D-rx6ugjwGc_a0vzHqZ5J0HI529ks5rWTyugaJpZM4LmO9M> .

bors · 2017-01-27T04:17:07Z

⌛ Testing commit e1280d8 with merge 9876cb0...

bors · 2017-01-27T04:49:59Z

💔 Test failed - status-travis

alexcrichton · 2017-01-27T04:51:02Z

@bors: retry * presumed bug in travis

…

On Thu, Jan 26, 2017 at 8:50 PM, bors ***@***.***> wrote: 💔 Test failed - status-travis <https://travis-ci.org/rust-lang/rust/builds/195760346> — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#39139 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD95F0qzRFshg7CladBntz0ewYL9Dujks5rWXd6gaJpZM4LmO9M> .

bors · 2017-01-27T04:57:17Z

⌛ Testing commit e1280d8 with merge 025fb7d...

Point to immutable arg/fields when trying to use as &mut Present the following output when trying to access an immutable borrow's field as mutable: ``` error[E0389]: cannot borrow data mutably in a `&` reference --> $DIR/issue-38147-1.rs:27:9 | 26 | fn f(&self) { | ----- use `&mut self` here to make mutable 27 | f.s.push('x'); | ^^^ assignment into an immutable reference ``` And the following when trying to access an immutable struct field as mutable: ``` error: cannot borrow immutable borrowed content `*self.s` as mutable --> $DIR/issue-38147-3.rs:17:9 | 12 | s: &'a String | ------------- use `&'a mut String` here to make mutable ...| 16 | fn f(&self) { | ----- use `&mut self` here to make mutable 17 | self.s.push('x'); | ^^^^^^ cannot borrow as mutable ``` Fixes#38147.

bors · 2017-01-27T07:28:57Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 025fb7d to master...

arielb1 · 2017-02-06T21:36:32Z

This causes the ICE #39544.

rust-highfive assigned nrc Jan 17, 2017

GuillaumeGomez reviewed Jan 17, 2017
View reviewed changes

rust-highfive assigned nikomatsakis and unassigned nrc Jan 18, 2017

estebank force-pushed the issue-38147 branch 3 times, most recently from 0df4676 to a9a0f48Compare January 19, 2017 05:50

nikomatsakis requested changes Jan 23, 2017
View reviewed changes

estebank commented Jan 23, 2017
View reviewed changes

estebank force-pushed the issue-38147 branch from 1d1160f to 7f5097eCompare January 25, 2017 18:28

estebank force-pushed the issue-38147 branch from 7f5097e to e1280d8Compare January 26, 2017 19:01

nikomatsakis approved these changes Jan 26, 2017
View reviewed changes

bors mentioned this pull request Jan 27, 2017
Refactor stdlib for pal #39325
Closed

bors merged commit e1280d8 into rust-lang:masterJan 27, 2017

bors mentioned this pull request Jan 27, 2017
Perform lifetime elision (more) syntactically, before type-checking. #39305
Merged

codyps mentioned this pull request Feb 5, 2017
ICE on nightly when calling method on an enum #39544
Closed

Mark-Simulacrum mentioned this pull request Feb 8, 2017
Error message for mutating field of immutable struct could be clearer #27593
Closed

estebank mentioned this pull request Mar 12, 2017
Point to let when modifying field of immutable variable #40445
Merged

estebank deleted the issue-38147 branch November 9, 2023 05:26

Uh oh!

Uh oh!

Conversation

estebank commented Jan 17, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jan 17, 2017

Uh oh!

estebank commented Jan 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomez commented Jan 17, 2017

Uh oh!

nrc commented Jan 18, 2017

Uh oh!

estebank commented Jan 18, 2017

Uh oh!

estebank commented Jan 18, 2017

Uh oh!

sophiajt commented Jan 19, 2017

Uh oh!

estebank commented Jan 19, 2017

Uh oh!

sophiajt commented Jan 19, 2017

Uh oh!

estebank commented Jan 19, 2017

Uh oh!

nikomatsakis commented Jan 19, 2017

Uh oh!

estebank commented Jan 20, 2017

Uh oh!

nikomatsakis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estebank left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikomatsakis commented Jan 23, 2017

Uh oh!

nikomatsakis commented Jan 25, 2017

Uh oh!

nikomatsakis commented Jan 25, 2017

Uh oh!

estebank commented Jan 25, 2017

Uh oh!

bors commented Jan 26, 2017

Uh oh!

nikomatsakis commented Jan 26, 2017

Uh oh!

nikomatsakis commented Jan 26, 2017

Uh oh!

bors commented Jan 26, 2017

Uh oh!

estebank commented Jan 17, 2017•
edited
Loading