fix: getFirstToken/getLastToken on comment-only node

[fasttime]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

---

This PR fixes an issue with some SourceCode methods that is causing unexpected behavior when the source code contains no tokens but one or more comments. The SourceCode type is part of the public API and is used by virtually all rules, although the issue described here should not affect any built-in rules.

The problem shows up when sourceCode.getFirstToken() or sourceCode.getLastToken() is called with the Program node (the AST object) of a source code than contains no tokens but one or more comments, i.e. a comment-only file.

The expected behavior is that sourceCode.getFirstToken(ast) or sourceCode.getLastToken(ast) should return null as per documentation. If a filter callback is passed, the callback should never be called.

The actual case is that sourceCode.getFirstToken(ast) returns undefined. If a filter callback is passed, the callback is called with an undefined argument.

My original, incomplete analysis of this problem can be found in issue #16742.

Closes #16742

What changes did you make? (Give an overview)

• Changed the handling of nodes without tokens in https://github.com/eslint/eslint/blob/main/lib/source-code/token-store/utils.js. In practice, this change only impacts Program nodes when the source code contains just comments.
• Added some unit tests to make sure that behavior does not change in other special cases.

Is there anything you'd like reviewers to focus on?

I am open to any suggestions.

[netlify]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	449e894
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/6418ac5863cd9b0008401885

[github-actions]

Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update.

[Rec0iL99]

Don't close. WIP: Looks like @fasttime is still working on this.

[fasttime]

Thanks @Rec0iL99! Actually I'm done with the fix, but I'm not particularly satisfied with my explanation of the problem and the solution. I realize it takes some patience to grasp how this code works and I would like to elaborate on my description and make it more accessible to reviewers.

[fasttime]

If token is undefined, then index is already out of bounds, i.e. index == tokens.length. In this case, the returned cursor index should point after the last token, at tokens.length.
Refs:

• ForwardTokenCursor
• ForwardTokenCommentCursor
• BackwardTokenCursor

[nzakas]

I may be reading this wrong, but it seems like this change doesn't have any effect. I think it's just refactoring the boolean expression without changing the result. Can you double-check?

[fasttime]

When token is null or undefined, the new expression evaluates to true, whereas the old expression would evaluate to a falsy (null or undefined) value. Other than this, the result is the same.

[nzakas]

Ah, right you are. Boolean expressions are hard. :)

So you're saying that token is null if we've already moved past the end of the tokens array?

For clarity, I think I'd prefer to split out null token check into its own if statement and return tokens.length directly instead of adding it to this condition.

[fasttime]

If token is undefined, then tokens is an empty list and index is 0. In this case, the returned cursor index should point before the first token, at -1.
Refs:

• BackwardTokenCursor
• BackwardTokenCommentCursor
• ForwardTokenCursor

[nzakas]

Same comment as above. :)

[fasttime]

#16889 (comment).

[mdjermanovic]

If a node has no tokens, I think getFirstToken should return null. Either way, this doesn't seem to be a case where a node has no tokens, because that token does belong to the TemplateElement node (ast.body[0].expression.quasi.quasis[0] node).

[nzakas]

Agree.

[fasttime]

@mdjermanovic you are correct! That node is not empty. I was fooled by AST explorer that shows the same value for both the start and end indices, probably because it uses an outdated version of Espree.
I added this test to make sure that I wouldn't inadvertently change the existing behavior, but now I think we can safely remove it because it's flawed.

[mdjermanovic]

Same as #16889 (comment)

[fasttime]

Same as #16889 (comment).

[nzakas]

Agree.

[nzakas]

Ah, right you are. Boolean expressions are hard. :)

So you're saying that token is null if we've already moved past the end of the tokens array?

For clarity, I think I'd prefer to split out null token check into its own if statement and return tokens.length directly instead of adding it to this condition.

[fasttime]

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

[fasttime]

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

[fasttime]

Rebased to fix the CI build.

[nzakas]

LGTM, thanks! Just want @mdjermanovic to verify his suggestions.

[mdjermanovic]

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.

[fasttime]

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.

[mdjermanovic]

LGTM, thanks!