Skip to content

Don’t create missing nodes for identifiers that would be valid in a newer script target #42520

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

Merged
merged 4 commits into from
Jan 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1637,8 +1637,8 @@ namespace ts {
// with magic property names like '__proto__'. The 'identifiers' object is used to share a single string instance for
// each identifier in order to reduce memory consumption.
function createIdentifier(isIdentifier: boolean, diagnosticMessage?: DiagnosticMessage, privateIdentifierDiagnosticMessage?: DiagnosticMessage): Identifier {
identifierCount++;
if (isIdentifier) {
identifierCount++;
Copy link
Member

Choose a reason for hiding this comment

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

We only use identifierCount for diagnostics purposes. Is there a reason to make it conditional?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn’t think it was super important, but realized it was double-counting invalid identifiers, and it was easy to make it more accurate.

const pos = getNodePos();
// Store original token kind if it is not just an Identifier so we can report appropriate error later in type checker
const originalKeywordKind = token();
Expand All @@ -1652,6 +1652,12 @@ namespace ts {
return createIdentifier(/*isIdentifier*/ true);
}

if (token() === SyntaxKind.Unknown && scanner.tryScan(() => scanner.reScanInvalidIdentifier() === SyntaxKind.Identifier)) {
// Scanner has already recorded an 'Invalid character' error, so no need to add another from the parser.
return createIdentifier(/*isIdentifier*/ true);
}

identifierCount++;
// Only for end of file because the error gets reported incorrectly on embedded script tags.
const reportAtCurrentPosition = token() === SyntaxKind.EndOfFileToken;

Expand Down
39 changes: 31 additions & 8 deletions src/compiler/scanner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ namespace ts {
reScanJsxToken(): JsxTokenSyntaxKind;
reScanLessThanToken(): SyntaxKind;
reScanQuestionToken(): SyntaxKind;
reScanInvalidIdentifier(): SyntaxKind;
scanJsxToken(): JsxTokenSyntaxKind;
scanJsDocToken(): JSDocSyntaxKind;
scan(): SyntaxKind;
Expand Down Expand Up @@ -966,6 +967,7 @@ namespace ts {
reScanJsxToken,
reScanLessThanToken,
reScanQuestionToken,
reScanInvalidIdentifier,
scanJsxToken,
scanJsDocToken,
scan,
Expand Down Expand Up @@ -2041,14 +2043,9 @@ namespace ts {
}
return token = SyntaxKind.PrivateIdentifier;
default:
if (isIdentifierStart(ch, languageVersion)) {
pos += charSize(ch);
while (pos < end && isIdentifierPart(ch = codePointAt(text, pos), languageVersion)) pos += charSize(ch);
tokenValue = text.substring(tokenPos, pos);
if (ch === CharacterCodes.backslash) {
tokenValue += scanIdentifierParts();
}
return token = getIdentifierToken();
const identifierKind = scanIdentifier(ch, languageVersion);
if (identifierKind) {
return token = identifierKind;
}
else if (isWhiteSpaceSingleLine(ch)) {
pos += charSize(ch);
Expand All @@ -2066,6 +2063,32 @@ namespace ts {
}
}

function reScanInvalidIdentifier(): SyntaxKind {
Debug.assert(token === SyntaxKind.Unknown, "'reScanInvalidIdentifier' should only be called when the current token is 'SyntaxKind.Unknown'.");
pos = tokenPos = startPos;
tokenFlags = 0;
const ch = codePointAt(text, pos);
const identifierKind = scanIdentifier(ch, ScriptTarget.ESNext);
if (identifierKind) {
return token = identifierKind;
}
pos += charSize(ch);
return token; // Still `SyntaKind.Unknown`
}

function scanIdentifier(startCharacter: number, languageVersion: ScriptTarget) {
let ch = startCharacter;
if (isIdentifierStart(ch, languageVersion)) {
pos += charSize(ch);
while (pos < end && isIdentifierPart(ch = codePointAt(text, pos), languageVersion)) pos += charSize(ch);
tokenValue = text.substring(tokenPos, pos);
if (ch === CharacterCodes.backslash) {
tokenValue += scanIdentifierParts();
}
return getIdentifierToken();
}
}

function reScanGreaterToken(): SyntaxKind {
if (token === SyntaxKind.GreaterThanToken) {
if (text.charCodeAt(pos) === CharacterCodes.greaterThan) {
Expand Down
4 changes: 2 additions & 2 deletions src/harness/fourslashImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1453,9 +1453,9 @@ namespace FourSlash {
}

public baselineRename(marker: string, options: FourSlashInterface.RenameOptions) {
const position = this.getMarkerByName(marker).position;
const { fileName, position } = this.getMarkerByName(marker);
const locations = this.languageService.findRenameLocations(
this.activeFile.fileName,
fileName,
position,
options.findInStrings ?? false,
options.findInComments ?? false,
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3970,6 +3970,7 @@ declare namespace ts {
reScanJsxToken(): JsxTokenSyntaxKind;
reScanLessThanToken(): SyntaxKind;
reScanQuestionToken(): SyntaxKind;
reScanInvalidIdentifier(): SyntaxKind;
scanJsxToken(): JsxTokenSyntaxKind;
scanJsDocToken(): JSDocSyntaxKind;
scan(): SyntaxKind;
Expand Down
1 change: 1 addition & 0 deletions tests/baselines/reference/api/typescript.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3970,6 +3970,7 @@ declare namespace ts {
reScanJsxToken(): JsxTokenSyntaxKind;
reScanLessThanToken(): SyntaxKind;
reScanQuestionToken(): SyntaxKind;
reScanInvalidIdentifier(): SyntaxKind;
scanJsxToken(): JsxTokenSyntaxKind;
scanJsDocToken(): JSDocSyntaxKind;
scan(): SyntaxKind;
Expand Down
21 changes: 21 additions & 0 deletions tests/baselines/reference/processInvalidSyntax1.baseline
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*====== /tests/cases/fourslash/decl.js ======*/

var RENAME = {};

/*====== /tests/cases/fourslash/unicode1.js ======*/

RENAME.𝒜 ;

/*====== /tests/cases/fourslash/unicode2.js ======*/

RENAME.¬ ;

/*====== /tests/cases/fourslash/unicode3.js ======*/

RENAME¬

/*====== /tests/cases/fourslash/forof.js ======*/

for (RENAME.prop of arr) {

}
25 changes: 25 additions & 0 deletions tests/cases/fourslash/processInvalidSyntax1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/// <reference path="fourslash.ts" />

// @allowJs: true

// Test validates that language service getChildren() doesn't
// crash due to invalid identifier in unicode.js.

// @Filename: decl.js
//// var obj = {};

// @Filename: unicode1.js
//// obj.𝒜 ;

// @Filename: unicode2.js
//// obj.¬ ;

// @Filename: unicode3.js
//// obj¬

// @Filename: forof.js
//// for (obj/**/.prop of arr) {
////
//// }

verify.baselineRename("", {});