Disable ref hint for pattern in let and adding ui tests #40402 #41564

gaurikholkar-zz · 2017-04-26T21:09:32Z

A fix to #40402

The to prevent move, use ref e or ref mut e has been disabled.

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

now gives

error[E0507]: cannot move out of indexed content
 --> example.rs:4:13
  |
4 |     let e = v[0];
  |             ^^^^ cannot move out of indexed content

error: aborting due to previous error

I have added ui tests for the same and also modified a compile-fail test.

rust-highfive · 2017-04-26T21:09:38Z

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

nikomatsakis · 2017-04-26T21:39:17Z

r? @nikomatsakis

nikomatsakis

Looks great @gaurikholkar! I have a few nits and style suggestions to be addressed.

nikomatsakis · 2017-04-26T21:44:50Z

src/librustc/hir/map/mod.rs

@@ -95,6 +95,14 @@ enum MapEntry<'hir> {
    RootCrate,
 }

+/// Represents the kind of pattern
+#[derive(Debug, Clone, Copy)]
+pub enum PatternSource<'hir> {


Can you move this struct into gather_move.rs as well?

nikomatsakis · 2017-04-26T21:47:23Z

src/librustc_borrowck/borrowck/gather_loans/gather_moves.rs

+    span_path_opt: Option<MoveSpanAndPath<'tcx>>
+}
+
+/// Returns the kind of the Pattern


I think a slightly bigger comment would be good. Something like:

/// Analyzes the context where the pattern appears to determine what /// sort of hint we want to give. In particular, if the pattern is in a `match` /// or nested within other patterns, we want to suggest a `ref` binding: /// /// let (a, b) = v[0]; // like the `a` and `b` patterns here /// match v[0] { a => ... } // or the `a` pattern here /// /// But if the pattern is the outermost pattern in a `let`, we would rather /// suggest that the author add a `&` to the initializer: /// /// let x = v[0]; // suggest `&v[0]` here /// /// In this latter case, this function will return `PatternSource::LetDecl` /// with a reference to the let.

nikomatsakis · 2017-04-26T21:48:19Z

src/librustc_borrowck/borrowck/gather_loans/move_error.rs

-                                  move_to.name, is_first_note);
-            is_first_note = false;
+
+	if let Some(pattern_source) = error.move_to_places.get(0){


Nit: the indentation is funny here. The if should be aligned with the let above, and I don't think you need the blank lines here. Also, the final brace should have a space (i.e., replace get(0){ with get(0) {).

Actually, I think I'd just remove the if let and merge it with the match. So something like this:

match error.move_to_places.get(0) { Some(MoveSpanAndPath { pat_source: PatternSource::LetDecl(_), .. }) => { // ignore patterns that are found at the top-level of a `let`; // see `get_pattern_source()` for details } _ => { for move_to in &error.move_to_places { err = note_move_destination(err, move_to.span, move_to.name, is_first_note); is_first_note = false; } } }

This might read nicer still if we changed the name of the MoveSpanAndPath struct into something a bit shorter, like Place or MovePlace.

nikomatsakis · 2017-04-26T21:52:33Z

src/test/ui/issue-40402-ref-hints/issue-40402-1.rs

+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+


It'd be good to put a comment at the top of this file explaining the purpose of the test:

// Check that we do not suggest `ref f` here in the `main()` function.

nikomatsakis · 2017-04-26T21:52:55Z

src/test/ui/issue-40402-ref-hints/issue-40402-2.rs

+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+


Same for this test:

// Check that we do suggest `(ref a, ref b)` here, since `a` and `b` // are nested within a pattern.

Will do the necessary changes

oli-obk · 2017-04-27T07:19:08Z

src/librustc_borrowck/borrowck/gather_loans/move_error.rs

+	if let Some(pattern_source) = error.move_to_places.get(0){
+
+        match pattern_source.pat_source {
+            PatternSource::LetDecl(_) => {}


If you store the expression on the rhs of the assignment in the LetDecl variant, then you can suggest to add an ampersand before the expression.

@oli-obk yeah, we are planning for a follow-up PR

nikomatsakis · 2017-04-27T19:08:52Z

@gaurikholkar let me know when you have updates :)

bors · 2017-04-28T03:19:59Z

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

gaurikholkar-zz · 2017-04-28T10:43:18Z

@nikomatsakis Updated the PR, @bors resolved the merge conflicts for #41591

nikomatsakis · 2017-04-28T18:27:41Z

@bors r+

bors · 2017-04-28T18:27:42Z

📌 Commit ab5d16a has been approved by nikomatsakis

bors · 2017-04-29T02:28:09Z

⌛ Testing commit ab5d16a with merge a618d5b...

bors · 2017-04-29T03:56:08Z

💔 Test failed - status-travis

Mark-Simulacrum · 2017-04-29T04:09:09Z

@bors retry

crates.io responded with 500 instead of 200 when downloading crate #41545

bors · 2017-04-29T06:16:28Z

⌛ Testing commit ab5d16a with merge b9e9a03...

Disable ref hint for pattern in let and adding ui tests #40402 A fix to #40402 The `to prevent move, use ref e or ref mut e ` has been disabled. ``` fn main() { let v = vec![String::from("oh no")]; let e = v[0]; } ``` now gives ``` error[E0507]: cannot move out of indexed content --> example.rs:4:13 | 4 | let e = v[0]; | ^^^^ cannot move out of indexed content error: aborting due to previous error ``` I have added ui tests for the same and also modified a compile-fail test.

bors · 2017-04-29T08:36:53Z

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

@nikomatsakis

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

rust-highfive assigned arielb1 Apr 26, 2017

gaurikholkar-zz mentioned this pull request Apr 26, 2017

Don't suggest ref for let bindings. #40402

Closed

gaurikholkar-zz closed this Apr 26, 2017

gaurikholkar-zz reopened this Apr 26, 2017

rust-highfive assigned nikomatsakis and unassigned arielb1 Apr 26, 2017

nikomatsakis requested changes Apr 26, 2017

View reviewed changes

gaurikholkar-zz changed the title ~~Disable ref hint for pattern in let and adding ui tests.~~ Disable ref hint for pattern in let and adding ui tests #40402 Apr 26, 2017

oli-obk reviewed Apr 27, 2017

View reviewed changes

nikomatsakis added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 27, 2017

Disable ref hint for pattern in let and adding ui-tests.

5436f85

gaurikholkar-zz force-pushed the master branch from 0680769 to 1e093a1 Compare April 28, 2017 10:39

Adding documentation, indentation fixes

ab5d16a

gaurikholkar-zz force-pushed the master branch from 1e093a1 to ab5d16a Compare April 28, 2017 10:59

nikomatsakis approved these changes Apr 28, 2017

View reviewed changes

frewsxcv mentioned this pull request Apr 28, 2017

Rollup of 6 pull requests #41614

Closed

bors merged commit ab5d16a into rust-lang:master Apr 29, 2017

gaurikholkar-zz mentioned this pull request Apr 29, 2017

Consider changing to & for let bindings #40402 #41640

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable ref hint for pattern in let and adding ui tests #40402 #41564

Disable ref hint for pattern in let and adding ui tests #40402 #41564

gaurikholkar-zz commented Apr 26, 2017 •

edited

Loading

rust-highfive commented Apr 26, 2017

nikomatsakis commented Apr 26, 2017

nikomatsakis left a comment

nikomatsakis Apr 26, 2017

nikomatsakis Apr 26, 2017

nikomatsakis Apr 26, 2017

nikomatsakis Apr 26, 2017

nikomatsakis Apr 26, 2017

nikomatsakis Apr 26, 2017

gaurikholkar-zz Apr 26, 2017

oli-obk Apr 27, 2017

nikomatsakis Apr 27, 2017

nikomatsakis commented Apr 27, 2017

bors commented Apr 28, 2017

gaurikholkar-zz commented Apr 28, 2017

nikomatsakis commented Apr 28, 2017

bors commented Apr 28, 2017

bors commented Apr 29, 2017

bors commented Apr 29, 2017

Mark-Simulacrum commented Apr 29, 2017

bors commented Apr 29, 2017

bors commented Apr 29, 2017

Disable ref hint for pattern in let and adding ui tests #40402 #41564

Disable ref hint for pattern in let and adding ui tests #40402 #41564

Conversation

gaurikholkar-zz commented Apr 26, 2017 • edited Loading

rust-highfive commented Apr 26, 2017

nikomatsakis commented Apr 26, 2017

nikomatsakis left a 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

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

Choose a reason for hiding this comment

nikomatsakis commented Apr 27, 2017

bors commented Apr 28, 2017

gaurikholkar-zz commented Apr 28, 2017

nikomatsakis commented Apr 28, 2017

bors commented Apr 28, 2017

bors commented Apr 29, 2017

bors commented Apr 29, 2017

Mark-Simulacrum commented Apr 29, 2017

bors commented Apr 29, 2017

bors commented Apr 29, 2017

gaurikholkar-zz commented Apr 26, 2017 •

edited

Loading