Skip to content

Commit

Permalink
fix: getFirstToken/getLastToken on comment-only node (#16889)
Browse files Browse the repository at this point in the history
* fix: `getFirstToken`/`getLastToken` on comment-only node

* Remove unnecessary array bound checks

* Split `if`-statement for clarity, remove inaccurate unit test

* Fix for trailing comments in root node
  • Loading branch information
fasttime authored Mar 22, 2023
1 parent 129e252 commit 1fbf118
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 12 deletions.
18 changes: 14 additions & 4 deletions lib/source-code/token-store/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,18 @@ exports.getFirstIndex = function getFirstIndex(tokens, indexMap, startLoc) {
}
if ((startLoc - 1) in indexMap) {
const index = indexMap[startLoc - 1];
const token = (index >= 0 && index < tokens.length) ? tokens[index] : null;
const token = tokens[index];

// If the mapped index is out of bounds, the returned cursor index will point after the end of the tokens array.
if (!token) {
return tokens.length;
}

/*
* For the map of "comment's location -> token's index", it points the next token of a comment.
* In that case, +1 is unnecessary.
*/
if (token && token.range[0] >= startLoc) {
if (token.range[0] >= startLoc) {
return index;
}
return index + 1;
Expand All @@ -77,13 +82,18 @@ exports.getLastIndex = function getLastIndex(tokens, indexMap, endLoc) {
}
if ((endLoc - 1) in indexMap) {
const index = indexMap[endLoc - 1];
const token = (index >= 0 && index < tokens.length) ? tokens[index] : null;
const token = tokens[index];

// If the mapped index is out of bounds, the returned cursor index will point before the end of the tokens array.
if (!token) {
return tokens.length - 1;
}

/*
* For the map of "comment's location -> token's index", it points the next token of a comment.
* In that case, -1 is necessary.
*/
if (token && token.range[1] > endLoc) {
if (token.range[1] > endLoc) {
return index - 1;
}
return index;
Expand Down
28 changes: 28 additions & 0 deletions tests/fixtures/parsers/all-comments-parser.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Similar to the default parser, but considers leading and trailing comments to be part of the root node.
// Some custom parsers like @typescript-eslint/parser behave in this way.

const espree = require("espree");
exports.parse = function(code, options) {
const ast = espree.parse(code, options);

if (ast.range && ast.comments && ast.comments.length > 0) {
const firstComment = ast.comments[0];
const lastComment = ast.comments[ast.comments.length - 1];

if (ast.range[0] > firstComment.range[0]) {
ast.range[0] = firstComment.range[0];
ast.start = firstComment.start;
if (ast.loc) {
ast.loc.start = firstComment.loc.start;
}
}
if (ast.range[1] < lastComment.range[1]) {
ast.range[1] = lastComment.range[1];
ast.end = lastComment.end;
if (ast.loc) {
ast.loc.end = lastComment.loc.end;
}
}
}
return ast;
};
80 changes: 72 additions & 8 deletions tests/lib/source-code/token-store.js
Original file line number Diff line number Diff line change
Expand Up @@ -627,8 +627,8 @@ describe("TokenStore", () => {
const tokenStore = new TokenStore(ast.tokens, ast.comments);

/*
* Actually, the first of nodes is always tokens, not comments.
* But I think this test case is needed for completeness.
* A node must not start with a token: it can start with a comment or be empty.
* This test case is needed for completeness.
*/
const token = tokenStore.getFirstToken(
{ range: [ast.comments[0].range[0], ast.tokens[5].range[1]] },
Expand All @@ -644,8 +644,8 @@ describe("TokenStore", () => {
const tokenStore = new TokenStore(ast.tokens, ast.comments);

/*
* Actually, the first of nodes is always tokens, not comments.
* But I think this test case is needed for completeness.
* A node must not start with a token: it can start with a comment or be empty.
* This test case is needed for completeness.
*/
const token = tokenStore.getFirstToken(
{ range: [ast.comments[0].range[0], ast.tokens[5].range[1]] }
Expand All @@ -654,6 +654,38 @@ describe("TokenStore", () => {
assert.strictEqual(token.value, "c");
});

it("should retrieve the first token if the root node contains a trailing comment", () => {
const parser = require("../../fixtures/parsers/all-comments-parser");
const code = "foo // comment";
const ast = parser.parse(code, { loc: true, range: true, tokens: true, comment: true });
const tokenStore = new TokenStore(ast.tokens, ast.comments);
const token = tokenStore.getFirstToken(ast);

assert.strictEqual(token, ast.tokens[0]);
});

it("should return null if the source contains only comments", () => {
const code = "// comment";
const ast = espree.parse(code, { loc: true, range: true, tokens: true, comment: true });
const tokenStore = new TokenStore(ast.tokens, ast.comments);
const token = tokenStore.getFirstToken(ast, {
filter() {
assert.fail("Unexpected call to filter callback");
}
});

assert.strictEqual(token, null);
});

it("should return null if the source is empty", () => {
const code = "";
const ast = espree.parse(code, { loc: true, range: true, tokens: true, comment: true });
const tokenStore = new TokenStore(ast.tokens, ast.comments);
const token = tokenStore.getFirstToken(ast);

assert.strictEqual(token, null);
});

});

describe("when calling getLastTokens", () => {
Expand Down Expand Up @@ -814,8 +846,8 @@ describe("TokenStore", () => {
const tokenStore = new TokenStore(ast.tokens, ast.comments);

/*
* Actually, the last of nodes is always tokens, not comments.
* But I think this test case is needed for completeness.
* A node must not end with a token: it can end with a comment or be empty.
* This test case is needed for completeness.
*/
const token = tokenStore.getLastToken(
{ range: [ast.tokens[0].range[0], ast.comments[0].range[1]] },
Expand All @@ -831,8 +863,8 @@ describe("TokenStore", () => {
const tokenStore = new TokenStore(ast.tokens, ast.comments);

/*
* Actually, the last of nodes is always tokens, not comments.
* But I think this test case is needed for completeness.
* A node must not end with a token: it can end with a comment or be empty.
* This test case is needed for completeness.
*/
const token = tokenStore.getLastToken(
{ range: [ast.tokens[0].range[0], ast.comments[0].range[1]] }
Expand All @@ -841,6 +873,38 @@ describe("TokenStore", () => {
assert.strictEqual(token.value, "b");
});

it("should retrieve the last token if the root node contains a trailing comment", () => {
const parser = require("../../fixtures/parsers/all-comments-parser");
const code = "foo // comment";
const ast = parser.parse(code, { loc: true, range: true, tokens: true, comment: true });
const tokenStore = new TokenStore(ast.tokens, ast.comments);
const token = tokenStore.getLastToken(ast);

assert.strictEqual(token, ast.tokens[0]);
});

it("should return null if the source contains only comments", () => {
const code = "// comment";
const ast = espree.parse(code, { loc: true, range: true, tokens: true, comment: true });
const tokenStore = new TokenStore(ast.tokens, ast.comments);
const token = tokenStore.getLastToken(ast, {
filter() {
assert.fail("Unexpected call to filter callback");
}
});

assert.strictEqual(token, null);
});

it("should return null if the source is empty", () => {
const code = "";
const ast = espree.parse(code, { loc: true, range: true, tokens: true, comment: true });
const tokenStore = new TokenStore(ast.tokens, ast.comments);
const token = tokenStore.getLastToken(ast);

assert.strictEqual(token, null);
});

});

describe("when calling getFirstTokensBetween", () => {
Expand Down

0 comments on commit 1fbf118

Please sign in to comment.