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.

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

Point to let when modifying field of immutable variable #40445

Merged
merged 3 commits into from
Mar 19, 2017

Conversation

estebank
Copy link
Contributor

Point at the immutable local variable when trying to modify one of its
fields.

Given a file:

struct Foo {
    pub v: Vec<String>
}

fn main() {
    let f = Foo { v: Vec::new() };
    f.v.push("cat".to_string());
}

present the following output:

error: cannot borrow immutable field `f.v` as mutable
 --> file.rs:7:13
  |
6 |    let f = Foo { v: Vec::new() };
  |        - this should be `mut`
7 |    f.v.push("cat".to_string());
  |    ^^^

error: aborting due to previous error

Fix #27593.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@eddyb
Copy link
Member

eddyb commented Mar 11, 2017

r? @jonathandturner

@rust-highfive rust-highfive assigned sophiajt and unassigned eddyb Mar 11, 2017
Point at the immutable local variable when trying to modify one of its
fields.

Given a file:

```rust
struct Foo {
    pub v: Vec<String>
}

fn main() {
    let f = Foo { v: Vec::new() };
    f.v.push("cat".to_string());
}
```

present the following output:

```
error: cannot borrow immutable field `f.v` as mutable
 --> file.rs:7:13
  |
6 |    let f = Foo { v: Vec::new() };
  |        - this should be `mut`
7 |    f.v.push("cat".to_string());
  |    ^^^

error: aborting due to previous error
```
@Emilgardis
Copy link
Contributor

I think this should read

error: cannot borrow immutable field `f.v` as mutable
 --> file.rs:7:13
  |
6 |    let f = Foo { v: Vec::new() };
  |       ^ - consider adding `mut` here
7 |    f.v.push("cat".to_string());
  |    ^^^

or

consider changing this to `mut f`

@nagisa
Copy link
Member

nagisa commented Mar 12, 2017

@Emilgardis I disagree, because with complex patterns there’s multiple ways to make some binding mutable.

IME it should read simply, without suggesting any specific syntax.

6 |    let f = Foo { v: Vec::new() };
  |        - consider making this binding mutable

@nagisa
Copy link
Member

nagisa commented Mar 12, 2017

Please also add a test for argument binding.

@sophiajt
Copy link
Contributor

@nagisa

6 |    let f = Foo { v: Vec::new() };
  |        - consider making this binding mutable

This is a useful suggestion to more experienced Rust programmers, but one of the nice things about making a suggestion is that the errors can help teach you the language. By teaching them about the mut keyword, they have something they can do to fix the error. Your suggestion has the unfortunate side effect of saying "go read the manual"

@sophiajt
Copy link
Contributor

consider changing this to `mut f`

👍

@estebank
Copy link
Contributor Author

Please also add a test for argument binding.

We already do that since #39139, and it isn't affected by this change because here it's checking wether it is an argument to only add this message if it isn't.

This is a useful suggestion to more experienced Rust programmers, but one of the nice things about making a suggestion is that the errors can help teach you the language. By teaching them about the mut keyword, they have something they can do to fix the error. Your suggestion has the unfortunate side effect of saying "go read the manual"

I have to agree with these points. I'll change it, as proposed, to

error: cannot borrow immutable field `f.v` as mutable
 --> file.rs:7:13
  |
6 |    let f = Foo { v: Vec::new() };
  |        - consider changing this to `mut f`
7 |    f.v.push("cat".to_string());
  |    ^^^

@Emilgardis
Copy link
Contributor

I agree with these changes.

Why did travis fail on only one suite?

@sophiajt
Copy link
Contributor

error: cannot borrow immutable field `f.v` as mutable
 --> file.rs:7:13
  |
6 |    let f = Foo { v: Vec::new() };
  |        - consider changing this to `mut f`
7 |    f.v.push("cat".to_string());
  |    ^^^

The ^^^ needs a label.

Change the wording of mutable borrow on immutable binding from "this
should be `mut`" to "consider changing this to `mut f`".
@estebank estebank force-pushed the issue-18150 branch 2 times, most recently from e339c17 to 90e69d6 Compare March 13, 2017 04:35
@estebank
Copy link
Contributor Author

@jonathandturner does the following wording make sense?

error: cannot borrow immutable field `z.x` as mutable
  --> $DIR/issue-39544.rs:21:18
   |
20 |     let z = Z { x: X::Y };
   |         - consider changing this to `mut z`
21 |     let _ = &mut z.x;
   |                  ^^^ cannot borrow mutably field of immutable binding

@sophiajt
Copy link
Contributor

Seems close. How about...

error: cannot borrow immutable field `z.x` as mutable
  --> $DIR/issue-39544.rs:21:18
   |
20 |     let z = Z { x: X::Y };
   |         - consider changing this to `mut z`
21 |     let _ = &mut z.x;
   |                  ^^^ cannot mutably borrow immutable field

@estebank
Copy link
Contributor Author

@jonathandturner LGTM.

Done.

@estebank
Copy link
Contributor Author

@jonathandturner ping.

@sophiajt
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 16, 2017

📌 Commit 9ac628d has been approved by jonathandturner

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 18, 2017
…rner

Point to let when modifying field of immutable variable

Point at the immutable local variable when trying to modify one of its
fields.

Given a file:

```rust
struct Foo {
    pub v: Vec<String>
}

fn main() {
    let f = Foo { v: Vec::new() };
    f.v.push("cat".to_string());
}
```

present the following output:

```
error: cannot borrow immutable field `f.v` as mutable
 --> file.rs:7:13
  |
6 |    let f = Foo { v: Vec::new() };
  |        - this should be `mut`
7 |    f.v.push("cat".to_string());
  |    ^^^

error: aborting due to previous error
```

Fix rust-lang#27593.
arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 18, 2017
…rner

Point to let when modifying field of immutable variable

Point at the immutable local variable when trying to modify one of its
fields.

Given a file:

```rust
struct Foo {
    pub v: Vec<String>
}

fn main() {
    let f = Foo { v: Vec::new() };
    f.v.push("cat".to_string());
}
```

present the following output:

```
error: cannot borrow immutable field `f.v` as mutable
 --> file.rs:7:13
  |
6 |    let f = Foo { v: Vec::new() };
  |        - this should be `mut`
7 |    f.v.push("cat".to_string());
  |    ^^^

error: aborting due to previous error
```

Fix rust-lang#27593.
bors added a commit that referenced this pull request Mar 18, 2017
arielb1 pushed a commit to arielb1/rust that referenced this pull request Mar 19, 2017
…rner

Point to let when modifying field of immutable variable

Point at the immutable local variable when trying to modify one of its
fields.

Given a file:

```rust
struct Foo {
    pub v: Vec<String>
}

fn main() {
    let f = Foo { v: Vec::new() };
    f.v.push("cat".to_string());
}
```

present the following output:

```
error: cannot borrow immutable field `f.v` as mutable
 --> file.rs:7:13
  |
6 |    let f = Foo { v: Vec::new() };
  |        - this should be `mut`
7 |    f.v.push("cat".to_string());
  |    ^^^

error: aborting due to previous error
```

Fix rust-lang#27593.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 19, 2017
…rner

Point to let when modifying field of immutable variable

Point at the immutable local variable when trying to modify one of its
fields.

Given a file:

```rust
struct Foo {
    pub v: Vec<String>
}

fn main() {
    let f = Foo { v: Vec::new() };
    f.v.push("cat".to_string());
}
```

present the following output:

```
error: cannot borrow immutable field `f.v` as mutable
 --> file.rs:7:13
  |
6 |    let f = Foo { v: Vec::new() };
  |        - this should be `mut`
7 |    f.v.push("cat".to_string());
  |    ^^^

error: aborting due to previous error
```

Fix rust-lang#27593.
bors added a commit that referenced this pull request Mar 19, 2017
Rollup of 13 pull requests

- Successful merges: #40441, #40445, #40562, #40564, #40583, #40588, #40589, #40590, #40603, #40611, #40621, #40646, #40648
- Failed merges:
@bors bors merged commit 9ac628d into rust-lang:master Mar 19, 2017
estebank added a commit to estebank/rust that referenced this pull request Mar 24, 2017
Given a file

```rust
struct S {
    x: i32,
}
fn foo() {
    let s = S { x: 42 };
    s.x += 1;
}
fn bar(s: S) {
    s.x += 1;
}
```

Provide the following output:

```rust
error: cannot assign to immutable field `s.x`
 --> $DIR/issue-35937.rs:16:5
  |
5 |     let s = S { x: 42 };
  |         - consider changing this to `mut s`
6 |     s.x += 1;
  |     ^^^^^^^^ cannot mutably borrow immutable field

error: cannot assign to immutable field `s.x`
 --> $DIR/issue-35937.rs:20:5
  |
8 | fn bar(s: S) {
  |        - consider changing this to `mut s`
9 |     s.x += 1;
  |     ^^^^^^^^ cannot mutably borrow immutable field
```

Follow up to rust-lang#40445. Fix rust-lang#35937.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants