Skip to content

Handle comment directives in incremental parsing #37632

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.

Sign up for GitHub

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 2 commits into from
Mar 27, 2020
Merged

Handle comment directives in incremental parsing #37632

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bdef043
Add test case that shows failure to handle commentDirectives in incre…
sheetalkamat Mar 26, 2020
ac6d9bb
Handle comment directives in incremental parsing
sheetalkamat Mar 26, 2020
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
61 changes: 59 additions & 2 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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);
}
Comment on lines +7860 to +7865
Copy link
Member

@andrewbranch andrewbranch Mar 27, 2020

Choose a reason for hiding this comment

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

I think concatenate in core can handle this in a single call, similarly to append

Copy link
Member Author

@sheetalkamat sheetalkamat Mar 27, 2020

Choose a reason for hiding this comment

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

That one creates new array if both arrays are !undefined which is what this avoids since anyways we already have new array

}
}

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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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
Loading