Be more obvious when suggesting dereference by estebank · Pull Request #45947 · rust-lang/rust

estebank · 2017-11-12T22:19:39Z

Include & span when suggesting dereference on a span that is already a reference:

error: non-reference pattern used to match a reference (see issue #42640)
--> dont-suggest-dereference-on-arg.rs:16:19
|
16 | .filter(|&(ref a, _)| foo(a))
| ^^^^^^^^^^^ help: consider using: `&&(ref k, _)`
|
= help: add #![feature(match_default_bindings)] to the crate attributes to enable

Fix#45925.

mark-i-m · 2017-11-12T23:55:48Z

I'm still not quite sure I understand the error... Isn't &(ref k, _) a reference pattern used to match a reference? What does it mean by "non-reference pattern"?

estebank · 2017-11-13T00:36:44Z

This PR is not a full solution, do not merge.

@mark-i-m the type checker sees the closure's argument as a pattern match, I believed that behavior was introduced as part of RFC 2005 (tracked in #42640).

mark-i-m · 2017-11-13T00:41:37Z

It is a pattern match, though, right? I was under the impression that in Rust all arguments to functions and closures are actually patterns, which can bind new names, just as in an if let or match. Is this not true?

Another way of asking the question is to ask if &(ref k, _) is actually wrong. I believe the types are correct, and I don't think I was violating the borrow rules or anything. So IIUC I should not have gotten any error at all; it should have just compiled, right?

mark-i-m · 2017-11-13T00:48:52Z

Ah, I see. The correct pattern would be &&(ref k, _) but the "type mismatch" error is covered over by the "match bindings" error... So the code should not have compiled...

mark-i-m · 2017-11-13T00:52:49Z

Hmm... so the hint was actually on the right track. It suggests replacing (ref k, _) with &(ref k, _) in &(ref k, _), which would have lead to &&(ref k, _). It is just not at all clear that's what it means...

arielb1 · 2017-11-13T12:18:59Z

@estebank

Can't we just adjust our error message until it includes all the surrounding reference patterns, so we'll get this:

error: non-reference pattern used to match a reference(see issue #42640)
--> src/coord.rs:194:23
|
194 | .filter(|&(ref k, _)| self.hash(k) == server)
| ^^^^^^^^^^^ help: consider using: `&&(ref k, _)`
|
= help: add #![feature(match_default_bindings)] to the crate attributes to enable
error: aborting due to previous error
error:Could not compile `kv_2pc`.To learn more, run the command again with --verbose.

estebank · 2017-11-13T23:47:18Z

@arielb1 yeah, I think that's a better approach.

kennytm · 2017-11-22T10:48:00Z

Hi @estebank, what is the status of this PR?

estebank · 2017-11-23T03:03:48Z

@kennytm @arielb1 I've modified the PR to do some span fiddling to see if there's a & right before. I'm not entirely happy with doing that, but it should be enough to handle the presented situation.

mark-i-m · 2017-11-23T03:44:53Z

@estebank

I'm not entirely happy with doing that

What problems do you foresee with it?

mark-i-m · 2017-11-23T03:48:38Z

Doesn't this mean we now have the same problem with the following case:
let x = &&&0;let&&x = x;
IIUC, the error message would suggest using &&, right?

arielb1 · 2017-11-23T12:28:25Z

This looks likely to introduce an infinite loop in some case, where we go up to the start of the file. You can return an Option<Span> instead.

arielb1 · 2017-11-23T12:29:31Z

@estebank

Please don't fiddle with spans, it causes very weird situations with macros. You can use tcx.hir.get_parent_node to go up the pattern tree and find the reference patterns until you find something that is not a reference pattern, and then you span the last reference pattern.

arielb1 · 2017-11-23T20:54:52Z

nit: s/loop/while let/

ok a while-let would be ugly too. Why can't we have a do-while loop?

arielb1 · 2017-11-23T20:56:25Z

@bors r+

bors · 2017-11-23T20:56:25Z

📌 Commit f1a672c has been approved by arielb1

kennytm · 2017-11-24T05:13:08Z

@bors r-

CI failed, the UI test ui/suggestions/dont-suggest-dereference-on-arg.rs’s output needs to be updated.

bors · 2017-11-24T15:14:13Z

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

Include enclosing span when suggesting dereference on a span that is already a reference: ``` error: non-reference pattern used to match a reference (see issue rust-lang#42640) --> dont-suggest-dereference-on-arg.rs:16:19 | 16 | .filter(|&(ref a, _)| foo(a)) | ^^^^^^^^^^^ help: consider using: `&&(ref k, _)` | = help: add #![feature(match_default_bindings)] to the crate attributes to enable ```

kennytm · 2017-11-25T17:02:22Z

[00:47:09] error: /checkout/src/test/ui/suggestions/dont-suggest-dereference-on-arg.rs:16: unexpected help message: '16:18: 16:29: add #![feature(match_default_bindings)] to the crate attributes to enable'
Needs one more //~| HELP.

estebank · 2017-11-25T20:19:02Z

@bors r=arielb1

bors · 2017-11-25T20:19:03Z

📌 Commit 15dfd7e has been approved by arielb1

…ielb1 Be more obvious when suggesting dereference Include `&` span when suggesting dereference on a span that is already a reference: ``` error: non-reference pattern used to match a reference (see issue #42640) --> dont-suggest-dereference-on-arg.rs:16:19 | 16 | .filter(|&(ref a, _)| foo(a)) | ^^^^^^^^^^^ help: consider using: `&&(ref k, _)` | = help: add #![feature(match_default_bindings)] to the crate attributes to enable ``` Fix#45925.

bors · 2017-11-26T01:06:02Z

⌛ Testing commit 15dfd7e with merge 693bb0d...

bors · 2017-11-26T03:30:46Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 693bb0d to master...

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

estebank force-pushed the match_default_bindings-arg-hint branch 3 times, most recently from 8b8dbaa to 8ca6925Compare November 23, 2017 03:01

estebank changed the title ~~Don't suggest dereference on tuple argument~~Be more obvious when suggesting dereferenceNov 23, 2017

mark-i-m reviewed Nov 23, 2017
View reviewed changes

arielb1 reviewed Nov 23, 2017
View reviewed changes

estebank force-pushed the match_default_bindings-arg-hint branch 2 times, most recently from b28b38e to f1a672cCompare November 23, 2017 20:29

arielb1 reviewed Nov 23, 2017
View reviewed changes

estebank added 2 commits November 24, 2017 08:26

Use get_parent_node instead of using spans
2461b7a

estebank force-pushed the match_default_bindings-arg-hint branch 3 times, most recently from 33f0684 to 48d291aCompare November 25, 2017 15:52

kennytm reviewed Nov 25, 2017
View reviewed changes

Fix test
15dfd7e

estebank force-pushed the match_default_bindings-arg-hint branch from 48d291a to 15dfd7eCompare November 25, 2017 18:16

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 25, 2017

bors merged commit 15dfd7e into rust-lang:masterNov 26, 2017

estebank deleted the match_default_bindings-arg-hint branch November 9, 2023 05:25

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

mark-i-m commented Nov 12, 2017

Uh oh!

estebank commented Nov 13, 2017

Uh oh!

mark-i-m commented Nov 13, 2017

Uh oh!

mark-i-m commented Nov 13, 2017

Uh oh!

mark-i-m commented Nov 13, 2017

Uh oh!

arielb1 commented Nov 13, 2017

Uh oh!

estebank commented Nov 13, 2017

Uh oh!

kennytm commented Nov 22, 2017

Uh oh!

estebank commented Nov 23, 2017

Uh oh!

mark-i-m commented Nov 23, 2017

Uh oh!

mark-i-mNov 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1Nov 23, 2017

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Nov 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arielb1Nov 23, 2017

Choose a reason for hiding this comment

Uh oh!

arielb1Nov 23, 2017

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Nov 23, 2017

Uh oh!

bors commented Nov 23, 2017

Uh oh!

kennytm commented Nov 24, 2017

Uh oh!

bors commented Nov 24, 2017

Uh oh!

kennytmNov 25, 2017

Choose a reason for hiding this comment

Uh oh!

estebank commented Nov 25, 2017

Uh oh!

bors commented Nov 25, 2017

Uh oh!

bors commented Nov 26, 2017

Uh oh!

bors commented Nov 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

estebank commented Nov 12, 2017•
edited
Loading

mark-i-mNov 23, 2017•
edited
Loading

arielb1 commented Nov 23, 2017•
edited
Loading