Skip to content

Commit 7f59949

Browse 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

File tree

9 files changed

+307
-96
lines changed

9 files changed

+307
-96
lines changed

‎src/compiler/parser.ts‎

Lines 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);

‎src/testRunner/unittests/incrementalParser.ts‎

Lines 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
}

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

Lines 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);

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

Lines 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,

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

Lines 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)