Fix: Remove all tokens inside comments from tokens array (fixes #422)

[kaicataldo]

This fixes an issue where TypeScript generates tokens for {Type} (for both the brackets and the Identifier Type), causing the indent rule in core to have some false positives. I'm assuming these tokens are generated as part of TypeScript's JS file checking and type hinting from doc blocks.

@eslint/eslint-team As an aside, could we potentially leverage this behavior for our own JSDoc parsing and validation in ESLint core? Would be nice if we could rely on a well-maintained project like TypeScript instead of doctrine, since we've been struggling to maintain that.

Maybe @JamesHenry has some idea of the feasibility of this, given your experience!

This is my first time working with this codebase or really anything TypeScript related, so please do let me know if there are better ways to achieve this or if any assumptions I've made are incorrect.

To illustrate the change, here's a before and after of the tokens and comments arrays generated for the following code:

/**
 * This is a function.
 * @param {String} text
 * @returns {String}
 */
function foo(text) {}

Before this patch:

Tokens:  [ { type: 'Punctuator',
    value: '{',
    range: [ 37, 38 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Identifier',
    value: 'String',
    range: [ 38, 44 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Punctuator',
    value: '}',
    range: [ 44, 45 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Identifier',
    value: 'text',
    range: [ 46, 50 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Punctuator',
    value: '{',
    range: [ 63, 64 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Identifier',
    value: 'String',
    range: [ 64, 70 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Punctuator',
    value: '}',
    range: [ 70, 71 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Keyword',
    value: 'function',
    range: [ 76, 84 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Identifier',
    value: 'foo',
    range: [ 85, 88 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Punctuator',
    value: '(',
    range: [ 88, 89 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Identifier',
    value: 'text',
    range: [ 89, 93 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Punctuator',
    value: ')',
    range: [ 93, 94 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Punctuator',
    value: '{',
    range: [ 95, 96 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Punctuator',
    value: '}',
    range: [ 96, 97 ],
    loc: { start: [Object], end: [Object] } } ]

Comments:  [ { type: 'Block',
    value: '*\n * This is a function.\n * @param {String} text\n * @returns {String}\n ',
    range: [ 0, 75 ],
    loc: { start: [Object], end: [Object] } } ]

After this patch:

Tokens:  [ { type: 'Keyword',
    value: 'function',
    range: [ 76, 84 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Identifier',
    value: 'foo',
    range: [ 85, 88 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Punctuator',
    value: '(',
    range: [ 88, 89 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Identifier',
    value: 'text',
    range: [ 89, 93 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Punctuator',
    value: ')',
    range: [ 93, 94 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Punctuator',
    value: '{',
    range: [ 95, 96 ],
    loc: { start: [Object], end: [Object] } },
  { type: 'Punctuator',
    value: '}',
    range: [ 96, 97 ],
    loc: { start: [Object], end: [Object] } } ]

Comments:  [ { type: 'Block',
    value: '*\n * This is a function.\n * @param {String} text\n * @returns {String}\n ',
    range: [ 0, 75 ],
    loc: { start: [Object], end: [Object] } } ]

[j-f1]

Looks good!

Can you delete package-lock.json.1799798656?

[kaicataldo]

@j-f1 Whoops! Not sure how that got in there. Thanks.

[kaicataldo]

I realized I was overthinking this and that it can can actually be simplified immensely by just not walking comment tokens. This way, comment tokens and their children will never be converted or added to the token list.

My only concern now is whether my comment checking algorithm is correct or not, since I'm still not very familiar with the AST or tokens generated by TypeScript.

[kaicataldo]

Is this the correct check for comment tokens?

[JamesHenry]

TypeScript themselves use this within their private utilities:

function isComment(kind: SyntaxKind): boolean {
  return kind === SyntaxKind.SingleLineCommentTrivia || kind === SyntaxKind.MultiLineCommentTrivia;
}

I feel like we should split the JSDoc checks out into a separate function.

The TypeScript team has these (again not in their public API, but useful utils):

// JSDoc
    /** True if node is of some JSDoc syntax kind. */
    /* @internal */
    export function isJSDocNode(node: Node): boolean {
        return node.kind >= SyntaxKind.FirstJSDocNode && node.kind <= SyntaxKind.LastJSDocNode;
    }

    /** True if node is of a kind that may contain comment text. */
    export function isJSDocCommentContainingNode(node: Node): boolean {
        return node.kind === SyntaxKind.JSDocComment || isJSDocTag(node);
    }

    // TODO: determine what this does before making it public.
    /* @internal */
    export function isJSDocTag(node: Node): boolean {
        return node.kind >= SyntaxKind.FirstJSDocTagNode && node.kind <= SyntaxKind.LastJSDocTagNode;
    }

[kaicataldo]

Friendly ping @JamesHenry @soda0289

[JamesHenry]

Please could you add a unit test that would be affected by these changes? Your same source from the integration test should do the trick!

[JamesHenry]

TypeScript themselves use this within their private utilities:

function isComment(kind: SyntaxKind): boolean {
  return kind === SyntaxKind.SingleLineCommentTrivia || kind === SyntaxKind.MultiLineCommentTrivia;
}

I feel like we should split the JSDoc checks out into a separate function.

The TypeScript team has these (again not in their public API, but useful utils):

// JSDoc
    /** True if node is of some JSDoc syntax kind. */
    /* @internal */
    export function isJSDocNode(node: Node): boolean {
        return node.kind >= SyntaxKind.FirstJSDocNode && node.kind <= SyntaxKind.LastJSDocNode;
    }

    /** True if node is of a kind that may contain comment text. */
    export function isJSDocCommentContainingNode(node: Node): boolean {
        return node.kind === SyntaxKind.JSDocComment || isJSDocTag(node);
    }

    // TODO: determine what this does before making it public.
    /* @internal */
    export function isJSDocTag(node: Node): boolean {
        return node.kind >= SyntaxKind.FirstJSDocTagNode && node.kind <= SyntaxKind.LastJSDocTagNode;
    }

[kaicataldo]

Updated! Two questions:

JSDoc Check

For the JSDoc check, it looks to me like node.kind === SyntaxKind.JSDocComment is sufficient since we stop walking as soon as we encounter the top level JSDocComment node. My inclination is to not add the other utils since the JSDocComment children node types should never be added to the token list since we're outputting ESTree.

Does that make sense or am I missing something? Do you think we should still include those utils? Or are there other downstream consumers who might want to opt into having those tokens available to them (I don't know if Prettier would ever want to get into the business of formatting JSDoc comments, for example)?

Other methods

Do convertToken and getNodeContainer in node-utils also need to be updated to correctly handle JSDoc nodes as well? I believe this change is sufficient for the current use cases for the methods, but can also see the argument for making it less likely to introduce bugs by having these not operate on JSDoc nodes.

Thanks for the review :)

[kaicataldo]

Friendly ping!

[JamesHenry]

I think this is great for now, thanks! I'm going to change it to be a Breaking change though just in case anybody was depending on the old behaviour somehow

[kaicataldo]

Thanks!