From 7f5994958b33dbbaaeccd4b6eef8584c960d4d71 Mon Sep 17 00:00:00 2001 From: Sheetal Nandi Date: Fri, 27 Mar 2020 12:00:34 -0700 Subject: [PATCH] Handle comment directives in incremental parsing (#37632) * Add test case that shows failure to handle commentDirectives in incremental parsing Testcase for #37536 * Handle comment directives in incremental parsing Fixes #37536 --- src/compiler/parser.ts | 61 ++++++- src/testRunner/unittests/incrementalParser.ts | 153 ++++++++++++++++++ .../unittests/tsserver/configuredProjects.ts | 23 +-- .../forceConsistentCasingInFileNames.ts | 25 +-- src/testRunner/unittests/tsserver/helpers.ts | 10 ++ src/testRunner/unittests/tsserver/openFile.ts | 69 ++++++++ .../unittests/tsserver/projectErrors.ts | 31 +--- .../unittests/tsserver/projectReferences.ts | 24 +-- src/testRunner/unittests/tsserver/projects.ts | 7 +- 9 files changed, 307 insertions(+), 96 deletions(-) diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts index 24efb0c5297..ae13b113897 100644 --- a/src/compiler/parser.ts +++ b/src/compiler/parser.ts @@ -7752,7 +7752,6 @@ namespace ts { const incrementalSourceFile = sourceFile; Debug.assert(!incrementalSourceFile.hasBeenIncrementallyParsed); incrementalSourceFile.hasBeenIncrementallyParsed = true; - const oldText = sourceFile.text; const syntaxCursor = createSyntaxCursor(sourceFile); @@ -7805,10 +7804,68 @@ namespace ts { // will immediately bail out of walking any subtrees when we can see that their parents // are already correct. const result = Parser.parseSourceFile(sourceFile.fileName, newText, sourceFile.languageVersion, syntaxCursor, /*setParentNodes*/ true, sourceFile.scriptKind); - + result.commentDirectives = getNewCommentDirectives( + sourceFile.commentDirectives, + result.commentDirectives, + changeRange.span.start, + textSpanEnd(changeRange.span), + delta, + oldText, + newText, + aggressiveChecks + ); return result; } + function getNewCommentDirectives( + oldDirectives: CommentDirective[] | undefined, + newDirectives: CommentDirective[] | undefined, + changeStart: number, + changeRangeOldEnd: number, + delta: number, + oldText: string, + newText: string, + aggressiveChecks: boolean + ): CommentDirective[] | undefined { + if (!oldDirectives) return newDirectives; + let commentDirectives: CommentDirective[] | undefined; + let addedNewlyScannedDirectives = false; + for (const directive of oldDirectives) { + const { range, type } = directive; + // Range before the change + if (range.end < changeStart) { + commentDirectives = append(commentDirectives, directive); + } + else if (range.pos > changeRangeOldEnd) { + addNewlyScannedDirectives(); + // Node is entirely past the change range. We need to move both its pos and + // end, forward or backward appropriately. + const updatedDirective: CommentDirective = { + range: { pos: range.pos + delta, end: range.end + delta }, + type + }; + commentDirectives = append(commentDirectives, updatedDirective); + if (aggressiveChecks) { + Debug.assert(oldText.substring(range.pos, range.end) === newText.substring(updatedDirective.range.pos, updatedDirective.range.end)); + } + } + // Ignore ranges that fall in change range + } + addNewlyScannedDirectives(); + return commentDirectives; + + function addNewlyScannedDirectives() { + if (addedNewlyScannedDirectives) return; + addedNewlyScannedDirectives = true; + if (!commentDirectives) { + commentDirectives = newDirectives; + } + else if (newDirectives) { + commentDirectives.push(...newDirectives); + } + } + } + function moveElementEntirelyPastChangeRange(element: IncrementalElement, isArray: boolean, delta: number, oldText: string, newText: string, aggressiveChecks: boolean) { if (isArray) { visitArray(element); diff --git a/src/testRunner/unittests/incrementalParser.ts b/src/testRunner/unittests/incrementalParser.ts index 8e00feefc6e..cd06c60d096 100644 --- a/src/testRunner/unittests/incrementalParser.ts +++ b/src/testRunner/unittests/incrementalParser.ts @@ -831,5 +831,158 @@ module m3 { }\ const index = source.indexOf("enum ") + "enum ".length; insertCode(source, index, "Fo"); }); + + describe("comment directives", () => { + const tsIgnoreComment = "// @ts-ignore"; + const textWithIgnoreComment = `const x = 10; +function foo() { + // @ts-ignore + let y: string = x; + return y; +} +function bar() { + // @ts-ignore + let z : string = x; + return z; +} +function bar3() { + // @ts-ignore + let z : string = x; + return z; +} +foo(); +bar(); +bar3();`; + verifyScenario("when deleting ts-ignore comment", verifyDelete); + verifyScenario("when inserting ts-ignore comment", verifyInsert); + verifyScenario("when changing ts-ignore comment to blah", verifyChangeToBlah); + verifyScenario("when changing blah comment to ts-ignore", verifyChangeBackToDirective); + verifyScenario("when deleting blah comment", verifyDeletingBlah); + verifyScenario("when changing text that adds another comment", verifyChangeDirectiveType); + verifyScenario("when changing text that keeps the comment but adds more nodes", verifyReuseChange); + + function verifyCommentDirectives(oldText: IScriptSnapshot, newTextAndChange: { text: IScriptSnapshot; textChangeRange: TextChangeRange; }) { + const { incrementalNewTree, newTree } = compareTrees(oldText, newTextAndChange.text, newTextAndChange.textChangeRange, -1); + assert.deepEqual(incrementalNewTree.commentDirectives, newTree.commentDirectives); + } + + function verifyScenario(scenario: string, verifyChange: (atIndex: number, singleIgnore?: true) => void) { + it(`${scenario} - 0`, () => { + verifyChange(0); + }); + it(`${scenario} - 1`, () => { + verifyChange(1); + }); + it(`${scenario} - 2`, () => { + verifyChange(2); + }); + it(`${scenario} - with single ts-ignore`, () => { + verifyChange(0, /*singleIgnore*/ true); + }); + } + + function getIndexOfTsIgnoreComment(atIndex: number) { + let index: number; + for (let i = 0; i <= atIndex; i++) { + index = textWithIgnoreComment.indexOf(tsIgnoreComment); + } + return index!; + } + + function textWithIgnoreCommentFrom(text: string, singleIgnore: true | undefined) { + if (!singleIgnore) return text; + const splits = text.split(tsIgnoreComment); + if (splits.length > 2) { + const tail = splits[splits.length - 2] + splits[splits.length - 1]; + splits.length = splits.length - 2; + return splits.join(tsIgnoreComment) + tail; + } + else { + return splits.join(tsIgnoreComment); + } + } + + function verifyDelete(atIndex: number, singleIgnore?: true) { + const index = getIndexOfTsIgnoreComment(atIndex); + const oldText = ScriptSnapshot.fromString(textWithIgnoreCommentFrom(textWithIgnoreComment, singleIgnore)); + const newTextAndChange = withDelete(oldText, index, tsIgnoreComment.length); + verifyCommentDirectives(oldText, newTextAndChange); + } + + function verifyInsert(atIndex: number, singleIgnore?: true) { + const index = getIndexOfTsIgnoreComment(atIndex); + const source = textWithIgnoreCommentFrom(textWithIgnoreComment.slice(0, index) + textWithIgnoreComment.slice(index + tsIgnoreComment.length), singleIgnore); + const oldText = ScriptSnapshot.fromString(source); + const newTextAndChange = withInsert(oldText, index, tsIgnoreComment); + verifyCommentDirectives(oldText, newTextAndChange); + } + + function verifyChangeToBlah(atIndex: number, singleIgnore?: true) { + const index = getIndexOfTsIgnoreComment(atIndex) + "// ".length; + const oldText = ScriptSnapshot.fromString(textWithIgnoreCommentFrom(textWithIgnoreComment, singleIgnore)); + const newTextAndChange = withChange(oldText, index, 1, "blah "); + verifyCommentDirectives(oldText, newTextAndChange); + } + + function verifyChangeBackToDirective(atIndex: number, singleIgnore?: true) { + const index = getIndexOfTsIgnoreComment(atIndex) + "// ".length; + const source = textWithIgnoreCommentFrom(textWithIgnoreComment.slice(0, index) + "blah " + textWithIgnoreComment.slice(index + 1), singleIgnore); + const oldText = ScriptSnapshot.fromString(source); + const newTextAndChange = withChange(oldText, index, "blah ".length, "@"); + verifyCommentDirectives(oldText, newTextAndChange); + } + + function verifyDeletingBlah(atIndex: number, singleIgnore?: true) { + const tsIgnoreIndex = getIndexOfTsIgnoreComment(atIndex); + const index = tsIgnoreIndex + "// ".length; + const source = textWithIgnoreCommentFrom(textWithIgnoreComment.slice(0, index) + "blah " + textWithIgnoreComment.slice(index + 1), singleIgnore); + const oldText = ScriptSnapshot.fromString(source); + const newTextAndChange = withDelete(oldText, tsIgnoreIndex, tsIgnoreComment.length + "blah".length); + verifyCommentDirectives(oldText, newTextAndChange); + } + + function verifyChangeDirectiveType(atIndex: number, singleIgnore?: true) { + const index = getIndexOfTsIgnoreComment(atIndex) + "// @ts-".length; + const oldText = ScriptSnapshot.fromString(textWithIgnoreCommentFrom(textWithIgnoreComment, singleIgnore)); + const newTextAndChange = withChange(oldText, index, "ignore".length, "expect-error"); + verifyCommentDirectives(oldText, newTextAndChange); + } + + function verifyReuseChange(atIndex: number, singleIgnore?: true) { + const source = `const x = 10; +function foo1() { + const x1 = 10; + // @ts-ignore + let y0: string = x; + let y1: string = x; + return y1; +} +function foo2() { + const x2 = 10; + // @ts-ignore + let y0: string = x; + let y2: string = x; + return y2; +} +function foo3() { + const x3 = 10; + // @ts-ignore + let y0: string = x; + let y3: string = x; + return y3; +} +foo1(); +foo2(); +foo3();`; + const oldText = ScriptSnapshot.fromString(textWithIgnoreCommentFrom(source, singleIgnore)); + const start = source.indexOf(`const x${atIndex + 1}`); + const letStr = `let y${atIndex + 1}: string = x;`; + const end = source.indexOf(letStr) + letStr.length; + const oldSubStr = source.slice(start, end); + const newText = oldSubStr.replace(letStr, `let yn : string = x;`); + const newTextAndChange = withChange(oldText, start, end - start, newText); + verifyCommentDirectives(oldText, newTextAndChange); + } + }); }); } diff --git a/src/testRunner/unittests/tsserver/configuredProjects.ts b/src/testRunner/unittests/tsserver/configuredProjects.ts index 3838702cd32..4c61106a1d6 100644 --- a/src/testRunner/unittests/tsserver/configuredProjects.ts +++ b/src/testRunner/unittests/tsserver/configuredProjects.ts @@ -896,14 +896,7 @@ declare var console: { const host = createServerHost([barConfig, barIndex, fooConfig, fooIndex, barSymLink, lib2017, libDom]); const session = createSession(host, { canUseEvents: true, }); openFilesForSession([fooIndex, barIndex], session); - verifyGetErrRequest({ - session, - host, - expected: [ - { file: barIndex, syntax: [], semantic: [], suggestion: [] }, - { file: fooIndex, syntax: [], semantic: [], suggestion: [] }, - ] - }); + verifyGetErrRequestNoErrors({ session, host, files: [barIndex, fooIndex] }); }); it("when file name starts with ^", () => { @@ -983,18 +976,12 @@ declare var console: { } const service = session.getProjectService(); checkProjectBeforeError(service); - verifyGetErrRequest({ + verifyGetErrRequestNoErrors({ session, host, - expected: errorOnNewFileBeforeOldFile ? - [ - { file: fooBar, syntax: [], semantic: [], suggestion: [] }, - { file: foo, syntax: [], semantic: [], suggestion: [] }, - ] : - [ - { file: foo, syntax: [], semantic: [], suggestion: [] }, - { file: fooBar, syntax: [], semantic: [], suggestion: [] }, - ], + files: errorOnNewFileBeforeOldFile ? + [fooBar, foo] : + [foo, fooBar], existingTimeouts: 2 }); checkProjectAfterError(service); diff --git a/src/testRunner/unittests/tsserver/forceConsistentCasingInFileNames.ts b/src/testRunner/unittests/tsserver/forceConsistentCasingInFileNames.ts index 0f669cd9599..1440b9cd38b 100644 --- a/src/testRunner/unittests/tsserver/forceConsistentCasingInFileNames.ts +++ b/src/testRunner/unittests/tsserver/forceConsistentCasingInFileNames.ts @@ -65,13 +65,7 @@ namespace ts.projectSystem { checkNumberOfProjects(service, { configuredProjects: 1 }); const project = service.configuredProjects.get(tsconfig.path)!; checkProjectActualFiles(project, [loggerFile.path, anotherFile.path, libFile.path, tsconfig.path]); - verifyGetErrRequest({ - host, - session, - expected: [ - { file: loggerFile.path, syntax: [], semantic: [], suggestion: [] } - ] - }); + verifyGetErrRequestNoErrors({ session, host, files: [loggerFile] }); const newLoggerPath = loggerFile.path.toLowerCase(); host.renameFile(loggerFile.path, newLoggerPath); @@ -97,14 +91,7 @@ namespace ts.projectSystem { }); // Check errors in both files - verifyGetErrRequest({ - host, - session, - expected: [ - { file: newLoggerPath, syntax: [], semantic: [], suggestion: [] }, - { file: anotherFile.path, syntax: [], semantic: [], suggestion: [] } - ] - }); + verifyGetErrRequestNoErrors({ session, host, files: [newLoggerPath, anotherFile] }); }); it("when changing module name with different casing", () => { @@ -130,13 +117,7 @@ namespace ts.projectSystem { checkNumberOfProjects(service, { configuredProjects: 1 }); const project = service.configuredProjects.get(tsconfig.path)!; checkProjectActualFiles(project, [loggerFile.path, anotherFile.path, libFile.path, tsconfig.path]); - verifyGetErrRequest({ - host, - session, - expected: [ - { file: anotherFile.path, syntax: [], semantic: [], suggestion: [] } - ] - }); + verifyGetErrRequestNoErrors({ session, host, files: [anotherFile] }); session.executeCommandSeq({ command: protocol.CommandTypes.UpdateOpen, diff --git a/src/testRunner/unittests/tsserver/helpers.ts b/src/testRunner/unittests/tsserver/helpers.ts index f73a0f8e74b..9d464ee7fcc 100644 --- a/src/testRunner/unittests/tsserver/helpers.ts +++ b/src/testRunner/unittests/tsserver/helpers.ts @@ -837,6 +837,16 @@ namespace ts.projectSystem { checkAllErrors({ ...request, expectedSequenceId }); } + export interface VerifyGetErrRequestNoErrors extends VerifyGetErrRequestBase { + files: readonly (string | File)[]; + } + export function verifyGetErrRequestNoErrors(request: VerifyGetErrRequestNoErrors) { + verifyGetErrRequest({ + ...request, + expected: request.files.map(file => ({ file, syntax: [], semantic: [], suggestion: [] })) + }); + } + export interface CheckAllErrors extends VerifyGetErrRequest { expectedSequenceId: number; } diff --git a/src/testRunner/unittests/tsserver/openFile.ts b/src/testRunner/unittests/tsserver/openFile.ts index 62a7fcc729a..1bc7f10cf9f 100644 --- a/src/testRunner/unittests/tsserver/openFile.ts +++ b/src/testRunner/unittests/tsserver/openFile.ts @@ -128,5 +128,74 @@ namespace ts.projectSystem { assert.equal(project.getCurrentProgram()?.getSourceFile(aFile.path)!.text, aFileContent); } }); + + it("when file makes edits to add/remove comment directives, they are handled correcrly", () => { + const file: File = { + path: `${tscWatch.projectRoot}/file.ts`, + content: `const x = 10; +function foo() { + // @ts-ignore + let y: string = x; + return y; +} +function bar() { + // @ts-ignore + let z : string = x; + return z; +} +foo(); +bar();` + }; + const host = createServerHost([file, libFile]); + const session = createSession(host, { canUseEvents: true, }); + openFilesForSession([file], session); + verifyGetErrRequestNoErrors({ session, host, files: [file] }); + + // Remove first ts-ignore and check only first error is reported + const tsIgnoreComment = `// @ts-ignore`; + const locationOfTsIgnore = protocolTextSpanFromSubstring(file.content, tsIgnoreComment); + session.executeCommandSeq({ + command: protocol.CommandTypes.UpdateOpen, + arguments: { + changedFiles: [{ + fileName: file.path, + textChanges: [{ + newText: " ", + ...locationOfTsIgnore + }] + }] + } + }); + const locationOfY = protocolTextSpanFromSubstring(file.content, "y"); + verifyGetErrRequest({ + session, + host, + expected: [ + { + file, + syntax: [], + semantic: [ + createDiagnostic(locationOfY.start, locationOfY.end, Diagnostics.Type_0_is_not_assignable_to_type_1, ["10", "string"]), + ], + suggestion: [] + }, + ] + }); + + // Revert the change and no errors should be reported + session.executeCommandSeq({ + command: protocol.CommandTypes.UpdateOpen, + arguments: { + changedFiles: [{ + fileName: file.path, + textChanges: [{ + newText: tsIgnoreComment, + ...locationOfTsIgnore + }] + }] + } + }); + verifyGetErrRequestNoErrors({ session, host, files: [file] }); + }); }); } diff --git a/src/testRunner/unittests/tsserver/projectErrors.ts b/src/testRunner/unittests/tsserver/projectErrors.ts index fe187f7501a..90196445375 100644 --- a/src/testRunner/unittests/tsserver/projectErrors.ts +++ b/src/testRunner/unittests/tsserver/projectErrors.ts @@ -350,16 +350,7 @@ namespace ts.projectSystem { verifyErrorsInApp(); function verifyErrorsInApp() { - verifyGetErrRequest({ - session, - host, - expected: [{ - file: app, - syntax: [], - semantic: [], - suggestion: [] - }], - }); + verifyGetErrRequestNoErrors({ session, host, files: [app] }); } }); @@ -418,12 +409,7 @@ namespace ts.projectSystem { checkErrors([serverUtilities.path, app.path]); function checkErrors(openFiles: [string, string]) { - verifyGetErrRequest({ - session, - host, - expected: openFiles.map(file => ({ file, syntax: [], semantic: [], suggestion: [] })), - existingTimeouts: 2 - }); + verifyGetErrRequestNoErrors({ session, host, files: openFiles, existingTimeouts: 2 }); } }); @@ -961,18 +947,7 @@ console.log(blabla);` const { host, session, test } = createSessionForTest({ include: ["./src/*.ts", "./src/*.json"] }); - verifyGetErrRequest({ - session, - host, - expected: [ - { - file: test, - syntax: [], - semantic: [], - suggestion: [] - } - ] - }); + verifyGetErrRequestNoErrors({ session, host, files: [test] }); }); it("should report error when json is not root file found by tsconfig", () => { diff --git a/src/testRunner/unittests/tsserver/projectReferences.ts b/src/testRunner/unittests/tsserver/projectReferences.ts index 0509eba17c7..2eb7d7710ce 100644 --- a/src/testRunner/unittests/tsserver/projectReferences.ts +++ b/src/testRunner/unittests/tsserver/projectReferences.ts @@ -1487,13 +1487,7 @@ function foo() { project, [aConfig.path, aTest.path, bFoo.path, bBar.path, libFile.path] ); - verifyGetErrRequest({ - host, - session, - expected: [ - { file: aTest, syntax: [], semantic: [], suggestion: [] } - ] - }); + verifyGetErrRequestNoErrors({ session, host, files: [aTest] }); session.executeCommandSeq({ command: protocol.CommandTypes.UpdateOpen, arguments: { @@ -1507,13 +1501,7 @@ function foo() { }] } }); - verifyGetErrRequest({ - host, - session, - expected: [ - { file: aTest, syntax: [], semantic: [], suggestion: [] } - ] - }); + verifyGetErrRequestNoErrors({ session, host, files: [aTest] }); } function config(packageName: string, extraOptions: CompilerOptions, references?: string[]): File { @@ -1937,13 +1925,7 @@ foo;` assert.equal(service.findDefaultConfiguredProject(info), project); // Verify errors - verifyGetErrRequest({ - session, - host, - expected: [ - { file: main, syntax: [], semantic: [], suggestion: [] }, - ] - }); + verifyGetErrRequestNoErrors({ session, host, files: [main] }); // Verify collection of script infos service.openClientFile(dummyFile.path); diff --git a/src/testRunner/unittests/tsserver/projects.ts b/src/testRunner/unittests/tsserver/projects.ts index 1bfe61934de..fe6486e706d 100644 --- a/src/testRunner/unittests/tsserver/projects.ts +++ b/src/testRunner/unittests/tsserver/projects.ts @@ -1558,13 +1558,10 @@ namespace ts.projectSystem { host.reloadFS(files); host.checkTimeoutQueueLength(2); - verifyGetErrRequest({ + verifyGetErrRequestNoErrors({ session, host, - expected: [ - { file: fileB, syntax: [], semantic: [], suggestion: [] }, - { file: fileSubA, syntax: [], semantic: [], suggestion: [] }, - ], + files: [fileB, fileSubA], existingTimeouts: 2, onErrEvent: () => assert.isFalse(hasErrorMsg()) });