libsyntax: stop early enough if keyword is found instead of ident

[matklad]

fixes #28439

The signature of public function check_strict_keywords is changed. This function is used only once in rustc in parse_ident.

This breaks 9 tests in parse-fail category, because they expect two messages instead of one. I think it's safe just to fix the tests expectations.

[rust-highfive]

r? @alexcrichton

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

[alexcrichton]

Looks like this has some regressions in the compile-fail tests which will need to be addressed.

I also think that the current behavior is fine, it's targeted at cases like:

enum Foo {
    Self(i32),
    // ...
}

In this case you'll get an error message about Self and nothing else, but the parser will keep going and try to emit more errors. In that sense I'm not 100% convinced that #28439 is indeed a bug.

[matklad]

In that sense I'm not 100% convinced that #28439 is indeed a bug.

Same for me.

Cons:

• the parser will not recover after Self(i32)
• a public interface is changed
• current behaviour is changed.

Pros:

• if we actually look at the changed tests, matklad@95a87ba, parser recovery usually leads to some obscure error like this "ERROR expected one of !, ,, ., ::, ;, ], {, or an operator, found 1". I argue that reporting only one error in such cases is much better. The caveat is that though such cases form a significant majority in tests, nothing can be said about distribution in user's code.
• inconsistency between check_strict_keywords and check_reserved_keywords does not feel right: https://github.com/rust-lang/rust/blob/master/src/libsyntax/parse/parser.rs#L548-L549

I have no strong opinion about what is best, but I am slightly in favor of change.

[matklad]

@alexcrichton so should I repair the test (and move it to parse-fail category), or this is a won't fix?

[alexcrichton]

In terms of recovering from parser errors, I think it makes the most sense to try to recover from errors that are likely to arise. For example it's expected that enum Foo { ident } is a valid declaration, so if ident is actually a keyword it may make the most sense for the parser to assume it was a bland identifier and just keep parsing as usual.

A lot of the parse-fail tests are either super old syntax or are just kinda semi-random syntax, which is bound to always give obscure parse errors. I suppose what I'm getting at is that I'm ok with obscure code giving obscure errors, it makes more sense to try to give high quality errors (and as many of them as possible) for almost-reasonable code as it's the most likely kind to be written by accident.

I also wouldn't worry too much about public interfaces here, everything is unstable so we're free to change at will.

[matklad]

So let's close the PR and the issue.

[alexcrichton]

Ok, thanks regardless for the PR @matklad!