Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 7f59949

Browse filesBrowse files
sheetalkamatsheetalkamat
authored
Handle comment directives in incremental parsing (microsoft#37632)
* Add test case that shows failure to handle commentDirectives in incremental parsing Testcase for microsoft#37536 * Handle comment directives in incremental parsing Fixes microsoft#37536
1 parent 0f3a9d4 commit 7f59949
Copy full SHA for 7f59949

9 files changed

+307-96Lines changed: 307 additions & 96 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎src/compiler/parser.ts‎

Copy file name to clipboardExpand all lines: src/compiler/parser.ts
+59-2Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7752,7 +7752,6 @@ namespace ts {
77527752
const incrementalSourceFile = <IncrementalNode><Node>sourceFile;
77537753
Debug.assert(!incrementalSourceFile.hasBeenIncrementallyParsed);
77547754
incrementalSourceFile.hasBeenIncrementallyParsed = true;
7755-
77567755
const oldText = sourceFile.text;
77577756
const syntaxCursor = createSyntaxCursor(sourceFile);
77587757

@@ -7805,10 +7804,68 @@ namespace ts {
78057804
// will immediately bail out of walking any subtrees when we can see that their parents
78067805
// are already correct.
78077806
const result = Parser.parseSourceFile(sourceFile.fileName, newText, sourceFile.languageVersion, syntaxCursor, /*setParentNodes*/ true, sourceFile.scriptKind);
7808-
7807+
result.commentDirectives = getNewCommentDirectives(
7808+
sourceFile.commentDirectives,
7809+
result.commentDirectives,
7810+
changeRange.span.start,
7811+
textSpanEnd(changeRange.span),
7812+
delta,
7813+
oldText,
7814+
newText,
7815+
aggressiveChecks
7816+
);
78097817
return result;
78107818
}
78117819

7820+
function getNewCommentDirectives(
7821+
oldDirectives: CommentDirective[] | undefined,
7822+
newDirectives: CommentDirective[] | undefined,
7823+
changeStart: number,
7824+
changeRangeOldEnd: number,
7825+
delta: number,
7826+
oldText: string,
7827+
newText: string,
7828+
aggressiveChecks: boolean
7829+
): CommentDirective[] | undefined {
7830+
if (!oldDirectives) return newDirectives;
7831+
let commentDirectives: CommentDirective[] | undefined;
7832+
let addedNewlyScannedDirectives = false;
7833+
for (const directive of oldDirectives) {
7834+
const { range, type } = directive;
7835+
// Range before the change
7836+
if (range.end < changeStart) {
7837+
commentDirectives = append(commentDirectives, directive);
7838+
}
7839+
else if (range.pos > changeRangeOldEnd) {
7840+
addNewlyScannedDirectives();
7841+
// Node is entirely past the change range. We need to move both its pos and
7842+
// end, forward or backward appropriately.
7843+
const updatedDirective: CommentDirective = {
7844+
range: { pos: range.pos + delta, end: range.end + delta },
7845+
type
7846+
};
7847+
commentDirectives = append(commentDirectives, updatedDirective);
7848+
if (aggressiveChecks) {
7849+
Debug.assert(oldText.substring(range.pos, range.end) === newText.substring(updatedDirective.range.pos, updatedDirective.range.end));
7850+
}
7851+
}
7852+
// Ignore ranges that fall in change range
7853+
}
7854+
addNewlyScannedDirectives();
7855+
return commentDirectives;
7856+
7857+
function addNewlyScannedDirectives() {
7858+
if (addedNewlyScannedDirectives) return;
7859+
addedNewlyScannedDirectives = true;
7860+
if (!commentDirectives) {
7861+
commentDirectives = newDirectives;
7862+
}
7863+
else if (newDirectives) {
7864+
commentDirectives.push(...newDirectives);
7865+
}
7866+
}
7867+
}
7868+
78127869
function moveElementEntirelyPastChangeRange(element: IncrementalElement, isArray: boolean, delta: number, oldText: string, newText: string, aggressiveChecks: boolean) {
78137870
if (isArray) {
78147871
visitArray(<IncrementalNodeArray>element);
Collapse file

‎src/testRunner/unittests/incrementalParser.ts‎

Copy file name to clipboardExpand all lines: src/testRunner/unittests/incrementalParser.ts
+153Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -831,5 +831,158 @@ module m3 { }\
831831
const index = source.indexOf("enum ") + "enum ".length;
832832
insertCode(source, index, "Fo");
833833
});
834+
835+
describe("comment directives", () => {
836+
const tsIgnoreComment = "// @ts-ignore";
837+
const textWithIgnoreComment = `const x = 10;
838+
function foo() {
839+
// @ts-ignore
840+
let y: string = x;
841+
return y;
842+
}
843+
function bar() {
844+
// @ts-ignore
845+
let z : string = x;
846+
return z;
847+
}
848+
function bar3() {
849+
// @ts-ignore
850+
let z : string = x;
851+
return z;
852+
}
853+
foo();
854+
bar();
855+
bar3();`;
856+
verifyScenario("when deleting ts-ignore comment", verifyDelete);
857+
verifyScenario("when inserting ts-ignore comment", verifyInsert);
858+
verifyScenario("when changing ts-ignore comment to blah", verifyChangeToBlah);
859+
verifyScenario("when changing blah comment to ts-ignore", verifyChangeBackToDirective);
860+
verifyScenario("when deleting blah comment", verifyDeletingBlah);
861+
verifyScenario("when changing text that adds another comment", verifyChangeDirectiveType);
862+
verifyScenario("when changing text that keeps the comment but adds more nodes", verifyReuseChange);
863+
864+
function verifyCommentDirectives(oldText: IScriptSnapshot, newTextAndChange: { text: IScriptSnapshot; textChangeRange: TextChangeRange; }) {
865+
const { incrementalNewTree, newTree } = compareTrees(oldText, newTextAndChange.text, newTextAndChange.textChangeRange, -1);
866+
assert.deepEqual(incrementalNewTree.commentDirectives, newTree.commentDirectives);
867+
}
868+
869+
function verifyScenario(scenario: string, verifyChange: (atIndex: number, singleIgnore?: true) => void) {
870+
it(`${scenario} - 0`, () => {
871+
verifyChange(0);
872+
});
873+
it(`${scenario} - 1`, () => {
874+
verifyChange(1);
875+
});
876+
it(`${scenario} - 2`, () => {
877+
verifyChange(2);
878+
});
879+
it(`${scenario} - with single ts-ignore`, () => {
880+
verifyChange(0, /*singleIgnore*/ true);
881+
});
882+
}
883+
884+
function getIndexOfTsIgnoreComment(atIndex: number) {
885+
let index: number;
886+
for (let i = 0; i <= atIndex; i++) {
887+
index = textWithIgnoreComment.indexOf(tsIgnoreComment);
888+
}
889+
return index!;
890+
}
891+
892+
function textWithIgnoreCommentFrom(text: string, singleIgnore: true | undefined) {
893+
if (!singleIgnore) return text;
894+
const splits = text.split(tsIgnoreComment);
895+
if (splits.length > 2) {
896+
const tail = splits[splits.length - 2] + splits[splits.length - 1];
897+
splits.length = splits.length - 2;
898+
return splits.join(tsIgnoreComment) + tail;
899+
}
900+
else {
901+
return splits.join(tsIgnoreComment);
902+
}
903+
}
904+
905+
function verifyDelete(atIndex: number, singleIgnore?: true) {
906+
const index = getIndexOfTsIgnoreComment(atIndex);
907+
const oldText = ScriptSnapshot.fromString(textWithIgnoreCommentFrom(textWithIgnoreComment, singleIgnore));
908+
const newTextAndChange = withDelete(oldText, index, tsIgnoreComment.length);
909+
verifyCommentDirectives(oldText, newTextAndChange);
910+
}
911+
912+
function verifyInsert(atIndex: number, singleIgnore?: true) {
913+
const index = getIndexOfTsIgnoreComment(atIndex);
914+
const source = textWithIgnoreCommentFrom(textWithIgnoreComment.slice(0, index) + textWithIgnoreComment.slice(index + tsIgnoreComment.length), singleIgnore);
915+
const oldText = ScriptSnapshot.fromString(source);
916+
const newTextAndChange = withInsert(oldText, index, tsIgnoreComment);
917+
verifyCommentDirectives(oldText, newTextAndChange);
918+
}
919+
920+
function verifyChangeToBlah(atIndex: number, singleIgnore?: true) {
921+
const index = getIndexOfTsIgnoreComment(atIndex) + "// ".length;
922+
const oldText = ScriptSnapshot.fromString(textWithIgnoreCommentFrom(textWithIgnoreComment, singleIgnore));
923+
const newTextAndChange = withChange(oldText, index, 1, "blah ");
924+
verifyCommentDirectives(oldText, newTextAndChange);
925+
}
926+
927+
function verifyChangeBackToDirective(atIndex: number, singleIgnore?: true) {
928+
const index = getIndexOfTsIgnoreComment(atIndex) + "// ".length;
929+
const source = textWithIgnoreCommentFrom(textWithIgnoreComment.slice(0, index) + "blah " + textWithIgnoreComment.slice(index + 1), singleIgnore);
930+
const oldText = ScriptSnapshot.fromString(source);
931+
const newTextAndChange = withChange(oldText, index, "blah ".length, "@");
932+
verifyCommentDirectives(oldText, newTextAndChange);
933+
}
934+
935+
function verifyDeletingBlah(atIndex: number, singleIgnore?: true) {
936+
const tsIgnoreIndex = getIndexOfTsIgnoreComment(atIndex);
937+
const index = tsIgnoreIndex + "// ".length;
938+
const source = textWithIgnoreCommentFrom(textWithIgnoreComment.slice(0, index) + "blah " + textWithIgnoreComment.slice(index + 1), singleIgnore);
939+
const oldText = ScriptSnapshot.fromString(source);
940+
const newTextAndChange = withDelete(oldText, tsIgnoreIndex, tsIgnoreComment.length + "blah".length);
941+
verifyCommentDirectives(oldText, newTextAndChange);
942+
}
943+
944+
function verifyChangeDirectiveType(atIndex: number, singleIgnore?: true) {
945+
const index = getIndexOfTsIgnoreComment(atIndex) + "// @ts-".length;
946+
const oldText = ScriptSnapshot.fromString(textWithIgnoreCommentFrom(textWithIgnoreComment, singleIgnore));
947+
const newTextAndChange = withChange(oldText, index, "ignore".length, "expect-error");
948+
verifyCommentDirectives(oldText, newTextAndChange);
949+
}
950+
951+
function verifyReuseChange(atIndex: number, singleIgnore?: true) {
952+
const source = `const x = 10;
953+
function foo1() {
954+
const x1 = 10;
955+
// @ts-ignore
956+
let y0: string = x;
957+
let y1: string = x;
958+
return y1;
959+
}
960+
function foo2() {
961+
const x2 = 10;
962+
// @ts-ignore
963+
let y0: string = x;
964+
let y2: string = x;
965+
return y2;
966+
}
967+
function foo3() {
968+
const x3 = 10;
969+
// @ts-ignore
970+
let y0: string = x;
971+
let y3: string = x;
972+
return y3;
973+
}
974+
foo1();
975+
foo2();
976+
foo3();`;
977+
const oldText = ScriptSnapshot.fromString(textWithIgnoreCommentFrom(source, singleIgnore));
978+
const start = source.indexOf(`const x${atIndex + 1}`);
979+
const letStr = `let y${atIndex + 1}: string = x;`;
980+
const end = source.indexOf(letStr) + letStr.length;
981+
const oldSubStr = source.slice(start, end);
982+
const newText = oldSubStr.replace(letStr, `let yn : string = x;`);
983+
const newTextAndChange = withChange(oldText, start, end - start, newText);
984+
verifyCommentDirectives(oldText, newTextAndChange);
985+
}
986+
});
834987
});
835988
}
Collapse file

‎src/testRunner/unittests/tsserver/configuredProjects.ts‎

Copy file name to clipboardExpand all lines: src/testRunner/unittests/tsserver/configuredProjects.ts
+5-18Lines changed: 5 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -896,14 +896,7 @@ declare var console: {
896896
const host = createServerHost([barConfig, barIndex, fooConfig, fooIndex, barSymLink, lib2017, libDom]);
897897
const session = createSession(host, { canUseEvents: true, });
898898
openFilesForSession([fooIndex, barIndex], session);
899-
verifyGetErrRequest({
900-
session,
901-
host,
902-
expected: [
903-
{ file: barIndex, syntax: [], semantic: [], suggestion: [] },
904-
{ file: fooIndex, syntax: [], semantic: [], suggestion: [] },
905-
]
906-
});
899+
verifyGetErrRequestNoErrors({ session, host, files: [barIndex, fooIndex] });
907900
});
908901

909902
it("when file name starts with ^", () => {
@@ -983,18 +976,12 @@ declare var console: {
983976
}
984977
const service = session.getProjectService();
985978
checkProjectBeforeError(service);
986-
verifyGetErrRequest({
979+
verifyGetErrRequestNoErrors({
987980
session,
988981
host,
989-
expected: errorOnNewFileBeforeOldFile ?
990-
[
991-
{ file: fooBar, syntax: [], semantic: [], suggestion: [] },
992-
{ file: foo, syntax: [], semantic: [], suggestion: [] },
993-
] :
994-
[
995-
{ file: foo, syntax: [], semantic: [], suggestion: [] },
996-
{ file: fooBar, syntax: [], semantic: [], suggestion: [] },
997-
],
982+
files: errorOnNewFileBeforeOldFile ?
983+
[fooBar, foo] :
984+
[foo, fooBar],
998985
existingTimeouts: 2
999986
});
1000987
checkProjectAfterError(service);
Collapse file

‎src/testRunner/unittests/tsserver/forceConsistentCasingInFileNames.ts‎

Copy file name to clipboardExpand all lines: src/testRunner/unittests/tsserver/forceConsistentCasingInFileNames.ts
+3-22Lines changed: 3 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -65,13 +65,7 @@ namespace ts.projectSystem {
6565
checkNumberOfProjects(service, { configuredProjects: 1 });
6666
const project = service.configuredProjects.get(tsconfig.path)!;
6767
checkProjectActualFiles(project, [loggerFile.path, anotherFile.path, libFile.path, tsconfig.path]);
68-
verifyGetErrRequest({
69-
host,
70-
session,
71-
expected: [
72-
{ file: loggerFile.path, syntax: [], semantic: [], suggestion: [] }
73-
]
74-
});
68+
verifyGetErrRequestNoErrors({ session, host, files: [loggerFile] });
7569

7670
const newLoggerPath = loggerFile.path.toLowerCase();
7771
host.renameFile(loggerFile.path, newLoggerPath);
@@ -97,14 +91,7 @@ namespace ts.projectSystem {
9791
});
9892

9993
// Check errors in both files
100-
verifyGetErrRequest({
101-
host,
102-
session,
103-
expected: [
104-
{ file: newLoggerPath, syntax: [], semantic: [], suggestion: [] },
105-
{ file: anotherFile.path, syntax: [], semantic: [], suggestion: [] }
106-
]
107-
});
94+
verifyGetErrRequestNoErrors({ session, host, files: [newLoggerPath, anotherFile] });
10895
});
10996

11097
it("when changing module name with different casing", () => {
@@ -130,13 +117,7 @@ namespace ts.projectSystem {
130117
checkNumberOfProjects(service, { configuredProjects: 1 });
131118
const project = service.configuredProjects.get(tsconfig.path)!;
132119
checkProjectActualFiles(project, [loggerFile.path, anotherFile.path, libFile.path, tsconfig.path]);
133-
verifyGetErrRequest({
134-
host,
135-
session,
136-
expected: [
137-
{ file: anotherFile.path, syntax: [], semantic: [], suggestion: [] }
138-
]
139-
});
120+
verifyGetErrRequestNoErrors({ session, host, files: [anotherFile] });
140121

141122
session.executeCommandSeq<protocol.UpdateOpenRequest>({
142123
command: protocol.CommandTypes.UpdateOpen,
Collapse file

‎src/testRunner/unittests/tsserver/helpers.ts‎

Copy file name to clipboardExpand all lines: src/testRunner/unittests/tsserver/helpers.ts
+10Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -837,6 +837,16 @@ namespace ts.projectSystem {
837837
checkAllErrors({ ...request, expectedSequenceId });
838838
}
839839

840+
export interface VerifyGetErrRequestNoErrors extends VerifyGetErrRequestBase {
841+
files: readonly (string | File)[];
842+
}
843+
export function verifyGetErrRequestNoErrors(request: VerifyGetErrRequestNoErrors) {
844+
verifyGetErrRequest({
845+
...request,
846+
expected: request.files.map(file => ({ file, syntax: [], semantic: [], suggestion: [] }))
847+
});
848+
}
849+
840850
export interface CheckAllErrors extends VerifyGetErrRequest {
841851
expectedSequenceId: number;
842852
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.