Point at `:` when using it instead of `;` by estebank · Pull Request #43096 · rust-lang/rust

estebank · 2017-07-06T21:35:53Z

When triggering type ascription in such a way that we can infer a
statement end was intended, add a suggestion for the change. Always
point out the reason for the expectation of a type is due to type
ascription.

Fix#42057, #41928.

rust-highfive · 2017-07-06T21:36:05Z

r? @nikomatsakis

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

bors · 2017-07-07T12:55:14Z

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

KalitaAlexey · 2017-07-07T14:18:25Z

Will it work if a colon was here?

The output in that case is
error: expected type, found `0` --> ../../src/test/ui/suggestions/type-ascription-instead-of-statement-end.rs:15:5 | 14 | println!("test"): | - help: did you mean to use `;` here instead? 15 | 0: | ^ expecting a type here because of type ascription

nikomatsakis · 2017-07-07T15:45:44Z

this sort of looks funny, perhaps because of the ^ having no label. I think I'd prefer:
error: expected type, found `0` --> $DIR/type-ascription-instead-of-statement-end.rs:15:5 | 14 | println!("test"): | - help: did you mean to use a `;` here? 15 | 0; | ^ expecting a type here because of type ascription error: aborting due to previous error

nikomatsakis · 2017-07-07T21:43:17Z

Huh, this looks strange to me somehow. Why not say "help: did you mean to use ; here?"? I think users might not know that "end the statement" means ; -- but also, I am not sure about this "floating" ; that appears at the end of the sentence.

The presentation is because it is an inline suggestion. Should I change the code so that we can control wether the code to be replaced should be displayed when inline?

Hmm; I think either change the code, or change the wording. But yeah probably just having the option to "suppress" the suggestion when displayed inline seems good.

shepmaster · 2017-07-14T20:03:30Z

This is your polite 7-day ping @estebank — when do you think you'll be able to respond to the comments?

When triggering type ascription in such a way that we can infer a statement end was intended, add a suggestion for the change. Always point out the reason for the expectation of a type is due to type ascription.

nikomatsakis · 2017-07-17T08:58:58Z

Failure:

[01:08:34] failures:
[01:08:34] [ui] ui/issue-22644.rs

The new message looks improved, though. =)

Now there's a way to add suggestions that hide the suggested code when presented inline, to avoid weird wording when short code snippets are added at the end.

estebank · 2017-07-17T19:23:41Z

@nikomatsakis fixed

estebank · 2017-07-18T23:07:18Z

@nikomatsakis covered another case that fails during resolve instead of parse:

fnf(){}fnmain(){f():f();}

estebank · 2017-07-23T18:17:50Z

@nikomatsakis ping

nikomatsakis · 2017-07-23T18:40:33Z

@bors r+

bors · 2017-07-23T18:40:34Z

📌 Commit e39bcec has been approved by nikomatsakis

nikomatsakis · 2017-07-23T18:48:36Z

cc @oli-obk, I don't this suggestion follows the guidelines, but I also think it looks good. Perhaps we need to adjust the guidelines to account for the "suppress example code" option?

bors · 2017-07-23T20:56:26Z

⌛ Testing commit e39bcec with merge afe145d...

Point at `:` when using it instead of `;` When triggering type ascription in such a way that we can infer a statement end was intended, add a suggestion for the change. Always point out the reason for the expectation of a type is due to type ascription. Fix#42057, #41928.

oli-obk · 2017-07-23T21:56:07Z

I'm assuming you mean that the guildelines aren't followed by the use of did you mean. This guideline exists because of #42033 (comment) which notes that the did you mean sounds condescending. I don't have a strong opinion on this guideline though.

"Try using a ; instead:" looks fine to me. Otherwise let's adjust the guidelines if that's desired.

bors · 2017-07-23T23:26:15Z

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

luser · 2017-07-25T12:24:32Z

@estebank Thanks for making this error message better!

Nokel81 · 2017-07-26T16:46:30Z

Which error message got finalized?

estebank · 2017-07-26T16:51:13Z

@Nokel81 sorry, I don't understand the question. Could you rephrase? This PR adds a label stating "help: did you mean to use ; here?" when type ascription (triggered by :) ends the previous line to the type error, as it probably was a mistyped :

Nokel81 · 2017-07-26T16:54:36Z

I was talking about the guideline of not using did you mean that was referenced several times near the end of this PR chain. Was the guidelines followed or was the original error message kept as help: did you mean to use ; here instead?

estebank · 2017-07-26T17:10:38Z

@Nokel81 ah, I see. No, I didn't change the message before it got merged. I can post a PR later today/tomorrow changing the wording, if you want.

Nokel81 · 2017-07-26T17:12:40Z

No it is fine, I personally feel that there are some cases where did you mean makes sense. Mostly for small syntactic typos like this. I was just wondering if it had actually been changed

rust-highfive assigned nikomatsakis Jul 6, 2017

estebank force-pushed the ascription-help branch from 37151af to 6627668Compare July 6, 2017 23:32

KalitaAlexey reviewed Jul 7, 2017
View reviewed changes

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

nikomatsakis reviewed Jul 7, 2017
View reviewed changes

estebank force-pushed the ascription-help branch 2 times, most recently from ad7c8eb to a4217cbCompare July 7, 2017 21:18

nikomatsakis reviewed Jul 7, 2017
View reviewed changes

Point at : when using it instead of ;
7239d77
When triggering type ascription in such a way that we can infer a statement end was intended, add a suggestion for the change. Always point out the reason for the expectation of a type is due to type ascription.

estebank force-pushed the ascription-help branch from 5f2261a to c55295fCompare July 16, 2017 18:49

Add flag to hide code on inline suggestions
faf9035
Now there's a way to add suggestions that hide the suggested code when presented inline, to avoid weird wording when short code snippets are added at the end.

estebank force-pushed the ascription-help branch from c55295f to faf9035Compare July 17, 2017 16:28

Handle type ascription cases with a method call instead of a type
e39bcec

estebank force-pushed the ascription-help branch from ce5da67 to e39bcecCompare July 18, 2017 20:33

nikomatsakis approved these changes Jul 23, 2017
View reviewed changes

bors merged commit e39bcec into rust-lang:masterJul 23, 2017

ebfull mentioned this pull request Jul 24, 2017
CodeSuggestion has new field in latest nightly rust-lang/rust-clippy#1902
Closed

Uh oh!

Uh oh!

Conversation

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

Uh oh!

Uh oh!

rust-highfive commented Jul 6, 2017

Uh oh!

bors commented Jul 7, 2017

Uh oh!

KalitaAlexeyJul 7, 2017

Choose a reason for hiding this comment

Uh oh!

estebankJul 16, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakisJul 7, 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!

estebankJul 7, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakisJul 7, 2017

Choose a reason for hiding this comment

Uh oh!

estebankJul 7, 2017

Choose a reason for hiding this comment

Uh oh!

nikomatsakisJul 10, 2017

Choose a reason for hiding this comment

Uh oh!

estebankJul 16, 2017

Choose a reason for hiding this comment

Uh oh!

shepmaster commented Jul 14, 2017

Uh oh!

nikomatsakis commented Jul 17, 2017

Uh oh!

estebank commented Jul 17, 2017

Uh oh!

estebank commented Jul 18, 2017

Uh oh!

estebank commented Jul 23, 2017

Uh oh!

nikomatsakis commented Jul 23, 2017

Uh oh!

bors commented Jul 23, 2017

Uh oh!

nikomatsakis commented Jul 23, 2017

Uh oh!

bors commented Jul 23, 2017

Uh oh!

oli-obk commented Jul 23, 2017

Uh oh!

bors commented Jul 23, 2017

Uh oh!

luser commented Jul 25, 2017

Uh oh!

Nokel81 commented Jul 26, 2017

Uh oh!

estebank commented Jul 26, 2017

Uh oh!

Nokel81 commented Jul 26, 2017

Uh oh!

estebank commented Jul 26, 2017

Uh oh!

Nokel81 commented Jul 26, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

estebank commented Jul 6, 2017•
edited
Loading

nikomatsakisJul 7, 2017•
edited
Loading

Nokel81 commented Jul 26, 2017•
edited
Loading