eslint-plugin-jsdoc
eslint-plugin-jsdoc copied to clipboard
gajus
Rule to mark types in JSDoc as used
Motivation
Consider the following code:
import type { ComponentType } from "react";
/** @see {@link ComponentType} */
export type Foo = number;
TypeScript compiler understands that ComponentType is used in JSDoc and hence will not flag it as unused; TS-ESLint, on the other hand, reports @typescript-eslint/no-unused-vars, because it's unaware of JSDoc.
ESLint offers the markVariableAsUsed API that we can leverage to mark this type as used. eslint-plugin-react takes advantage of this to mark React as used when there's JSX. I don't know if it's portable to types, but hopefully it is.
Current behavior
Reports error.
Desired behavior
Should not give @typescript-eslint/no-unused-vars
Alternatives considered
This could also be an extension in TS-ESLint, but AFAIK they never handle JSDoc, so I decided to see if you would consider it first.
I personally prefer to use import() within the types, feeling the importing into code as kind of an anti-pattern, and as per #99 we don't want to continue the buggy support we have for this in no-undefined-types (the bugginess relates to the fact that our approach was to handle comment blocks one-by-one, but for proper unused variable support, it needs to process them all together).
However, as a separate rule, not on by default, I'd be open to a PR, but am not really inclined to work on this, as again, I kind of view it as an anti-pattern, albeit an approach supported by TypeScript and I think meriting accommodation to projects which want to use it.
I see. I do prefer to split the marking types as used feature into a separate rule, because then it would be purely syntactical (detecting @see or @param or similar tags), so we can probably handle the comment blocks one-by-one. I get your point about anti patterns; frankly, I like to use import type in TS files because the JSDoc import() alternative looks quite... ugly. Furthermore, I'm actually unable to get this working:
/** @typedef {import('react').ComponentType} ComponentType */
/** @see {@link ComponentType} */
export type Foo = number;
ComponentType is resolved as any, even when import('react').ComponentType seems to be correctly resolved.
Does this work for you?
/** @typedef {typeof import(https://github.com/facebook/react/).ComponentType} ComponentType */
One admitted problem with the typedef approach though is that there is no means to mark it as local (as suggested per
https://github.com/microsoft/TypeScript/issues/46011#issuecomment-926244613 ). It automatically gets exported as a type if generating a declaration file. Hopefully that will be fixed though, as it's one of two items I think particularly make TypeScript less than practical when trying to use plain JavaScript files (as some projects like ESLint want to do to keep the codebase accessible to more developers, and who do not want to curate a separate type declaration file as it is more likely to go out of sync when the types are not inline to the page) and thus holding me back from switching to TypeScript as our default mode (even for users without a TypeScript parser). @sandersn
If I'm understanding this issue correctly, we would also like this feature as users of the Closure Compiler. We import types that are only used in JsDoc with
const {SomeType} = goog.requireType('Ns.SomeType');
but we have to add /* eslint-disable-next-line no-unused-vars */ above every one of these requireTypes. Since requireType is the Closure method for importing types specifically then I don't think this would be considered an antipattern.
I looked into doing this myself but we are moving towards es modules rather than closure modules so it wasn't worth implementing. But it would probably be useful to anyone using Closure Compiler.
I also would appreciate seeing this implemented. My use-case is also for @link. It works great with VS Code, but without marking the variables as used, other ES Lint plugin grumble.
As discovered in #507 (referenced also in #99), there was an eslint issue which pointed to an issue in marking variables used on program exit (i.e., as when examining comment nodes which are not attached to other AST). As per that issue, we might need to implement our own custom scope analysis in place of markVariableAsUsed.
@brettz9 do you think something changed upstream that might make it more feasible to implement now? If you think this is possible and something you would take a PR for (even as a rule disabled by default) I'd love to take a stab at it 👍
@kraenhansen : While there were some architectural changes on markVariableAsUsed recently (moving it to the SourceCode object away from context), I think the same issue must still exist with that function. You might just search online about scope analysis and ESLint. I haven't worked very much at all with that, but it should I think be possible to do what you need with such analysis.
But if you were to do so, I think you'd have to disable/reimplement the TypeScript no-unused-vars rule since there'd be no common API to rely on. Ideally, this would be fixed at the ESLint level as markVariableAsUsed is intended for this purpose of sharing such data between rules, but maybe implementing it separately would be more stable given the fact that ESLint has been ambivalent about markVariableAsUsed (being as that rules typically don't influence other rules' behavior, and there can be a deterministic problem as I recall with the order in which they're applied).
@brettz9: Looking more at the code, it looks to me as if the inline {@link} tag isn't supported at all.
The parsed comment returned from @es-joy/jsdoccomment's parseComment function via
https://github.com/gajus/eslint-plugin-jsdoc/blob/ec278a292ed343ac875359d66acc4dd81301ec23/src/iterateJsdoc.js#L1141-L1149
returns no tags for the following block comment:
/** This thing uses {@link foo} for something */
It looks like the tokenizers used by the underlying implementation is configurable:
https://github.com/es-joy/jsdoccomment/blob/08b258d7ede45abf58d354683faf1a7391dcf409/src/parseComment.js#L112-L118
... so I wonder if I'd have to implement that as well or if I'm missing something obvious 🤔
No, inline tags are indeed not tokenized. You'd have to get at them through the block or tag descriptions. Same with synonyms, @linkplain and @linkcode, or inline @tutorial.
Just to clarify: Would you consider that the responsibility of @es-joy/jsdoccomment or do you think it'd be reasonable to implement that in eslint-plugin-jsdoc (which would probably be the easiest for me).
@es-joy/jsdcomment is meant as a place for reusable utilities of interest to projects using JSDoc, but it should probably only add properties (e.g., an inlineTags array property with objects like {offset: [start: number, end: number], tag: string, text: string, path: string, linkStyle: "plain"|"markdown"|"pipe"|"space"}) since existing code depends on the current structure. (The linkStyle is following the four listed styles at both https://jsdoc.app/tags-inline-link.html and https://jsdoc.app/tags-inline-tutorial.html .) Another benefit of adding to jsdoccomment is we can build comment AST for it--so rules can target JSDoc blocks using specific inline tags or structures of inline tags.
@brettz9 I started working on this: https://github.com/gajus/eslint-plugin-jsdoc/pull/1058, so perhaps you can assign the issue to me?
Note that this is still draft and I have outstanding tasks (see the PR description): I wanted to get this up quickly in hope of some early feedback to increase the likelihood of an eventual merge.
A good deal of this issue has now been resolved by #1062 and #1065. In short, there is no new rule, but new config (disableReporting) can be used in no-undefined-types to disable the reporting of undefined tags (solely getting the marking of used variables from the rule, as though it were a separate rule). (Config was also added (markVariablesAsUsed) to allow disabling the used-variable marking.)
Note that a variable will still not be marked as used if it is not among the defined types; if someone wants to trust all used types (even potentially misspelled ones), let us know.
Thanks to @kraenhansen 's work, inline tags (such as @link) can now be parsed and are checked for this rule, so together with the above, the issue as reported should now be addressed.
However, as mentioned earlier, the ESLint API on which we still rely, markVariableAsUsed, is error prone (see #507 and https://github.com/eslint/eslint/issues/9810 ), so it may not work to suppress used variable errors in all cases. I'll therefore keep the issue open in case someone can replace this API with a custom solution.
On second thought, since the remaining issue, markVariableAsUsed, is a bug with the ESLint API itself, I think we can close. Note that no-undefined-types is now off by default in recommended TypeScript configs, as TypeScript can handle this.