fix(eslint-plugin): [unified-signatures] exempt this from optional parameter overload check
#11005
Conversation
|
Thanks for the PR, @jedwards1211! 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
View your CI Pipeline Execution ↗ for commit f4689e4.
☁️ Nx Cloud last updated this comment at |
jedwards1211
force-pushed
the
fix-10982
branch
2 times, most recently
from
ea91923 to
2532ff3
Compare
jedwards1211
changed the title
this from optional parameter overload checkthis from optional parameter overload check in unified-signatures
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11005 +/- ##
=======================================
Coverage 88.03% 88.03%
=======================================
Files 470 470
Lines 16801 16804 +3
Branches 4745 4747 +2
=======================================
+ Hits 14790 14793 +3
Misses 1664 1664
Partials 347 347
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jedwards1211
changed the title
this from optional parameter overload check in unified-signaturesthis from optional parameter overload check in unified-signatures
jedwards1211
changed the title
this from optional parameter overload check in unified-signaturesthis from optional parameter overload check
JoshuaKGoldberg
added
the
1 approval
| @@ -310,6 +314,12 @@ export default createRule<Options, MessageIds>({ | |||
| const shorter = sig1.length < sig2.length ? sig1 : sig2; | |||
| const shorterSig = sig1.length < sig2.length ? a : b; | |||
|
|
|||
| // If one signature has explicit this type and another doesn't, they can't | |||
| // be unified. | |||
| if (isThisParam(sig1[0]) !== isThisParam(sig2[0])) { | |||
There was a problem hiding this comment.
Question -
| if (isThisParam(sig1[0]) !== isThisParam(sig2[0])) { | |
| if (isThisParam(sig1[0]) || isThisParam(sig2[0])) { |
I was kind of thinking we would also want to exempt cases where they both include this, since this: Foo | void isn't equivalent to the overload form (playground). Is that not the case?
There was a problem hiding this comment.
Wouldn't that break cases that should be flagged for merge like this?
function foo(this: SomeClass)
function foo(this: SomeClass, x: number)There was a problem hiding this comment.
Hmm but using different logic, we could also exempt cases where some but not all signatures have this: void without breaking the above example
There was a problem hiding this comment.
@kirkwaiblinger btw does returnsVoid (especially calling it with a function argument) do something tricky in your example that I'm not aware of? It seems to me instead of
overloadedThisVoid.call(returnsVoid(() => 'lol'));you could just do
overloadedThisVoid.call((() => {})())Or more explicitly
overloadedThisVoid.call(1 as any as void)Both of which trigger the same TypeScript error.
There was a problem hiding this comment.
Hmm, yeah, I guess it would come down to special-casing around void, since, in addition to your example, which I agree with, we'd also want to consider:
function foo(this: Foo, x: number)
function foo(this: Bar, x: number)can still be
function foo(this: Foo | Bar, x: number)Wonder if special-casing around void is even relevant for non-this parameters too?
function foo(x: void);
function foo(x: number);Would have to play around to figure that one out.
There was a problem hiding this comment.
It appears that TS picks the this type from the last overload when it comes to .call, lol.
function foo(this: string): void;
function foo(this: number): void;
function foo() {}
foo.call(1)
foo.call('a') // Argument of type 'string' is not assignable to parameter of type 'number'.(2345)
function bar(this: number): void;
function bar(this: string | boolean): void;
function bar() {}
bar.call(1) // Argument of type 'number' is not assignable to parameter of type 'string | boolean'.(2345)
bar.call('a')
bar.call(true)
There was a problem hiding this comment.
ah right, the unsoundness of TS function typing rears its head again
Do you know why TS doesn't treat embedded void expressions as an error the way no-confusing-void-expression does? Would be curious to know the explicit rationale from the TS team.
There was a problem hiding this comment.
It appears that TS picks the
thistype from the last overload when it comes to.call, lol.
Huh. 👀 . Well - that the .bind()/.call()/.apply() types are going over my head at this point 😆
There was a problem hiding this comment.
possibly related - microsoft/TypeScript#14107, microsoft/TypeScript#33815, microsoft/TypeScript#38353... they seem to have more to do with teh parameter types than the this type, but anyway. Let's not stress too much about bind/call/apply type behavior, I suppose. 🤷
There was a problem hiding this comment.
Do you know why TS doesn't treat embedded void expressions as an error the way
no-confusing-void-expressiondoes? Would be curious to know the explicit rationale from the TS team.
Not off the top of my head, but I'm sure you'll get (or more likely be pointed to) a thorough answer if you ask in the TS discord.
PR Checklist
Overview
In
signaturesDifferByOptionalOrRestParameter, checks the first parameter of both signatures. If one is athisparameter but not the other, thenreturn undefinedinstead ofkind: 'extra-parameter', so that no error is flagged.