Specific diagnostic suggestions for unexpected keyword or identifier

[JoshuaKGoldberg]

Fixes #25429

I started off by special casing the generic ';' expected message for just the "parser sees a word instead of a semicolon" case... but that exposed a ton of test cases in the baselines where different unexpected words were equally or even more confusing with the new error message. There are some baselines that aren't strictly better in this new version but for fear of bloating this PR I tried to stay away from parser behavior changes and limit to only:

• Cases reasonably likely to be hit by users that are noticeably worse with the new message
• Cases relatively straightforward to add in this new system

Each of these errors are improvements over previous scenarios that gave a ';' expected complaint and generally fall into one of three categories:

• A word was parsed that seems to have a low edit distance from a known common keyword
• A word was parsed that seems to be a known common keyword and a name without a space in-between
• Parsing in a particular type of node (mostly a class property declaration) got a different word or token than expected

Error messages could definitely use a round of proofreading from someone who knows better than me how to write them...

// Unknown keyword or identifier. Did you mean 'async'?
asdync function foo() { }
~~~~~~

// Unknown keyword or identifier. Did you mean 'class'?
classs MyClass3 {}
~~~~~~

// Unexpected keyword or identifier.
classs MyClass3 {}
       ~~~~~~~~

// Unknown keyword or identifier. Did you mean 'const'?
consd myConst2 = 1;
~~~~~

// Unknown keyword or identifier. Did you mean 'declare const'?
declareconst myDeclareConst5;
~~~~~~~~~~~~

// Decorators must precede the name and all keywords of property declarations.
class Decorating {
    public @dec: number;
             ~
// Declaration expected.
    public @dec: number;
}

// Missing '=' before default property value.
class Defaulting {
    public name: string "hello";
                        ~~~~~~~
}

// Module declaration names may only use ' or " quoted strings.
module `M1` {}
       ~~~~

// Variable declaration not allowed at this location.
class MyClass {
    public const var foo = 10;
                 ~~~
}

// '{' or ';' expected.
function hasTypeGuard(x): x is x is A {
                                 ~~

// Unexpected keyword or identifier.
function hasTypeGuard(x): x is x is A {
                                    ~

// Namespace must be given a name.
namespace { }
          ~

// Type alias name cannot be 'const'.
type const = 3 ;
     ~~~~~

// Variable declaration expected.
type const = 3 ;
           ~

// Variable declaration expected.
type const = 3 ;
             ~

// Cannot start a function call in a type annotation.
class CallClass {
    a: hello();
            ~

// Unexpected token. A constructor, method, accessor, or property was expected.
    a: hello();
             ~
}

[sandersn]

Is there any corpus of syntactically incorrect code that we could use to figure out how this changes errors? For the number of new error messages, there isn't a lot of churn in our existing tests, which makes me think our coverage isn't very good.

[JoshuaKGoldberg]

@sandersn I haven't found one. Looking around at Stryker was a fun curiosity but there isn't any prior art to go off of.

Is there something you'd like me to do? I can set up some sample code given parameters for what you're interested in, such as size of the test case.

[andrewbranch]

A couple to take a closer look at. Looks pretty nice overall, though!

[sandersn]

@JoshuaKGoldberg I was hoping for a brainstorming session leading to serendipity. Thanks for brainstorming; maybe next time we'll hit on a great idea. I know other parsers have used fuzzers to good effect, so that might be a worthwhile thing for us to do as a standalone project.

Edit Second attempt for clarity: I was asking for you (and any onlookers) to brainstorm ideas for finding syntactically incorrect program text, not with a high expectation of success, but hoping for a small chance. Which you did! Your fuzzing idea is better than I hoped for, although it's probably not worth getting it set up just for this one PR.

[DanielRosenwasser]

This becomes unfortunate because you will now possibly get a semantic error. Ideally, we would just allow no-substitution template strings in these positions and gracefully handle them later in the compiler.

I'm not sure if it's that big of a problem though. I'd hold off on any changes here.

[andrewbranch]

In hindsight, I’m actually surprised this wasn’t one of the places that IllusionMH made work with NoSubstitutionTemplateLiterals. I thought we hit basically everything that didn’t conflict with the ES spec. Why even bother issuing a grammar error? But that’s a discussion for another PR 🙃

[JoshuaKGoldberg]

A few clarification requests on this thread:

• "possibly get a semantic error" -- is giving a parseErrorAt with the Module declaration names may only use ' or " quoted strings not going to stop that? I'm under the impression this PR only changes which diagnostic is emitted, not the phase.
• I'm inferring "no-substitution template strings" to be template strings that don't have any ${...}s in them?
• By "hold off on any changes here" do you mean I should leave the PR as-is, or revert the logic back to the way it was before this PR?

[JoshuaKGoldberg]

Ping @andrewbranch @DanielRosenwasser 👋 , I think this PR is ready for re-review pending my questions in this thread? 🙏

[andrewbranch]

I’m also not sure what Daniel meant on this one. This shouldn’t change semantic errors since the parse tree isn’t changing, as far as I can tell. My approval on the PR still stands, just want to make sure Daniel is happy with the changes to address his feedback.

[JoshuaKGoldberg]

Ping, @DanielRosenwasser? 🙏

[DanielRosenwasser]

I meant leave the PR as-is, sorry!

Module declaration names may only use ' or " quoted strings.

Don't know if I caught this diagnostic - I think that should be sufficient!

[DanielRosenwasser]

I think that function name is my only nit. Otherwise this seems good to go!

[rbuckton]

We should only report an error here if there's no preceding line terminator. This is valid JS:

class C {
  a
  b
  c() {}
}

ASI inserts a ; between a and b, and between b and c(). I would expect the same to be true for decorators:

class C {
  a
  @dec b
  c() {}
}

This should be legal since ASI would insert a ; between a and @dec, however now it is an error.