Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Specific diagnostic suggestions for unexpected keyword or identifier #43005

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Feb 28, 2021

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();
             ~
}

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Feb 28, 2021
@JoshuaKGoldberg JoshuaKGoldberg force-pushed the jg-unexpected-keyword-identifier-diagnostics branch 2 times, most recently from 61a4bf3 to ec81e4d Compare March 1, 2021 00:13
@JoshuaKGoldberg JoshuaKGoldberg force-pushed the jg-unexpected-keyword-identifier-diagnostics branch from ec81e4d to 0c967f9 Compare March 1, 2021 00:15
@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as ready for review March 1, 2021 00:49
@JoshuaKGoldberg JoshuaKGoldberg changed the title Specific diagnostic suggestions for unexpected keywords or identifier Specific diagnostic suggestions for unexpected keyword or identifier Mar 1, 2021
src/compiler/parser.ts Outdated Show resolved Hide resolved
@sandersn sandersn added this to Not started in PR Backlog Mar 9, 2021
@sandersn sandersn moved this from Not started to Needs review in PR Backlog Mar 9, 2021
@sandersn
Copy link
Member

sandersn commented Mar 9, 2021

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
Copy link
Contributor Author

@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.

Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sandersn
Copy link
Member

sandersn commented Mar 31, 2021

@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
Copy link
Member

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

PR Backlog automation moved this from Waiting on author to Needs merge Jul 14, 2021
@DanielRosenwasser DanielRosenwasser merged commit 541e553 into microsoft:main Jul 14, 2021
PR Backlog automation moved this from Needs merge to Done Jul 14, 2021
@JoshuaKGoldberg JoshuaKGoldberg deleted the jg-unexpected-keyword-identifier-diagnostics branch July 14, 2021 21:26
function parseSemicolonAfterPropertyName(name: PropertyName, type: TypeNode | undefined, initializer: Expression | undefined) {
switch (token()) {
case SyntaxKind.AtToken:
parseErrorAtCurrentToken(Diagnostics.Decorators_must_precede_the_name_and_all_keywords_of_property_declarations);
Copy link
Member

@rbuckton rbuckton Jul 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

Provide a better error message when "async" is mistyped?
6 participants