Uh oh!
There was an error while loading. Please reload this page.
Combine multiple overloads into a single contextual signature - #42620
Conversation
… a single signature, rather than producing `any` and an error
…ions, rather than the prior exclusively union behavior
Wesley Wigham (weswigham)
commented
Feb 2, 2021
TypeScript Bot (@typescript-bot) perf test this |
Heya Wesley Wigham (@weswigham), I've started to run the parallelized community code test suite on this PR at 429b88f. You can monitor the build here. |
Heya Wesley Wigham (@weswigham), I've started to run the parallelized Definitely Typed test suite on this PR at 429b88f. You can monitor the build here. |
Heya Wesley Wigham (@weswigham), I've started to run the extended test suite on this PR at 429b88f. You can monitor the build here. |
Heya Wesley Wigham (@weswigham), I've started to run the perf test suite on this PR at 429b88f. You can monitor the build here. Update: The results are in! |
TypeScript Bot (typescript-bot)
commented
Feb 2, 2021
Wesley Wigham (@weswigham) Here they are:Comparison Report - master..42620
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Feb 2, 2021
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
Perf is good, rwc looks good (1 change removing an implicit previously, that 2-overload union member would produce no contextual information (since it had two overloads), making the 1-overload union the only union member with a visible contextual signature. Now, both union members produce a signature, they aren't identical, and the contextual signature is dropped entirely. I can preserve our current behavior here (dropping the two-overload's contextual information) without changing the behavior outside of union contextual types, but I'm unsure if we should? Certainly, we feel justified giving no contextual signature for so why should the addition of an overload change that? |
Wesley Wigham (weswigham)
commented
Feb 8, 2021
TypeScript Bot (@typescript-bot) pack this |
Heya Wesley Wigham (@weswigham), I've started to run the tarball bundle task on this PR at 33a5727. You can monitor the build here. |
Hey Wesley Wigham (@weswigham), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
Nathan Shively-Sanders (sandersn)
left a comment
There was a problem hiding this comment.
Looks pretty good. I have a couple of questions and a lot of minor style suggestions.
| === tests/cases/compiler/redefineArray.ts === | ||
| Array = function (n:number, s:string) {return n;}; | ||
| >Array = function (n:number, s:string) {return n;} : (n: number, s: string) => number | ||
| >Array = function (n:number, s:string) {return n;} : <T>(n: number, s: string) => number |
There was a problem hiding this comment.
The addition of the type parameter seems a little odd. Why does a contextual signature give this function a type parameter?
There was a problem hiding this comment.
Array's call signature has type parameters (and multiple overloads, hence why we only now pick it up), so our contextual signature logic says to persist those type parameters onto the inferred signature (so they can be inferred and used at the parameter types). That type parameter just happens to end up unused, since the parameters are all annotated with types that override the inferred ones. Specifically, it has the signatures:
(arrayLength?: number): any[];
<T>(arrayLength: number): T[];
<T>(...items: T[]): T[];
so it used to be that you'd get no contextual signature whatsoever, whereas now the contextual signature is something like <T>(arrayLengthOrItem?: number | T, ...items: T[]): T[]. Technically any time a contextually typed signature has all the parameter types specified, we could probably omit these type parameters. Probably. You can see similar behavior in the types baselines today with only one overload and something like:
// @strictFunctionTypes: falseinterfaceMyCallable{<T>(a: T|number): T[];}constx: MyCallable=function(arg: number){returnnullasany};(strictFunctionTypes has to be off for the assignment to succeed, in both cases). I don't know why, but we also only do this for function expressions and not arrow functions... that's probably indicative of some bug somewhere; but it's a preexisting one.
There was a problem hiding this comment.
OK, I figured it was pre-existing.
| const obj: {field: Rule} = { | ||
| field: { | ||
| validate: (_t, _p, _s) => false, | ||
| normalize: match => match.x, |
There was a problem hiding this comment.
why doesn't this one get a contextual signature too?
There was a problem hiding this comment.
This is a test capturing this change I describe from DT - that comment contains the explanation, and is the bulk of what I was trying to discuss at the design meeting.
| } | ||
| function getUnionOrIntersectionType(types: Type[], kind: TypeFlags | undefined, unionReduction?: UnionReduction) { | ||
| return kind !== TypeFlags.Intersection ? getUnionType(types, unionReduction) : getIntersectionType(types); |
There was a problem hiding this comment.
Is this still correct? It's easier to read.
| returnkind!==TypeFlags.Intersection ? getUnionType(types,unionReduction) : getIntersectionType(types); | |
| returnkind===TypeFlags.Union ? getUnionType(types,unionReduction) : getIntersectionType(types); |
There was a problem hiding this comment.
I intentionally made all the comparisons kind !== TypeFlags.Intersection so that the kind being undefined is synonymous with Union (to preserve the existing structure where there is no kind). Now, I don't intentionally leave kindundefined anywhere, but just in case, I've written the comparisons to be resilient to it (since it is an "optional" signature member, and maybe some plugin author manufactures signatures and send them into the checker, who knows).
Uh oh!
There was an error while loading. Please reload this page.
| results.push(propType); | ||
| } | ||
| return getIntersectionType(results); | ||
| return getIntersectionType(results); // Same result for both union and intersection signatures |
There was a problem hiding this comment.
why does this need to be commented? It didn't surprise me since no other code changed around here, so I think I'm missing the actual surprising thing.
There was a problem hiding this comment.
The actual surprising thing is that nothing changed here. This code path handled union composite signatures... and now we're handling intersection composite signatures in the same way (rather than inverting anything), which is odd. The reason for that is explained in this comment (namely that for compat reasons we intentionally do "the wrong thing" for union signatures here, which happens to be "the right thing" for intersection signatures)
| } | ||
| // A signature `this` type might be a read or a write position... It's very possible that it should be invariant | ||
| // and we should refuse to merge signatures if there are `this` types and they do not match. However, so as to be | ||
| // pessimistic when contextual typing, for now, we'll union the `this` types. |
There was a problem hiding this comment.
what would cause us to change this? (not a big fan of 'for now'-style comments because they don't have enough context with them)
There was a problem hiding this comment.
also: this is basically a parameter, so it should union just like the other parameters.
There was a problem hiding this comment.
so it should union just like the other parameters.
And so we are, the comment is just an inversion of the comment that already exists in combineUnionThisParam which does the opposite.
| return params; | ||
| } | ||
| function combineSignaturesOfIntersectionMembers(left: Signature, right: Signature): Signature { |
There was a problem hiding this comment.
this should probably come before combineIntersectionParameters and combineIntersectionThisParam
There was a problem hiding this comment.
This is in the same order that the very similar combineUnionParameters and combineUnionThisParam are~
| minArgCount, | ||
| (left.flags | right.flags) & SignatureFlags.PropagatingFlags | ||
| ); | ||
| result.compositeKind = TypeFlags.Intersection; |
There was a problem hiding this comment.
stupid efficiency question: Is it more efficient to have createSignature set these? (I'm guessing not, since probably composite signatures (1) are rare (2) have their own internal class.)
There was a problem hiding this comment.
createSignature sets them to undefined already, along with every other optional field on signatures~
| const longest = leftCount >= rightCount ? left : right; | ||
| const shorter = longest === left ? right : left; |
There was a problem hiding this comment.
could they be named longest/shortest or longer/shorter?
There was a problem hiding this comment.
This follows the same terminology we already use in combineUnionParameters, which combinedIntersectionParametersreally closely mirrors.
| const paramName = leftName === rightName ? leftName : | ||
| !leftName ? rightName : | ||
| !rightName ? leftName : | ||
| undefined; |
There was a problem hiding this comment.
uhhhhhhhhh, why not just leftName || rightName? I'm not even sure it's observable, but even if it is, some name is better than arg0.
There was a problem hiding this comment.
Because that's what we merged for combineUnionParameters in #32056 :V
Also, "some name is better than arg0" isn't really true. Names carry semantic meaning, so let's say you have (index: number) => any and (object: Whatever): any - calling the result index where's it's type is number | Whatever has the potential to be misleading. Particularly problematic is when the types are the same, but semantically different, like(length: number): any[] and (firstElement: number): any[] - yeah, number is the type of both, but there're not semantically the same! So calling both "length" or "firstElement" would be dead wrong. Now, I had a super old version of this (like, years old. insert "deja vu" theme here) that concatenated the names in cases like this, to lengthOrFirstElement, but that was shot down as too easily producing names that are unwieldy as unions grow. And thus, the preference for arg0 was born. It carries no information, thus you can draw no (potentially inaccurate) conclusion as to the usage of the parameter.
There was a problem hiding this comment.
If Titian Cernicova-Dragomir (@dragomirtitian) cares enough to fix it, that's good enough for me. =)
Nathan Shively-Sanders (sandersn)
left a comment
There was a problem hiding this comment.
Looks good to me...with the caveat that the minor style issues that stem from combineUnionParameters precedent should be fixed there and here in a followup PR =P
Wesley Wigham (weswigham)
commented
Feb 10, 2021
Daniel Rosenwasser (@DanielRosenwasser) I assume you want this to hit 4.3 and not 4.2 at this point - will you LMK when I should merge? |
SO Question asks about this strange behavior: interfaceOvld{(): void;(x: string): void;}constoNo: Ovld=x=>{}// -> ~~~// error! Type '(x: string) => void ' is not assignable to type 'Ovld'.constok: Ovld=(x?: string)=>{}// okayLooks like the contextual signature isn't what I'd expect in the face of differing parameter list lengths. Wondering if this behavior is intended or unintended as per this PR, and if there should be a new issue filed about it. |
Forrest Hopkins (forresthopkinsa)
commented
Mar 8, 2022
For the sake of search, this also fixes an unmentioned bug (though somewhat similar to the comment from 35641): constobj={a: ['x'],b: ['y'],}asconst;typeObj=typeofobj;typeKey=keyofObj;typeVal=Obj[Key];typeItem=Val[number];constkeys=Object.keys(obj)asKey[];keys.forEach(key=>{obj[key].forEach(item=>{// Parameter 'item' implicitly has an 'any' type. (7006)console.log(item);});for(constitemofobj[key]){// no error, type inferred correctlyconsole.log(item);}}); |
With this PR, we combine multiple overloads into a single contextual signature when
noImplcitAnyis set (for backwards compatibility, since otherwise these positions had typeany). The rules for signature combining are similar to union signature combining, but swapping union/intersection rules where applicable; Either no generics, or identical generic lists, Parameters (andthisparameters) union together, return types (and predicates) intersect (return types/predicates are likely visible via return type inference, so do need sensible rules).With the caveat that this only changes our behavior when
noImplicitAnyis set, this:Fixes#42559
Fixes#42504
Doesn't change #38625 - since the signatures in question have a mix of type parameters and no type parameters.
Doesn't change #35641 - there's no contextual typing involved; generic signature resolution has a similar drawback where it doesn't handle overloads which this PR does not touch.
Fixesthis comment though not the containing issue (which is moreso a dupe/precursor of #35641)