Should not place errors on comments

[egamma]

From @alexandrudima on January 27, 2016 13:39

Testing #2218

Interesting squiggly placement strategy:

Number(3).

var x = 3;

// Just_some_good_text_that_did_nothing_wrong

Copied from original issue: microsoft/vscode#2449

[billti]

This is not Salsa specific (happens in TypeScript also). It's likely as the last comment is the last token in the file (and there is no next statement after the error), so it attaches there. Minor issue. Could fix if trivial, else maybe wont fix.

[zakhenry]

Looks like the problem is in this position https://github.com/Microsoft/TypeScript/blob/3853bb86d0f26e150666ab736092824eb781babc/src/compiler/parser.ts#L1849

Relevant excerpt:

        function parseRightSideOfDot(allowIdentifierNames: boolean): Identifier {
            // Technically a keyword is valid here as all identifiers and keywords are identifier names.
            // However, often we'll encounter this in error situations when the identifier or keyword
            // is actually starting another valid construct.
            //
            // So, we check for the following specific case:
            //
            //      name.
            //      identifierOrKeyword identifierNameOrKeyword
            //
            // Note: the newlines are important here.  For example, if that above code
            // were rewritten into:
            //
            //      name.identifierOrKeyword
            //      identifierNameOrKeyword
            //
            // Then we would consider it valid.  That's because ASI would take effect and
            // the code would be implicitly: "name.identifierOrKeyword; identifierNameOrKeyword".
            // In the first case though, ASI will not take effect because there is not a
            // line terminator after the identifier or keyword.
            if (scanner.hasPrecedingLineBreak() && tokenIsIdentifierOrKeyword(token)) {
                const matchesPattern = lookAhead(nextTokenIsIdentifierOrKeywordOnSameLine);

                if (matchesPattern) {
                    // Report that we need an identifier.  However, report it right after the dot,
                    // and not on the next token.  This is because the next token might actually
                    // be an identifier and the error would be quite confusing.
                    return <Identifier>createMissingNode(SyntaxKind.Identifier, /*reportAtCurrentPosition*/ true, Diagnostics.Identifier_expected);
                }
            }

            return allowIdentifierNames ? parseIdentifierName() : parseIdentifier();
        }

I think, maybe changing the reportAtCurrentPosition arg to false may fix the issue as the comment says it should be reported right after the dot, which would be the parseErrorAtCurrentToken case I believe. Sorry I'm not sure how to get TS set up to get all the tests running / where to put new unit tests etc to fully dive in and fix this.

[DanielRosenwasser]

Hey @xiphiaz, I think that's call is ultimately the cause. The change you're proposing would end up leaving an error at the end of file token, which is still potentially less confusing. Give it a try and we'll see.

I'd put a new test in tests/cases/compiler/missingIdentifierWithTrailingComment01.ts with the following contents:

Number(3).

// var x = 3;

// Just_some_good_text_that_did_nothing_wrong

Take a look at the relevant section in our CONTRIBUTING.md for writing a new test and running our test suite.