check-tag-names: targeting JSDocs unnecessary with a type checker (e.g. TypeScript)

[JoshuaKGoldberg]

Motivation

There are quite a few tags that are unnecessary when using TypeScript, such as @abstract, @access/@private, and @class. Other tags are unnecessary when there's no comment, such as @this. It'd be nice to have a lint rule that flags those unnecessary tags.

This actually exists today in DefinitelyTyped-tools, the repo that contains dtslint rules for DefinitelyTyped. Its tracking issue for porting rules from TSLint to ESLint/typescript-eslint is microsoft/DefinitelyTyped-tools#648.

Current behavior

The no-types rule is similar in intent, but only covers unnecessary types. It doesn't cover other tags.

Desired behavior

A port of the dtslint rule no-redundant-jsdoc2: https://github.com/microsoft/DefinitelyTyped-tools/blob/d5429a4fdb0832e89510ad35e44c7b32fa0149da/packages/dtslint/src/rules/noRedundantJsdoc2Rule.ts

Alternatives considered

We could keep the implementation inside DefinitelyTyped-tools... but the plumbing in this project around JSDoc parsing & reporting is quite useful! ❤️ -

This seems like a universally applicable rule for TypeScript projects. I'd want to add it to the recommended-typescript(-error) presets.

[brettz9]

We already support this through the likes of:

      settings: {
        jsdoc: {
          tagNamePreference: {
            abstract: false,
          },
        },
      },

...but as I understand ESLint will not merge this beyond a single depth (i.e., it won't merge it with other jsdoc settings), I think we should add a blacklist option as well, and we can use that within the TypeScript config.

[JoshuaKGoldberg]

Thanks for the quick response! A couple reasons why I don't think that solution is adequate:

• The logic is a little more complex than just settings.jsdoc.tagNamePreference. For example, the context within a file dictates which tags are ok: https://github.com/microsoft/DefinitelyTyped-tools/blob/d5429a4fdb0832e89510ad35e44c7b32fa0149da/packages/dtslint/src/rules/noRedundantJsdoc2Rule.ts#L75
• Indeed, ESLint configurations aren't great at nested configurations. This wouldn't be very ergonomic for users - especially if we want to add configuration options to the rule.

[brettz9]

I'm fine in theory with a separate rule and context-dependent tag allowances, but with this package being oriented to plain JSDoc as well as TypeScript users, I'm not sure we want to be adding a TypeScript dependency. Hopefully the detection you require can be done just by looking at the JSDoc structure and surrounding JS/TS AST?

[JoshuaKGoldberg]

Absolutely! My view of this is just to make a rule that lets you know about tags that are unnecessary in the presence of a type checker. It'd be just as relevant when using e.g. Ezno or Flow.

And thanks! I'll send a PR as soon as I can. ✨

[brettz9]

Cool, though it just occurred to me that we might be able to just add contexts (as we have for other rules and which can be dependent on comment AST as well as TS/JS AST) to the check-tag-names rule. Would that suffice?

[JoshuaKGoldberg]

I'll admit, I'm not super familiar with how contexts work in this repo. But my concern is asking folks to put a lot of configuration effort into existing rules. no-redundant-jsdoc2 has a bunch of logic (in order of source code placement):

• @see, @deprecated, @throws, and @override are always allowed
• "Redundant" ¹ tags are allowed in the following cases, and 🧑‍🔧 will be fix-removed otherwise:
  • The text associated with them is "default"
  • The tag is a root level tag in a .d.ts file
  • The node is inside a declare context
• @template is allowed if there's a comment associated
• "Type" ² tags are always banned with a 🧑‍🔧 fixer
• @return/@returns and @param/@parameter: allowed if there's associated text
  • If there's a type expression and a comment, a 🧑‍🔧 fixer for removing the type expression
  • Always a 🧑‍🔧 fixer for removing the tag

That's a lot of configuration - and specific tailored 🧑‍🔧 fixers! And it's likely to change over time as TypeScript adds more support for JSDoc.

Even if this is all doable in context configuration, it'd be very unwieldy for users to configure it. For example, I'm planning on adding this rule to template-typescript-node-package. But users who use the template aren't going to want dozens of ESLint config file lines for this logic - especially if they want to use the existing rules for their own purposes.

Footnotes

1. @abstract, @access, @class, @constant, @constructs, @default, @enum, @export, @exports, @function, @global, @inherits, @interface, @instance, @member, @method, @memberof, @memberOf, @mixes, @mixin, @module, @name, @namespace, @override, @property, @requires, @static, and @this ↩

2. @public, @private, @protected, @class, @type, @typedef, @readonly, @property, @augments, @implements, @callback, @this, and @enum ↩

[brettz9]

Ok, you make a compelling case, and it is good to see the rules spelled out. (I assume other JSDoc tags which are unrelated to types and have no special meaning for TypeScript would be allowed like @version or whatever.)

I think though that users would still be best served if we kept the behavior within the check-tag-names rule, but with an option which triggers the various effects. Thus users browsing the check-tag-names would discover the option, not be confused by another rule, and there would be no interaction problems between the rules.

[github-actions]

🎉 This issue has been resolved in version 40.2.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀