suggest `!` instead of erroneous `not` on if/while block parse failure #48858

zackmdavis · 2018-03-09T00:12:39Z

Impressing confused Python users with magical diagnostics is probably
worth this very slight (only 25ish lines) extra complexity in the
parser?

FIXME comments have been left to note that the formatting after
autofixing the suggestion (with rustfix or an RLS-powered IDE) won't
be optimal. (It doesn't look like any of the existing CodeMap methods
make it easy to adjust the span.)

Unfortunately, the parser doesn't seem smart enough to know which of the "try placing this code inside a block" and ! suggestions is correct. (Also, the function that sets the "inside a block" suggestion doesn't and shouldn't know about the condition, so canceling that suggestion if the ! suggestion is issued would require a new clear-suggestions method on Diagnostic/DiagnosticBuilder, which I didn't want to write until we're convinced it's necessary.) The last function in the new UI test demonstrates a case in which the "block" suggestion is right and the ! suggestion is wrong.

Resolves #46836.

r? @estebank

Impressing confused Python users with magical diagnostics is probably worth this very slight (only 25ish lines) extra complexity in the parser? FIXME comments have been left to note that the formatting after autofixing the suggestion (with `rustfix` or an RLS-powered IDE) won't be optimal. (It doesn't look like any of the existing `CodeMap` methods make it easy to adjust the span.) Resolves rust-lang#46836.

estebank

LGTM, minor suggestions.

estebank · 2018-03-09T00:22:41Z

src/test/ui/did_you_mean/issue-46836-identifier-not-instead-of-negation.stderr

+LL | |         println!("pass");
+LL | |         //~^ ERROR expected one of
+LL | |     }
+   | |_____- help: try placing this code inside a block: `{ department{println,}; }`


Can you also modify parse_block so that this suggestion is not given if the next token is an open brace, like in these cases? The suggestion as is is more likely to be wrong than right.

estebank · 2018-03-09T00:26:06Z

src/libsyntax/parse/parser.rs

+            .map_err(|mut err| {
+                if self.is_identifier_not(&cond) {
+                    let msg = "try replacing identifier `not` with the negation operator";
+                    // FIXME: replaced span doesn't include trailing whitespace


You could merge the cond.span with the next span, get the snippet and synthesize a span that does include the whitespace (in a similar way as suggested in #47574 (comment)).

estebank · 2018-03-09T00:26:43Z

src/libsyntax/parse/parser.rs

@@ -3232,6 +3247,11 @@ impl<'a> Parser<'a> {
        }
        let not_block = self.token != token::OpenDelim(token::Brace);
        let thn = self.parse_block().map_err(|mut err| {
+            if self.is_identifier_not(&cond) {
+                err.span_suggestion(cond.span, // FIXME: replaced span doesn't incl. whitespace


re: span, same as below

bjorn3 · 2018-03-14T11:58:23Z

Could you please place the try replacing identifier `not` with the negation operator help message first, as I think it is the most likely solution when you use not.

petrochenkov · 2018-03-17T00:41:09Z

This is a quite limited ad hoc way to check for not used as !.
I'd expect it to work as normal parsing would work.

You start parsing an expression (priority is important, should be the same as for ! EXPR, i.e. parse_prefix_expr).
If the current token is not and ...
The next token is not in the "can continue expression after ident" set (you can look through enum Token and make this set, or I can do it tomorrow), then ...
Report a non-fatal error, recover not as ! and continue parsing.

This would cover tests from this PR and more.

petrochenkov · 2018-03-17T00:46:28Z

src/libsyntax/parse/parser.rs

@@ -3232,6 +3247,11 @@ impl<'a> Parser<'a> {
        }
        let not_block = self.token != token::OpenDelim(token::Brace);
        let thn = self.parse_block().map_err(|mut err| {
+            if self.is_identifier_not(&cond) {
+                err.span_suggestion(cond.span, // FIXME: replaced span doesn't incl. whitespace
+                                    "try replacing identifier `not` with the negation operator",


Looks a bit too wordy for a label, I'd personally use something "try replacing not with !", but as a minimum the word "identifier" can be removed here.

The motivation was to emphasize that not has no special meaning in Rust-the-language (it's just another ordinary variable/identifier), even if rustc-the-compiler is (after this PR) nice enough to try to figure out notice what you meant, but I agree that ceteris paribus, longer messages are worse.

petrochenkov · 2018-03-17T19:28:09Z

The next token is not in the "can continue expression after ident" set

zackmdavis · 2018-03-17T19:37:52Z

This is a quite limited ad hoc way to check for not used as !. I'd expect it to work as normal parsing would work.

Yeah, that's a good point.

You start parsing an expression [...] So, the set (already inverted) is

Thanks for the pointer! I can probably look into doing it this way later today or tomorrow.

zackmdavis · 2018-03-18T06:52:32Z

I've made some exciting progress on the parse-prefix approach, but in addition to producing awesome diagnostics, my code also misparses some legitimate uses of not as a variable name. 😞

I'm pretty busy with a new dayjob lately, so I likely won't be able to look at this again for a week.

petrochenkov · 2018-03-19T22:40:58Z

@zackmdavis

also misparses some legitimate uses of not as a variable name

For example?
I certainly could miss something when making that token list.
Maybe something in macros?

zackmdavis · 2018-03-20T02:38:30Z

@petrochenkov

For example?

zmd@ReflectiveCoherence:~/Code/rust$ cat correct.rs 
fn main() {
    let not = 2;
    let _really = not;
}
zmd@ReflectiveCoherence:~/Code/rust$ rustc correct.rs # sane rustc
zmd@ReflectiveCoherence:~/Code/rust$ echo $?
0
zmd@ReflectiveCoherence:~/Code/rust$ rustc +stage1 correct.rs # my delusional work-in-progress build
error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, or an operator, found `}`
 --> correct.rs:4:1
  |
3 |     let _really = not;
  |                      - expected one of 7 possible tokens here
4 | }
  | ^ unexpected token

error: aborting due to previous error

I certainly could miss something when making that token list.

I think it's more likely to be an off-by-one-token error in my recovery logic, an error which we can pray will be revealed in the cold light of info! statements and careful thought. (But not tonight.)

(This commit is only using ident/literal/pound as the can't-continue-expression-after-ident set, because of the conjunction of my not feeling confident that I understand how the Nts work, and their being a different enum which would require another match and the fact that leaving tokens out of the set should only decrease the number of situations in which we recover without affecting the correctness of the cases where we do try to recover.)

petrochenkov · 2018-03-20T08:44:21Z

an off-by-one-token error

Ah, of course, the next token lookup is done with look_ahead, not next_tok.

zackmdavis · 2018-03-22T05:46:35Z

continued: #49258

@petrochenkov

suggest `!` for erroneous identifier `not` ![not_recovery](https://user-images.githubusercontent.com/1076988/37753255-3b669c42-2d59-11e8-9071-efad8eaf3086.png) This supersedes #48858. r? @petrochenkov

rust-highfive assigned estebank Mar 9, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 9, 2018

petrochenkov self-assigned this Mar 9, 2018

estebank reviewed Mar 9, 2018

View reviewed changes

zackmdavis mentioned this pull request Mar 9, 2018

Tracking issue for RFC 1940: allow #[must_use] on functions #43302

Closed

3 tasks

mark-i-m mentioned this pull request Mar 13, 2018

RFC: libsyntax2.0 rust-lang/rfcs#2256

Closed

estebank added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 16, 2018

petrochenkov reviewed Mar 17, 2018

View reviewed changes

zackmdavis mentioned this pull request Mar 22, 2018

suggest ! for erroneous identifier not #49258

Merged

zackmdavis closed this Mar 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

suggest `!` instead of erroneous `not` on if/while block parse failure #48858

suggest `!` instead of erroneous `not` on if/while block parse failure #48858

zackmdavis commented Mar 9, 2018

estebank left a comment

estebank Mar 9, 2018

estebank Mar 9, 2018

estebank Mar 9, 2018

bjorn3 commented Mar 14, 2018

petrochenkov commented Mar 17, 2018

petrochenkov Mar 17, 2018

zackmdavis Mar 17, 2018

petrochenkov commented Mar 17, 2018

zackmdavis commented Mar 17, 2018

zackmdavis commented Mar 18, 2018

petrochenkov commented Mar 19, 2018

zackmdavis commented Mar 20, 2018

petrochenkov commented Mar 20, 2018

zackmdavis commented Mar 22, 2018

suggest ! instead of erroneous not on if/while block parse failure #48858

suggest ! instead of erroneous not on if/while block parse failure #48858

Conversation

zackmdavis commented Mar 9, 2018

estebank left a comment

Choose a reason for hiding this comment

estebank Mar 9, 2018

Choose a reason for hiding this comment

estebank Mar 9, 2018

Choose a reason for hiding this comment

estebank Mar 9, 2018

Choose a reason for hiding this comment

bjorn3 commented Mar 14, 2018

petrochenkov commented Mar 17, 2018

petrochenkov Mar 17, 2018

Choose a reason for hiding this comment

zackmdavis Mar 17, 2018

Choose a reason for hiding this comment

petrochenkov commented Mar 17, 2018

zackmdavis commented Mar 17, 2018

zackmdavis commented Mar 18, 2018

petrochenkov commented Mar 19, 2018

zackmdavis commented Mar 20, 2018

petrochenkov commented Mar 20, 2018

zackmdavis commented Mar 22, 2018

suggest `!` instead of erroneous `not` on if/while block parse failure #48858

suggest `!` instead of erroneous `not` on if/while block parse failure #48858