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

Fix conditional type resolution #29338

Merged
merged 3 commits into from
Jan 10, 2019
Merged

Conversation

ahejlsberg
Copy link
Member

This PR improves our logic for determining when to resolve conditional types. We previously used a special "definitely assignable" relation which ignored type parameter constraints, but that didn't correctly distinguish between type parameters referenced in the conditional type vs. type parameters local to contained members such as generic methods. The PR gets rid of this special relation and instead uses the regular assignable relation on a special restrictive form of each type parameter that has no constraint. Now, for a conditional type T extends U ? X : Y, the algorithm we use to determine whether to defer resolution of a conditional type is:

  • we resolve to Y when T is not assignable to U considering all type parameters referenced in T and U related (i.e. T is definitely not assignable to U),
  • otherwise we resolve to X when T is assignable to U considering all type parameters referenced in T and U unrelated (i.e. T is definitely assignable to U),
  • otherwise we defer resolution.

Fixes #23843.

@ahejlsberg ahejlsberg requested a review from weswigham January 10, 2019 00:32
@@ -7413,7 +7409,7 @@ namespace ts {
if (type.root.isDistributive) {
const simplified = getSimplifiedType(type.checkType);
const constraint = simplified === type.checkType ? getConstraintOfType(simplified) : simplified;
if (constraint) {
if (constraint && constraint !== type.checkType) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This particular change fixes an issue where a circularity resulted from the constraint of a conditional type being the conditional type itself. This can happen when a distributive conditional type is applied to an intersection of mapped types. The issue was uncovered because we now correctly defer resolution of conditional types involving generic mapped types.

@@ -9971,7 +9967,7 @@ namespace ts {
if (checkType === wildcardType || extendsType === wildcardType) {
return wildcardType;
}
const checkTypeInstantiable = maybeTypeOfKind(checkType, TypeFlags.Instantiable);
const checkTypeInstantiable = maybeTypeOfKind(checkType, TypeFlags.Instantiable | TypeFlags.GenericMappedType);
Copy link
Member Author

Choose a reason for hiding this comment

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

We previously didn't defer resolution when the check or extends type was a generic mapped type. That definitely was not correct.

@weswigham
Copy link
Member

@typescript-bot test this ❤️

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jan 10, 2019

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

wildcardInstantiation?: Type; // Instantiation with type parameters mapped to wildcard type
permissiveInstantiation?: Type; // Instantiation with type parameters mapped to wildcard type
/* @internal */
restrictiveInstantiation?: Type; // Instantiation with type parameters mapped to unconstrained form
Copy link
Member

Choose a reason for hiding this comment

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

Should permissiveInstantiations and restrictiveInstantiations have pointers back to the original type, this way when we attempt to get them, if we already have one of them we can avoid bothering instantiating a new type (and often creating a new type identity)?

@ahejlsberg ahejlsberg merged commit 52b8256 into master Jan 10, 2019
@ahejlsberg ahejlsberg deleted the fixConditionalTypeResolution branch January 10, 2019 01:18
return type.flags & TypeFlags.TypeParameter ? wildcardType : type;
}

function getRestrictiveTypeParameter(tp: TypeParameter) {
return !tp.constraint ? tp : tp.restrictiveInstantiation || (tp.restrictiveInstantiation = createTypeParameter(tp.symbol));
Copy link
Member

@weswigham weswigham Jan 16, 2019

Choose a reason for hiding this comment

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

This should be

        function getRestrictiveTypeParameter(tp: TypeParameter) {
            return tp.constraint === noConstraintType ? tp : tp.restrictiveInstantiation || (
                tp.restrictiveInstantiation = createTypeParameter(tp.symbol),
                (tp.restrictiveInstantiation as TypeParameter).constraint = noConstraintType,
                tp.restrictiveInstantiation
            );
        }

to align with the original intent - as is, since constraint is a lazily calculated field (and is calculated from the symbol), all this is doing right now is cloning the type parameter (constraint and all!) if it's constraint has been calculated already, or returning it as-is if it hasn't been calculated yet (in both cases you still end up with a potentially constrained type parameter).

Fixing this, however, is not without warts. For example, take this type:

type IsDefinitelyDefined<T extends unknown> = [T] extends [{}] ? true : false;

when compared with an "unconstrained" T it's always true (we have a rule stating that unconstrained type params are assignable to {} for back compat), but the unknown constraint makes it false. Maybe that's a good indicator that the constraint for the restrictiveInstantiation should be set as unknownType instead of noConstraintType?

Copy link
Member

Choose a reason for hiding this comment

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

(I ran into this while tracking down why I couldn't simplify conditional types without causing a stack overflow, and this was a contributor to why one of my possible fixes to that had unintended behavioral changes)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. The restrictive instantiation should be a type parameter with an unknown constraint.

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

Successfully merging this pull request may close these issues.

Bug with conditional types and constraints
3 participants