Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Bug: [unified-signatures] Recommends merging overloads that would then allow incorrect call signatures #10962

Copy link
Copy link
Open
@requinix

Description

@requinix
Issue body actions

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.8.2&fileType=.tsx&code=PTAEAZQQwJwcwK4FsCmA7ALgZwLACgAzBNAYwwEsB7NUAyygCgEoAuUAN0vIBMBufEKACMAWgBM0eMnTYANKAwALFLXIwsGUOSygYKAI4I1KbviKkK1WvQZQ2GmOTRx5AIwD89jI%2BesOXPnwzYjIqGjpGAH0oT1AHJxdQSI8vHzg-Th5QAG9QAF8gvEElFQI1DVAGcgJoNABPJlAkBArXFW4UMrQTMxtiDq6TJn5CPrQBpxN5fs7J7mHCwRFlxbBwcUlEVEwseSh60BJKJFcnKEsaShr451ruUBnB00IQi9BXWFtYm8SUuO8EhkAiNghYwu9PtFvgDnPJktC0kCsrkCngBGA6igoIo4gg9AplKp1JoSPt3ipHnN5EpzgByHSUADWUDq%2BA%2BMAYlO68xG7M541m3OmAqeCzwQA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6WJygM0sQBNaySgHMmAQ3yxoKdFADuY6E0jgwAXxBqgA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3TgwAXxCSgA&tokens=false

Repro Code

function foo(): void;
function foo(a: string, b?: string): void;
//                      ^^^^^^^^^^

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/unified-signatures": "warn",
  },
};

Expected Result

No lint message.

Actual Result

Lint says the two overloads should be combined.

Additional Info

tldr 1: yes, I'm talking about undefined, but (a) there's more to it than simply "NAB because optional == undefined", (b) the edits the lint rule wants me to make will cause code to allow something that wasn't allowed previously, and (c) the rule is reporting on a parameter that it doesn't make sense to report on.

tldr 2: some commentary in the Playground link


These two overloads

function foo(): void;
function foo(a: string, b?: string): void;

are supposed to be combined into

function foo(a?: string, b?: string): void;

The first problem with doing so is that foo(undefined) becomes a valid call signature when it wasn't before. Now yes, I know that a developer should understand that calling foo like that is almost* the same as just foo(), so let's ignore that one. (It's also been brought up in a past report or two about this lint rule.)

The second problem is that foo(undefined, "bar") becomes valid as well. In that case the whole "basically the same" argument backfires because a developer would probably expect this call to be meaningful - the "bar" is meaningful - when the implementation for foo is likely* something simple like if (a === undefined) { ... } and so completely ignoring the value.

(A workaround I've used in other cases has been to enable ignoreDifferentlyNamedParameters, which was really nice because it helped to point out that the meaning of the parameters was actually slightly different between the overloads, but obviously that doesn't work here.)

The main reason I would call this a bug and not simply "lol Javascript" is that Typescript will quite (un)happily complain about calling foo(undefined, string) when using the two overloads but will not complain about it when using the combined one. And I'd think one of the main principles of linting is that following its suggestions/fixes should not make invalid code become valid, right?

There's also a second reason this seems to be a bug: it's triggering on the optional b? parameter and I can't imagine why it should. There's a lot of code to sift through, but I'd speculate that it's detecting "overload with N required args / overload with N+1 required args" and triggering on the last parameter in the signature, when I think it should be accounting for the presence of optional parameters before deciding they can be combined.

I think my point's been made, but here's an interesting thing. Consider this:

function foo(): void;
function foo(a: string): void;
function foo(a: string, b: string): void;

unified-signatures will say 1+2 can combine and 2+3 can combine, so naturally you'd think 1+2+3 could combine too. And currently, if you did merge a pair then the lint rule would agree and say to merge once more. But if that's incorrect and lint rule is fixed, if you merge one pair then you won't get a lint message to merge again. Which could be surprising.

Except it was incorrect. You can safely combine 1+2 or 2+3 but you can't combine 1+2+3. For the reasons I gave earlier, which I'll repeat here as:

// 1+2, 3
function foo(a?: string): void;
function foo(a: string, b: string): void; // foo(undefined, string) not okay

// 1, 2+3
function foo(): void;
function foo(a: string, b?: string): void; // foo(undefined, string) not okay

// 1+2+3
function foo(a?: string, b?: string): void; // foo(undefined, string) suddenly okay

* Additional backstory that may be more relevant than I think: for better or worse, I tend to code the implementation of such overloaded functions like

function foo(...args:
    | []
    | [a: string, b?: string]
): void {
    // branch on args.length

Merging the overloads then breaks their compatibility with the implementation, not just with args but with the code - now I have to support a args.length == 1 && args[0] == undefined case that didn't exist before. Yes, I know, "but what if someone calls the function incorrectly", but to that I say "sorry, not sorry".

Metadata

Metadata

Assignees

No one assigned

    Labels

    awaiting responseIssues waiting for a reply from the OP or another partyIssues waiting for a reply from the OP or another partybugSomething isn't workingSomething isn't workingpackage: eslint-pluginIssues related to @typescript-eslint/eslint-pluginIssues related to @typescript-eslint/eslint-plugin

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions

      Morty Proxy This is a proxified and sanitized view of the page, visit original site.