Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
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 havesearched for related issues and found none that matched my issue.
- I haveread the FAQ and my problem is not listed.
Playground Link
Repro Code
functionfoo():void;functionfoo(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 aboutundefined
, 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
functionfoo():void;functionfoo(a:string,b?:string):void;
are supposed to be combined into
functionfoo(a?:string,b?:string):void;
The first problem with doing so is thatfoo(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 justfoo()
, 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 thatfoo(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 forfoo
is likely* something simple likeif (a === undefined) { ... }
and so completely ignoring the value.
(A workaround I've used in other cases has been to enableignoreDifferentlyNamedParameters
, 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 callingfoo(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 optionalb?
parameter and I can't imagine why it should. There'sa 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:
functionfoo():void;functionfoo(a:string):void;functionfoo(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 youwon't get a lint message to merge again. Which could be surprising.
Except itwas incorrect. You can safely combine 1+2 or 2+3 but youcan't combine 1+2+3. For the reasons I gave earlier, which I'll repeat here as:
// 1+2, 3functionfoo(a?:string):void;functionfoo(a:string,b:string):void;// foo(undefined, string) not okay// 1, 2+3functionfoo():void;functionfoo(a:string,b?:string):void;// foo(undefined, string) not okay// 1+2+3functionfoo(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
functionfoo(...args:|[]|[a:string,b?:string]):void{// branch on args.length
Merging the overloads then breaks their compatibility with the implementation, not just withargs
but with the code - now I have to support aargs.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".