When calculating spreads, merge empty object into nonempty object to … #34853
Conversation
…produce partial object to reduce complexity
|
@typescript-bot user test this Prooobably not much use in it, but also |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 8957dfe. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 8957dfe. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 8957dfe. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 8957dfe. You can monitor the build here. It should now contribute to this PR's status checks. |
| } | ||
|
|
||
| function tryMergeUnionOfObjectTypeAndEmptyObject(type: UnionType, readonly: boolean): Type | undefined { | ||
| if (type.types.length === 2) { |
There was a problem hiding this comment.
why only cover a pair of types? is that just the most common case?
There was a problem hiding this comment.
Pretty much. {} | Whatever being roughly equivalent to Partial<whatever> is fairly obvious - {} | Foo | Bar is... less obvious?
| return isEmptyObjectType(firstType) ? firstType : isEmptyObjectType(secondType) ? secondType : emptyObjectType; | ||
| } | ||
| if (isEmptyObjectTypeOrSpreadsIntoEmptyObject(firstType)) { | ||
| return getAnonymousPartialType(secondType); |
There was a problem hiding this comment.
why can't we construct a real partial type here? Probably because it would just be expensive and might not exist, right?
There was a problem hiding this comment.
This is just doing the spread operation (skipping privates, only taking spreadable props) here, but also mixing in the Optional flag. I could map it thru partial then feed it back out thru this process, but that's a lot of extra steps (that can fail) for no real gain.
| if (getDeclarationModifierFlagsFromSymbol(prop) & (ModifierFlags.Private | ModifierFlags.Protected)) { | ||
| // do nothing, skip privates | ||
| } | ||
| else if (isSpreadableProperty(prop)) { |
There was a problem hiding this comment.
should isSpreadableProperty exclude private/protected instead of needing a separate check?
There was a problem hiding this comment.
Nope - this is pretty much a clone of the property copying code farther down in getSpreadType, but I don't need to bother collecting skippedPrivateMembers (the other type is already known to be empty), and I set the Optional property on every property.
|
@weswigham Here they are:Comparison Report - master..34853
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Everything's green - DT's failure's are |
|
@sandersn @ahejlsberg you wanna drop by for a code review so we can get this merged? 😄 |
|
The code still looks good to me. I can't remember if we decided to aim for more accuracy in the design meeting. The notes don't say anything but I remember something to that effect being tossed out. |
|
@sandersn IIRC the request was to limit the simplification case to only when the type being merged in has one property, to limit the potential area it affects, which this now does. |
…produce a partial object to reduce complexity of the calculation. This special-cases scenarios like repeated spreads of
false | ObjTypeorObjType | undefinedso they produce a single type (rather than a union of types_ that we can operate on. Specifically, in this PR, we recognize unions similar to{} | {a: string}when spreading, and interpret them as{a?: string}instead (essentially moving the outer union to an inner union, though the constraint is a bit stronger than a nonfresh{}would normally imply). Strictly speaking, this reduces the accuracy of the type logic a bit, but in exchange, improves its performance in pathological scenarios like those described in the linked issue.To some up, while previously,
{} | {a: string}spread with{} | {b: string}would produce{} | {a: string} | {b: string} | {a: string; b: string}, now it produces just{a?: string; b?: string;}instead.Fixes #34599