Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify substitution type any handling into costruction and instantiation #30592

Merged
merged 6 commits into from
Mar 27, 2019

Conversation

weswigham
Copy link
Member

@weswigham weswigham commented Mar 26, 2019

And clean up conditional types a little to match. The changes to conditional resolvedTrueType/resolvedFalseType aren't strictly required, per sey, but they clean up a lot and reduce complexity. We were choosing not to cache types we'd already computed (for no real reason other than a misunderstanding on my part as to when the combinedMapper was used).

Fixing the creation of substitutes to any exposed an issue where we'd generate infinitely expanding types in our infiniteConstraints.ts test (which previously were never expanded because their assignment to a substituted any simply succeeded) - I had to both introduce type-parameter like circularity guards into getConstraintOfType for conditionals and indexed accesses (it was missing them), and further penalize deep instantiations of conditionals within computeBaseConstraint (to capture the exponential explosion of types instantiated by circularly referential conditionals).

Fixes #30569
Fixes #30489

@weswigham
Copy link
Member Author

@typescript-bot test this & @typescript-bot run dt go go go

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 26, 2019

Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at d5f32bd. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 26, 2019

Heya @weswigham, I've started to run the extended test suite on this PR at d5f32bd. You can monitor the build here. It should now contribute to this PR's status checks.

src/compiler/types.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
src/compiler/checker.ts Show resolved Hide resolved
@weswigham
Copy link
Member Author

The egg-mock and protractor DT failures are also from master, however the react-redux failure I'll need to look into. It looks like we're assigning something a default inference in a place we don't want to be (or at least weren't before) - will need to check it.

@weswigham
Copy link
Member Author

RWC diff is actually nil - test only failed because there's an unaccepted RWC diff on master right now. (Looks like we accepted making the JSON global into a var instead of a const)

@weswigham
Copy link
Member Author

Wow, so the react-redux break is indicative of a IIRC known issue in our subtyping rules. (payload: boolean) => { type: string; payload: boolean; } is not seen as a subtype of (...args: any[]) => any), because the parameter any is not a subtype of boolean (just assignable - the position is contravariant, so never is the needed bound there, not unknown). (Thus we produce an intersection and materialize an any in the return position, creating a type with any's in it which passes a following check which it should not) any not behaving as the correct bound for the context it's in for the subtype relation here is painful.

I can definitely special-case the all-any signature inside signaturesRelatedTo such that an any-ish signature has all other signatures as subtypes, but that's only a partial fix (since any in other contravariant positions will still behave undesirably under the subtype relationship).

@weswigham
Copy link
Member Author

@typescript-bot test this & @typescript-bot run dt - I've confirmed react-redux is now clean locally, but let's check the state of the world with the small change to subtyping.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 26, 2019

Heya @weswigham, I've started to run the extended test suite on this PR at a2fcddc. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 26, 2019

Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at a2fcddc. You can monitor the build here. It should now contribute to this PR's status checks.

@weswigham
Copy link
Member Author

Remaining two DT failures are the two from master - this change is now clean 👍

*/
function isAnySignature(s: Signature) {
return !s.typeParameters && (!s.thisParameter || isTypeAny(getTypeOfParameter(s.thisParameter))) && s.parameters.length === 1 &&
s.hasRestParameter && getTypeOfParameter(s.parameters[0]) === anyArrayType &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also allow for (...args: any) => any.

@weswigham
Copy link
Member Author

@ahejlsberg done.

@typescript-bot test this & @typescript-bot run dt because this is an immediately-before-release fix and we like making sure we're not breaking people~

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 27, 2019

Heya @weswigham, I've started to run the extended test suite on this PR at 34cc427. You can monitor the build here. It should now contribute to this PR's status checks.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Mar 27, 2019

Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at 34cc427. You can monitor the build here. It should now contribute to this PR's status checks.

@weswigham
Copy link
Member Author

DT's just as clean as before, this is good. ❤️

@ahejlsberg can you update your review status?

@RyanCavanaugh RyanCavanaugh merged commit b7881a2 into microsoft:master Mar 27, 2019
@weswigham weswigham deleted the unify-substitution-handling branch March 27, 2019 19:56
const trueTypeNode = typeToTypeNodeHelper(getTrueTypeFromConditionalType(<ConditionalType>type), context);
const falseTypeNode = typeToTypeNodeHelper(getFalseTypeFromConditionalType(<ConditionalType>type), context);
const trueTypeNode = typeToTypeNodeHelper((<ConditionalType>type).trueType, context);
const falseTypeNode = typeToTypeNodeHelper((<ConditionalType>type).falseType, context);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought <angle brackets> casting is meant to be phased out in favour of as casting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The angle brackets were already here~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants