Description
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
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".