Skip to content

Commit

Permalink
Handle comment directives in incremental parsing (#37632)
Browse files Browse the repository at this point in the history 
* Add test case that shows failure to handle commentDirectives in incremental parsing
Testcase for #37536

* Handle comment directives in incremental parsing
Fixes #37536
  • Loading branch information
@sheetalkamat
sheetalkamat committed Mar 27, 2020
1 parent 0f3a9d4 commit 7f59949
Show file tree
Hide file tree
Showing 9 changed files with 307 additions and 96 deletions.
61 changes: 59 additions & 2 deletions src/compiler/parser.ts
View file
Expand Up @@ -7752,7 +7752,6 @@ namespace ts {
const incrementalSourceFile = <IncrementalNode><Node>sourceFile;
Debug.assert(!incrementalSourceFile.hasBeenIncrementallyParsed);
incrementalSourceFile.hasBeenIncrementallyParsed = true;

const oldText = sourceFile.text;
const syntaxCursor = createSyntaxCursor(sourceFile);

Expand Down Expand Up @@ -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(<IncrementalNodeArray>element);
Expand Down
153 changes: 153 additions & 0 deletions src/testRunner/unittests/incrementalParser.ts
View file
Expand Up @@ -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);
}
});
});
}
23 changes: 5 additions & 18 deletions src/testRunner/unittests/tsserver/configuredProjects.ts
View file
Expand Up @@ -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 ^", () => {
Expand Down Expand Up @@ -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);
Expand Down
25 changes: 3 additions & 22 deletions src/testRunner/unittests/tsserver/forceConsistentCasingInFileNames.ts
View file
Expand Up @@ -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);
Expand All @@ -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", () => {
Expand All @@ -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<protocol.UpdateOpenRequest>({
command: protocol.CommandTypes.UpdateOpen,
Expand Down
10 changes: 10 additions & 0 deletions src/testRunner/unittests/tsserver/helpers.ts
View file
Expand Up @@ -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;
}
Expand Down

0 comments on commit 7f59949

Please sign in to comment.