Suggest solutions for `fn foo(&foo: Foo)` #38605

estebank · 2016-12-25T19:31:38Z

For a given file:

struct Foo {}

fn foo(&foo: Foo) {}

suggest:

error[E0308]: mismatched types
 --> file.rs:3:8
  |
3 | fn foo(&foo: Foo) {}
  |        ^^^^ expected struct `Foo`, found reference
  |
  = note: expected type `Foo`
  = note:    found type `&_`
  = help: did you mean `foo: &Foo`?

Fix #38371.

rust-highfive · 2016-12-25T19:31:57Z

r? @nikomatsakis

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

bors · 2016-12-28T23:10:15Z

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

nikomatsakis · 2016-12-30T15:04:24Z

@estebank I definitely think we need suggestions here, but somehow I'm not sure this output will address the core problem, which tends to be that people don't know where to put the &. I wonder if we want something more targeted. I am imagining something like:

error[E0308]: inappropriate pattern for type
 --> file.rs:3:8
  |
3 | fn foo(&foo: Foo) {}
  |        ^^^^ `&` pattern cannot be used to match a value of type `Foo`
  |
  = help: use `foo: &Foo` to declare an argument `foo` of type `&Foo`

I realize this is a deeper change, but my goals here are:

convey to user that in fact this is a "pattern" and use the word "match" to show that is related to match
give a suggestion for what I suspect they actually wanted

I am not crazy about the precise wording of my final suggestion there. (I wonder if @wycats has any suggestions here?)

pnkfelix · 2017-01-03T20:05:55Z

Perhaps the most important point of @nikomatsakis 's comment:

in practice, someone who writes: fn f(&foo: Foo) wants

neither foo: Foo (pass by value, but clearly the developer wanted a reference somewhere),
nor &foo: &Foo (because pattern-binding of &foo will attempt to copy the dereferenced value into foo, which is probably not what was wanted either).

So I imagine a shallower change to your PR would be to make it emit:

help: did you mean `foo: &Foo`?

nikomatsakis · 2017-01-04T20:22:17Z

help: did you mean foo: &Foo?

Yeah, this is probably best

estebank · 2017-01-04T21:50:08Z

Changed to @pnkfelix's version. Next time I have some time to look at this, I will look at adding a new error code for this specific case so that --explain has a complete explanation of the differences for the three valid cases (f: F, f: &F, &f: &F), as well as the more specific wording in @nikomatsakis' suggestion.

nikomatsakis

Can you add the wacky test cases too?

nikomatsakis · 2017-01-05T17:58:54Z

src/librustc_typeck/check/_match.rs

+                                    if let Ok(snippet) = self.sess().codemap()
+                                        .span_to_snippet(pat.span)
+                                    {
+                                        err.help(&format!("did you mean `{}: &{}`?",


One concern -- what happens in more extreme cases? I think we should check that the inner pattern (represented by inner, here) is a binding pattern, so as not to trigger for something like fn foo(&&bar: &u32) or fn foo(&[bar]: &u32). Suggesting &bar: &&u32 and [bar]: &&u32 respectively doesn't seem all that helpful (that is what we will do, right?)

I tried adding these cases to the test and the results where:

fn ugh(&[bar]: &u32) { } error: slice pattern syntax is experimental (see issue #23121) --> ../../src/test/ui/mismatched_types/issue-38371.rs:26:9 | 26 | fn ugh(&[bar]: &u32) { | ^^^^^

fn agh(&&bar: &u32) { } error[E0308]: mismatched types --> ../../src/test/ui/mismatched_types/issue-38371.rs:29:9 | 29 | fn agh(&&bar: &u32) { | ^^^^ expected u32, found reference | = note: expected type `u32` = note: found type `&_`

without changes to the code.

So, the first one is a case of needing a feature-gate, but I'm surprised by the second one. What does &&bar: &Foo do? I can't tell why the help doesn't trigger here...?

I can't tell why the help doesn't trigger here...?

Neither can I. I've never even thought about trying to use something like (&&bar: &u32).

the first one is a case of needing a feature-gate

I figured as much. Last time I looked at this I couldn't find the appropriate feature gate, but just googled again and found it. I added the test back.

Maybe I'll pull your branch and build it locally and try to figure out what is going on then. I mean the tests look pretty good but I'd rather not enable misleading output if we can help it. Might take a day or two though.

nikomatsakis · 2017-01-05T17:59:53Z

src/test/ui/mismatched_types/issue-38371.stderr

+   |        ^^^^ expected struct `Foo`, found reference
+   |
+   = note: expected type `Foo`
+   = note:    found type `&_`


I don't think we have to fix this here necessarily, but I feel like these note: messages are kind of distracting from the help, which is more likely to be of use. But I'm not sure how to fix without doing potentially more harm than good, so let's leave as is.

For a while I've been thinking wether it would make sense to make the expected/found notes a first class comment, so that these errors would look closer to

error[E0308]: mismatched types --> ../../src/test/ui/mismatched_types/issue-38371.rs:14:8 | 14 | fn foo(&foo: Foo) { | ^^^^ expected struct `Foo`, found reference | = expected type `Foo` = found type `&_` = help: did you mean `foo: &Foo`? error[E0308]: mismatched types --> ../../src/test/ui/mismatched_types/issue-38371.rs:26:9 | 26 | fn agh(&&bar: &u32) { | ^^^^ expected u32, found reference | = expected type `u32` = found type `&_`

If you feel there'd be value on that, I could create an issue and start a PR for it.

I agree that looks better

Opened issue #38901 and PR #38902 for this.

nikomatsakis · 2017-01-10T19:48:24Z

OK, I investigated. The reason that my examples were not producing bad suggestions was because the is_arg flag is only true on the outermost pattern. This was not obvious to me and probably merits a comment. In any case, you can produce bad output with an example like this one:

fn agh(&&&bar: u32) {
}

fn main() { }

which emits:

error[E0308]: mismatched types
 --> /home/nmatsakis/tmp/foo.rs:1:8
  |
1 | fn agh(&&bar: u32) {
  |        ^^^^^ expected u32, found reference
  |
  = note: expected type `u32`
  = note:    found type `&_`
  = help: did you mean `&bar: &u32`?

error: aborting due to previous error

I pushed a commit addressing this issue and adding a comment -- but without a test case yet.

nikomatsakis · 2017-01-11T16:52:46Z

@bors r+

bors · 2017-01-11T16:52:46Z

📌 Commit d723e02 has been approved by nikomatsakis

bors · 2017-01-11T22:33:47Z

⌛ Testing commit d723e02 with merge 2a0b975...

bors · 2017-01-11T23:19:50Z

💔 Test failed - status-travis

alexcrichton · 2017-01-11T23:23:13Z

@bors: retry * #39003

…

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

bors · 2017-01-12T01:00:40Z

⌛ Testing commit d723e02 with merge 1ca100d...

Suggest solutions for `fn foo(&foo: Foo)` For a given file: ```rust struct Foo {} fn foo(&foo: Foo) {} ``` suggest: ``` error[E0308]: mismatched types --> file.rs:3:8 | 3 | fn foo(&foo: Foo) {} | ^^^^ expected struct `Foo`, found reference | = note: expected type `Foo` = note: found type `&_` = help: did you mean `foo: &Foo`? ``` Fix #38371.

bors · 2017-01-12T03:13:13Z

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

rust-highfive assigned nikomatsakis Dec 25, 2016

estebank force-pushed the fix-38371 branch from 995c7d5 to f4725ba Compare December 25, 2016 21:09

Suggest solutions for fn foo(&foo: Foo)

5598f35

estebank force-pushed the fix-38371 branch from f4725ba to 5598f35 Compare December 28, 2016 23:25

review comments

f2dd75c

nikomatsakis requested changes Jan 5, 2017

View reviewed changes

estebank mentioned this pull request Jan 8, 2017

Make the expected/found notes a first class comment #38901

Closed

estebank force-pushed the fix-38371 branch 2 times, most recently from c6abd17 to a10f838 Compare January 8, 2017 06:13

add test case

563ecc1

estebank force-pushed the fix-38371 branch from a10f838 to 563ecc1 Compare January 8, 2017 07:26

only emit suggestion to move & if the inner pattern is a binding

59d7d4c

update test case to include fn(&&u32: Foo)

d723e02

bors merged commit d723e02 into rust-lang:master Jan 12, 2017

estebank deleted the fix-38371 branch November 9, 2023 05:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest solutions for `fn foo(&foo: Foo)` #38605

Suggest solutions for `fn foo(&foo: Foo)` #38605

estebank commented Dec 25, 2016 •

edited

Loading

rust-highfive commented Dec 25, 2016

bors commented Dec 28, 2016

nikomatsakis commented Dec 30, 2016

pnkfelix commented Jan 3, 2017 •

edited

Loading

nikomatsakis commented Jan 4, 2017

estebank commented Jan 4, 2017

nikomatsakis left a comment

nikomatsakis Jan 5, 2017

estebank Jan 6, 2017 •

edited

Loading

nikomatsakis Jan 6, 2017

estebank Jan 8, 2017

nikomatsakis Jan 9, 2017

nikomatsakis Jan 5, 2017

estebank Jan 6, 2017

nikomatsakis Jan 6, 2017

estebank Jan 7, 2017 •

edited

Loading

nikomatsakis commented Jan 10, 2017

nikomatsakis commented Jan 11, 2017

bors commented Jan 11, 2017

bors commented Jan 11, 2017

bors commented Jan 11, 2017

alexcrichton commented Jan 11, 2017 via email

bors commented Jan 12, 2017

bors commented Jan 12, 2017

Suggest solutions for fn foo(&foo: Foo) #38605

Suggest solutions for fn foo(&foo: Foo) #38605

Conversation

estebank commented Dec 25, 2016 • edited Loading

rust-highfive commented Dec 25, 2016

bors commented Dec 28, 2016

nikomatsakis commented Dec 30, 2016

pnkfelix commented Jan 3, 2017 • edited Loading

nikomatsakis commented Jan 4, 2017

estebank commented Jan 4, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Jan 5, 2017

Choose a reason for hiding this comment

estebank Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Jan 6, 2017

Choose a reason for hiding this comment

estebank Jan 8, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 9, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 5, 2017

Choose a reason for hiding this comment

estebank Jan 6, 2017

Choose a reason for hiding this comment

nikomatsakis Jan 6, 2017

Choose a reason for hiding this comment

estebank Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

nikomatsakis commented Jan 10, 2017

nikomatsakis commented Jan 11, 2017

bors commented Jan 11, 2017

bors commented Jan 11, 2017

bors commented Jan 11, 2017

alexcrichton commented Jan 11, 2017 via email

bors commented Jan 12, 2017

bors commented Jan 12, 2017

Suggest solutions for `fn foo(&foo: Foo)` #38605

Suggest solutions for `fn foo(&foo: Foo)` #38605

estebank commented Dec 25, 2016 •

edited

Loading

pnkfelix commented Jan 3, 2017 •

edited

Loading

estebank Jan 6, 2017 •

edited

Loading

estebank Jan 7, 2017 •

edited

Loading