feat(eslint-plugin): [unified-signatures] support ignoring overload signatures with different JSDoc comments #10781

ronami · 2025-02-04T11:34:22Z

PR Checklist

Addresses an existing open issue: fixes Feature: [unified-signatures] Option to not report when signature has different JSDoc #10520
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This PR attempts to tackle #10520 and adds the ignoreOverloadsWithDifferentJSDoc option. When enabled, the following is considered valid:

/**
 * This signature does something.
 */
declare function f(x: number): void;

/**
 * This signature does something else.
 */
declare function f(x: string): void;

Some thoughts/notes:

I've only considered JSDoc comments that annotate a function signature, not ones that annotate its params (or return type annotation, see below), so the following is reported, even though it has similar issues with unifying two signatures with different jsdoc comments:
```
declare function f(x: {
  /** @deprecated */
  a: boolean;
}): void;
declare function f(x: { a: number }): void;
```
This is tricky, as the following overloads, for example, can be unified (and should be reported):
```
declare function f(x: {
  /** @deprecated */
  a: boolean;
}): void;
declare function f(x: { b: number }): void;
```
I'm a bit unsure if this should be checked or not, as it wasn't described on the issue. I'd love to hear opinions on this, and would be happy to add a check for this (although this seems somewhat complex 🙈).
I didn't add any checks to an annotated return type, as return type annotations are checked by comparing the source code text.

So the following signatures already count as unique (playground link):
```
declare function f(x: boolean): {
  /** @deprecated */
  a: boolean;
};
declare function f(x: number): {
  a: number;
};
```
Unrelated to the change in the PR: Considering the paragraph above, it does have some false positives, as the following should be reported but doesn't:
```
declare function f(x: boolean): {
  a: number;
};
declare function f(x: number): { a: number };
```
This also includes random, non-jsdoc comments, spaces, line-breaks, etc (playground link). Should I open a separate issue on this?

I think it can at least be compared with something similar to isNodeEqual so spacing/line-breaks don't affect it.

typescript-eslint · 2025-02-04T11:34:25Z

Thanks for the PR, @ronami!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

netlify · 2025-02-04T11:34:41Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`ec9a1ea`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/67be3b1423f4f70008696659
😎 Deploy Preview	https://deploy-preview-10781--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 79 (🔴 down 19 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

ronami · 2025-02-04T11:36:35Z

packages/eslint-plugin/src/rules/unified-signatures.ts

+    /**
+     * @returns the first valid JSDoc comment annotating `node`
+     */
+    function getJSDocCommentForNode(
+      node: TSESTree.Node,
+    ): TSESTree.Comment | undefined {
+      return context.sourceCode
+        .getCommentsBefore(node)
+        .reverse()
+        .find(comment => isJSDocComment(comment));
+    }


It seems only the first jsdoc comment affects a function signature (playground link), and non-jsdoc comments are ignored (playground link).

nx-cloud · 2025-02-04T11:39:12Z

View your CI Pipeline Execution ↗ for commit ec9a1ea.

Command	Status	Duration	Result
`nx run-many --target=build --exclude website --...`	✅ Succeeded	3s	View ↗
`nx run-many --target=clean`	✅ Succeeded	11s	View ↗

☁️ Nx Cloud last updated this comment at 2025-02-25 22:06:06 UTC

codecov · 2025-02-04T12:08:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.29%. Comparing base (c910ac1) to head (ec9a1ea).
Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10781      +/-   ##
==========================================
+ Coverage   87.27%   87.29%   +0.01%     
==========================================
  Files         453      453              
  Lines       15833    15840       +7     
  Branches     4639     4643       +4     
==========================================
+ Hits        13819    13827       +8     
  Misses       1658     1658              
+ Partials      356      355       -1

Flag	Coverage Δ
unittest	`87.29% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ages/eslint-plugin/src/rules/unified-signatures.ts	`87.03% <100.00%> (+1.23%)`	⬆️

LukeAbby · 2025-02-06T04:38:32Z

Not a maintainer so just a random guy's two cents.

Do you think it'd be worth considering a sibling option to NOT suggest unifying two overloads when one is deprecated and the other isn't? This is actually what the original issue's code example was. The issue I see with doing it based off of any JSDoc change is that if you dutifully document the optional parameter (but leave the rest of the JSDoc identical) then it'll be missed.

JoshuaKGoldberg

LGTM, another very thorough investigation 🔥. Just requesting changes on simplifying the block-comment/JSDoc checking.

JoshuaKGoldberg · 2025-02-17T13:11:03Z

packages/eslint-plugin/src/rules/unified-signatures.ts

+    return false;
+  }
+
+  const lines = comment.value.split(LINEBREAK_MATCHER);


[Feature] I tried replacing the rest of this function with return true and only two tests failed:

/** * This signature does something. **/

/* * This signature does something. */

ACK that these are technically not correctly formed for JSDocs, but TypeScript in VS Code still shows them well:

Proposal: let's just go off of whether it's a Block?

Oh, nice catch!

I could make TypeScript in VSCode to show this in the first example, but I couldn't do it in the second one. Reverse engineering this a bit, it seems the comment has to start with at least two `*'s (playground link):

// seem to show when the function is hovered /** * This signature does something. */ declare function f(x: number): unknown; //doesn't seem to show when the function is hovered /* * This signature does something. */ declare function g(x: number): unknown;

I agree though, that matching TypeScript's behavior would probably be pretty complex (and possibly not worth the extra complexity), especially since invalid jsdoc also applies.

Also, for comments that aren't recognized by TypeScript (like the second example), I think it may be better not to report them since it's most likely a typo on the developer's part (or at least I assume so).

I've updated my PR to match your suggestion, thanks! 🚀

JoshuaKGoldberg · 2025-02-17T13:13:56Z

This also includes random, non-jsdoc comments, spaces, line-breaks, etc (playground link). Should I open a separate issue on this?
I think it can at least be compared with something similar to isNodeEqual so spacing/line-breaks don't affect it.

Agreed, this seems like a separate bug to me.

JoshuaKGoldberg

@jfet97

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.24.0 | 8.26.0 | | npm | @typescript-eslint/parser | 8.24.0 | 8.26.0 | ## [v8.26.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8260-2025-03-03) ##### 🚀 Features - **eslint-plugin:** \[unified-signatures] support ignoring overload signatures with different JSDoc comments ([#10781](typescript-eslint/typescript-eslint#10781)) - **eslint-plugin:** \[explicit-module-boundary-types] add an option to ignore overload implementations ([#10889](typescript-eslint/typescript-eslint#10889)) - **eslint-plugin:** \[no-unused-var] handle implicit exports in declaration files ([#10714](typescript-eslint/typescript-eslint#10714)) - support TypeScript 5.8 ([#10903](typescript-eslint/typescript-eslint#10903)) - **eslint-plugin:** \[no-unnecessary-type-parameters] special case tuples and parameter location arrays as single-use ([#9536](typescript-eslint/typescript-eslint#9536)) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-type-assertion] handle unknown ([#10875](typescript-eslint/typescript-eslint#10875)) - **eslint-plugin:** \[no-invalid-void-type] report `accessor` properties with an invalid `void` type ([#10864](typescript-eslint/typescript-eslint#10864)) - **eslint-plugin:** \[unified-signatures] does not differentiate truly private methods ([#10806](typescript-eslint/typescript-eslint#10806)) ##### ❤️ Thank You - Andrea Simone Costa [@jfet97](https://github.com/jfet97) - Dirk Luijk [@dirkluijk](https://github.com/dirkluijk) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) - Yukihiro Hasegawa [@y-hsgw](https://github.com/y-hsgw) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. ## [v8.25.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8250-2025-02-24) ##### 🚀 Features - **eslint-plugin:** \[no-misused-spread] add suggestions ([#10719](typescript-eslint/typescript-eslint#10719)) ##### 🩹 Fixes - **eslint-plugin:** \[prefer-nullish-coalescing] report on chain expressions in a ternary ([#10708](typescript-eslint/typescript-eslint#10708)) - **eslint-plugin:** \[no-deprecated] report usage of deprecated private identifiers ([#10844](typescript-eslint/typescript-eslint#10844)) - **eslint-plugin:** \[unified-signatures] handle getter-setter ([#10818](typescript-eslint/typescript-eslint#10818)) ##### ❤️ Thank You - Olivier Zalmanski [@OlivierZal](https://github.com/OlivierZal) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website. ## [v8.24.1](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8241-2025-02-17) ##### 🩹 Fixes - **eslint-plugin:** \[class-methods-use-this] check `accessor` methods with a function initializer ([#10796](typescript-eslint/typescript-eslint#10796)) - **eslint-plugin:** \[no-misused-promises] don't report on `static` `accessor` properties ([#10814](typescript-eslint/typescript-eslint#10814)) - **eslint-plugin:** \[no-deprecated] don't report on deprecated `accessor` property declaration ([#10813](typescript-eslint/typescript-eslint#10813)) - **eslint-plugin:** \[explicit-member-accessibility] check `accessor` class properties for missing accessibility modifier ([#10805](typescript-eslint/typescript-eslint#10805)) - **eslint-plugin:** \[explicit-module-boundary-types] check `accessor` class properties with a function initializer ([#10804](typescript-eslint/typescript-eslint#10804)) - **eslint-plugin:** \[prefer-return-this-type] check `accessor` properties with a function initializer ([#10794](typescript-eslint/typescript-eslint#10794)) - **eslint-plugin:** \[consistent-generic-constructors] check `accessor` class properties ([#10789](typescript-eslint/typescript-eslint#10789)) - **eslint-plugin:** \[no-unsafe-assignment] report on an `any` value assigned as an initializer of an `accessor` property ([#10785](typescript-eslint/typescript-eslint#10785)) - **eslint-plugin:** \[no-unnecessary-template-expression] ignore enum and enum members ([#10782](typescript-eslint/typescript-eslint#10782)) - **eslint-plugin:** \[no-inferrable-types] handle accessor ([#10780](typescript-eslint/typescript-eslint#10780)) ##### ❤️ Thank You - Ronen Amiel - YeonJuan You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

initial implementation

30aaac3

ronami commented Feb 4, 2025

View reviewed changes

add a test for interfaces

b9d0b2a

ronami changed the title ~~feat(unified-signatures): support ignoring overload signatures with different JSDoc comments~~ feat(eslint-plugin): [unified-signatures] support ignoring overload signatures with different JSDoc comments Feb 4, 2025

ronami changed the title ~~feat(eslint-plugin): [unified-signatures] support ignoring overload signatures with different JSDoc comments~~ feat(eslint-plugin): [unified-signatures] support ignoring overload signatures with different JSDoc comments Feb 4, 2025

ronami marked this pull request as ready for review February 4, 2025 12:17

remove some redundant 'export function ...' tests

cabc56c

JoshuaKGoldberg requested changes Feb 17, 2025

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Feb 17, 2025

ronami added 2 commits February 25, 2025 23:38

merge main branch

4bec06c

check for block comments instead of a heuristic to check jsdoc comments

ec9a1ea

ronami requested a review from JoshuaKGoldberg February 25, 2025 22:08

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Feb 25, 2025

JoshuaKGoldberg approved these changes Mar 3, 2025

View reviewed changes

JoshuaKGoldberg merged commit 02d9d73 into typescript-eslint:main Mar 3, 2025
89 checks passed

ronami deleted the unified-signatures-allow-jsdoc-comments branch March 3, 2025 20:21

github-actions bot locked as resolved and limited conversation to collaborators Mar 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [unified-signatures] support ignoring overload signatures with different JSDoc comments #10781

feat(eslint-plugin): [unified-signatures] support ignoring overload signatures with different JSDoc comments #10781

ronami commented Feb 4, 2025 •

edited

Loading

typescript-eslint bot commented Feb 4, 2025

netlify bot commented Feb 4, 2025 •

edited

Loading

ronami Feb 4, 2025

nx-cloud bot commented Feb 4, 2025 •

edited

Loading

codecov bot commented Feb 4, 2025 •

edited

Loading

LukeAbby commented Feb 6, 2025

JoshuaKGoldberg left a comment

JoshuaKGoldberg Feb 17, 2025

ronami Feb 25, 2025 •

edited

Loading

JoshuaKGoldberg commented Feb 17, 2025

JoshuaKGoldberg left a comment

feat(eslint-plugin): [unified-signatures] support ignoring overload signatures with different JSDoc comments #10781

feat(eslint-plugin): [unified-signatures] support ignoring overload signatures with different JSDoc comments #10781

Conversation

ronami commented Feb 4, 2025 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Feb 4, 2025

netlify bot commented Feb 4, 2025 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

ronami Feb 4, 2025

Choose a reason for hiding this comment

nx-cloud bot commented Feb 4, 2025 • edited Loading

codecov bot commented Feb 4, 2025 • edited Loading

Codecov Report

LukeAbby commented Feb 6, 2025

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Feb 17, 2025

Choose a reason for hiding this comment

ronami Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

JoshuaKGoldberg commented Feb 17, 2025

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

ronami commented Feb 4, 2025 •

edited

Loading

netlify bot commented Feb 4, 2025 •

edited

Loading

nx-cloud bot commented Feb 4, 2025 •

edited

Loading

codecov bot commented Feb 4, 2025 •

edited

Loading

ronami Feb 25, 2025 •

edited

Loading