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

Consider changing to & for let bindings #40402 #41640

Merged
merged 2 commits into from
May 3, 2017

Conversation

gaurikholkar-zz
Copy link

@gaurikholkar-zz gaurikholkar-zz commented Apr 29, 2017

This is a fix for #40402

For the example

fn main() {
    let v = vec![String::from("oh no")];
    
    let e = v[0];
}

It gives

error[E0507]: cannot move out of indexed content
 --> ex1.rs:4:13
  |
4 |     let e = v[0];
  |             ^^^^ cannot move out of indexed content
  |
  = help: consider changing to `&v[0]`

error: aborting due to previous error

Another alternative is

error[E0507]: cannot move out of indexed content
 --> ex1.rs:4:13
  |
4 |     let e = v[0];
  |             ^^^^ consider changing to `&v[0]`

error: aborting due to previous error

Also refer to #41564 for more details.

r? @nikomatsakis

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

@gaurikholkar-zz gaurikholkar-zz changed the title Consider changing to & for let bindings Consider changing to & for let bindings #40402 Apr 29, 2017
@gaurikholkar-zz
Copy link
Author

@estebank @jonathandturner please review the format.

@nikomatsakis
Copy link
Contributor

please review the format.

Specifically, as @gaurikholkar said, she and I were wondering what you guys thought might be best. Because the span to label for the suggestion is the same as the one for which we are reporting the error, the initial stab looked something like:

error[E0507]: cannot move out of indexed content
 --> ex1.rs:4:13
  |
4 |     let e = v[0];
  |             ^^^^
  |             |
  |             consider changing to `&v[0]`
  |             cannot move out of indexed content
  |

which didn't seem good. So we did the two other experiments you see in the main comment area. The main question is, should we repeat the "main error text" also in the label, or not?

@nikomatsakis
Copy link
Contributor

The code looks good, only question is how to format the error.

@estebank
Copy link
Contributor

What about using a span_suggestion?

error[E0507]: cannot move out of indexed content
 --> ex1.rs:4:13
  |
4 |     let e = v[0];
  |             ^^^^ cannot move out of indexed content
  |
help: consider using a reference instead:
  |     let e = &v[0];

It takes more vertical space, which I care little for, but it is the way these kind of suggestions are presented in other diagnostics.

@nikomatsakis
Copy link
Contributor

@estebank hmm, I thought we were moving away from that format for some reason. I'm not a big fan but maybe we should just use it for consistency's sake, and then we can think about upgrading how all uses of it are rendered in one step at some point.

@estebank
Copy link
Contributor

estebank commented May 1, 2017

@nikomatsakis I'd opened #39152 some time ago as a catch all to improve the suggestion output.

It was my understanding that span suggestions should be reserved for cases where certainty is high so that they can be applied automatically by tools.

@nikomatsakis
Copy link
Contributor

@estebank I've heard that line of thought. I find it hard to judge what an appropriate level of certainty is for a fictional tool -- but in general I do think people get quite frustrated when they apply a suggestion, only to find that it leads them to another error. And definitely presenting ill-formed code as a suggestion is very bad.

It seems to me that this suggestion is certainly "as good" as the ref suggestion we used to give. It will solve the immediate error, though it may not necessarily be the right move in the larger picture.

I'm trying to think if it's possible for the suggestion to be malformed. The only reason I can think for that to happen would be if () were required. However, I think that for this error to occur, the initializer must be an lvalue (a path, basically), and hence just inserting an & in front should always be valid.

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 1, 2017
@estebank
Copy link
Contributor

estebank commented May 2, 2017

@nikomatsakis I'm ok with the current output if we don't want to use span_suggestion. The help in label output doesn't look good IHMO. Also, I'm weary of having suggestions as the only the primary span in general, I get the feeling that the first line of the error is usually overlooked so I'd rather the primary span to always be a very short version of the error text.

@gaurikholkar-zz
Copy link
Author

#40851

@gaurikholkar-zz
Copy link
Author

gaurikholkar-zz commented May 2, 2017

@nikomatsakis @estebank have updated the PR.
Using span_suggestions the output is

error[E0507]: cannot move out of indexed content
 --> ex1.rs:4:13
  |
4 |     let e = v[0];
  |             ^^^^
  |             |
  |             help: consider using a reference instead `&v[0]`
  |             cannot move out of indexed content

error: aborting due to previous error

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented May 2, 2017

📌 Commit 1c57bb4 has been approved by nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this pull request May 3, 2017
Consider changing to & for let bindings rust-lang#40402

This is a fix for rust-lang#40402

For the example
```
fn main() {
    let v = vec![String::from("oh no")];

    let e = v[0];
}
```

It gives
```
error[E0507]: cannot move out of indexed content
 --> ex1.rs:4:13
  |
4 |     let e = v[0];
  |             ^^^^ cannot move out of indexed content
  |
  = help: consider changing to `&v[0]`

error: aborting due to previous error
```

Another alternative is
```
error[E0507]: cannot move out of indexed content
 --> ex1.rs:4:13
  |
4 |     let e = v[0];
  |             ^^^^ consider changing to `&v[0]`

error: aborting due to previous error
```
Also refer to rust-lang#41564 for more details.

r? @nikomatsakis
bors added a commit that referenced this pull request May 3, 2017
Rollup of 7 pull requests

- Successful merges: #41217, #41625, #41640, #41653, #41656, #41657, #41705
- Failed merges:
@bors bors merged commit 1c57bb4 into rust-lang:master May 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants