Skip to content
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

Jump to bottom

fix: getFirstToken/getLastToken on comment-only node #16889

Merged
merged 4 commits into from Mar 22, 2023
Merged

fix: getFirstToken/getLastToken on comment-only node #16889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0fbfe64
fix: `getFirstToken`/`getLastToken` on comment-only node
fasttime Feb 13, 2023
9c7b978
Remove unnecessary array bound checks
fasttime Mar 1, 2023
551ad3b
Split `if`-statement for clarity, remove inaccurate unit test
fasttime Mar 9, 2023
449e894
Fix for trailing comments in root node
fasttime Mar 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 14 additions & 4 deletions lib/source-code/token-store/utils.js
View file
Open in desktop
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];
fasttime marked this conversation as resolved.
Show resolved Hide resolved

// If the mapped index is out of bounds, the returned cursor index will point after the end of the tokens array.
if (!token) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, we could change this condition into index >= tokens.length and move the const token declaration after the if-block.

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];
fasttime marked this conversation as resolved.
Show resolved Hide resolved

// If the mapped index is out of bounds, the returned cursor index will point before the end of the tokens array.
if (!token) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as https://github.com/eslint/eslint/pull/16889/files#r1131283705. index can exceed the array length, but it should never be negative.

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
View file
Open in desktop
@@ -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
View file
Open in desktop
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