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 3 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 start 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 -1;
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we return tokens.length - 1 (index of the last token) here? Hardcoded -1 would always mean that there are no tokens before endLoc.

I couldn't construct an example where this would be observable with the default parser because it doesn't include trailing comments in the range of the Program node, but here's an example with @typescript-eslint/parser:

const { RuleTester } = require('eslint');

const rule = {
    meta: {
        docs: { description: 'Require at least one token in the source code' },
        messages: { fail: 'no tokens found' },
    },
    create(context) {
        const sourceCode = context.getSourceCode();
        return {
            Program: node => {
                if (!sourceCode.getFirstToken(node)) {
                    context.report({ node, messageId: 'fail' });
                }
            }
        };
    }
};

new RuleTester().run(
    'must-have-tokens',
    rule,
    {
        valid:
        [
            {
                code: 'foo // comment',
                parser: require.resolve("@typescript-eslint/parser")
            }
        ],
        invalid: [],
    },
);

The expected result is that sourceCode.getFirstToken finds the token foo, but it doesn't because it looks for tokens between indexes 0 and -1.

Copy link
Member Author

@fasttime fasttime Mar 20, 2023 •
edited

Choose a reason for hiding this comment

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

You are perfectly right @mdjermanovic, thanks for pointing me to a proper test case to disproof my incorrect assumption. I've applied the change you suggested and added a unit test to ensure the correct behavior when the root node includes a trailing comment in its range.

It strikes me as a peculiarity of Espree that comments at the beginning and at the end of the source code are not included in the root node, while standalone comments are.


/*
* 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
60 changes: 52 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,28 @@ describe("TokenStore", () => {
assert.strictEqual(token.value, "c");
});

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 +836,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 +853,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 +863,28 @@ describe("TokenStore", () => {
assert.strictEqual(token.value, "b");
});

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