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.

Sign up for GitHub

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

Jump to bottom

Optimize substitution types #50397

Merged
merged 6 commits into from
Aug 26, 2022
Merged

Optimize substitution types #50397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c09b7dc
Optimize substitution type infrastructure
ahejlsberg Aug 22, 2022
a4a9ede
Accept new baselines
ahejlsberg Aug 22, 2022
e73d268
Preserve instantiated substitution types for type variables
ahejlsberg Aug 22, 2022
f48a923
Restrictive type parameters should have no constraint
ahejlsberg Aug 22, 2022
95ea0ba
Fix issues from top100 test run
ahejlsberg Aug 25, 2022
aed9659
Accept new baselines
ahejlsberg Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 33 additions & 27 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12344,7 +12344,7 @@ namespace ts {
return constraint && getBaseConstraint(constraint);
}
if (t.flags & TypeFlags.Substitution) {
return getBaseConstraint((t as SubstitutionType).substitute);
return getBaseConstraint(getSubstitutionIntersection(t as SubstitutionType));
amcasey marked this conversation as resolved.
Show resolved Hide resolved
}
return t;
}
Expand Down Expand Up @@ -13881,22 +13881,27 @@ namespace ts {
return links.resolvedJSDocType;
}

function getSubstitutionType(baseType: Type, substitute: Type) {
if (substitute.flags & TypeFlags.AnyOrUnknown || substitute === baseType) {
function getSubstitutionType(baseType: Type, constraint: Type) {
if (constraint.flags & TypeFlags.AnyOrUnknown || constraint === baseType ||
Copy link
Member

@DanielRosenwasser DanielRosenwasser Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

constraint === baseType

Is this just when someone writes

T extends T ? TrueType<T> : FalseType<T>

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I presume so. The check was already there.

!isGenericType(baseType) && !isGenericType(constraint)) {
return baseType;
}
const id = `${getTypeId(baseType)}>${getTypeId(substitute)}`;
const id = `${getTypeId(baseType)}>${getTypeId(constraint)}`;
const cached = substitutionTypes.get(id);
if (cached) {
return cached;
}
const result = createType(TypeFlags.Substitution) as SubstitutionType;
result.baseType = baseType;
result.substitute = substitute;
result.constraint = constraint;
substitutionTypes.set(id, result);
return result;
}

function getSubstitutionIntersection(substitutionType: SubstitutionType) {
return getIntersectionType([substitutionType.constraint, substitutionType.baseType]);
}

function isUnaryTupleTypeNode(node: TypeNode) {
return node.kind === SyntaxKind.TupleType && (node as TupleTypeNode).elements.length === 1;
}
Expand Down Expand Up @@ -13941,7 +13946,7 @@ namespace ts {
}
node = parent;
}
return constraints ? getSubstitutionType(type, getIntersectionType(append(constraints, type))) : type;
return constraints ? getSubstitutionType(type, getIntersectionType(constraints)) : type;
}

function isJSDocTypeReference(node: Node): node is TypeReferenceNode {
Expand Down Expand Up @@ -15341,7 +15346,7 @@ namespace ts {
type.flags & TypeFlags.Conditional ? (type as ConditionalType).root.isDistributive && (type as ConditionalType).checkType === typeVariable :
type.flags & (TypeFlags.UnionOrIntersection | TypeFlags.TemplateLiteral) ? every((type as UnionOrIntersectionType | TemplateLiteralType).types, isDistributive) :
type.flags & TypeFlags.IndexedAccess ? isDistributive((type as IndexedAccessType).objectType) && isDistributive((type as IndexedAccessType).indexType) :
type.flags & TypeFlags.Substitution ? isDistributive((type as SubstitutionType).substitute) :
type.flags & TypeFlags.Substitution ? isDistributive((type as SubstitutionType).baseType) && isDistributive((type as SubstitutionType).constraint):
type.flags & TypeFlags.StringMapping ? isDistributive((type as StringMappingType).type) :
false;
}
Expand Down Expand Up @@ -15861,7 +15866,7 @@ namespace ts {
if (type.flags & TypeFlags.Substitution) {
if (!((type as SubstitutionType).objectFlags & ObjectFlags.IsGenericTypeComputed)) {
(type as SubstitutionType).objectFlags |= ObjectFlags.IsGenericTypeComputed |
getGenericObjectFlags((type as SubstitutionType).substitute) | getGenericObjectFlags((type as SubstitutionType).baseType);
getGenericObjectFlags((type as SubstitutionType).baseType) | getGenericObjectFlags((type as SubstitutionType).constraint);
}
return (type as SubstitutionType).objectFlags & ObjectFlags.IsGenericType;
}
Expand Down Expand Up @@ -16073,11 +16078,7 @@ namespace ts {
const objectType = getTypeFromTypeNode(node.objectType);
const indexType = getTypeFromTypeNode(node.indexType);
const potentialAlias = getAliasSymbolForTypeNode(node);
const resolved = getIndexedAccessType(objectType, indexType, AccessFlags.None, node, potentialAlias, getTypeArgumentsForAliasSymbol(potentialAlias));
links.resolvedType = resolved.flags & TypeFlags.IndexedAccess &&
(resolved as IndexedAccessType).objectType === objectType &&
(resolved as IndexedAccessType).indexType === indexType ?
getConditionalFlowTypeOfType(resolved, node) : resolved;
links.resolvedType = getIndexedAccessType(objectType, indexType, AccessFlags.None, node, potentialAlias, getTypeArgumentsForAliasSymbol(potentialAlias));
}
return links.resolvedType;
}
Expand Down Expand Up @@ -17018,9 +17019,9 @@ namespace ts {
}

function getRestrictiveTypeParameter(tp: TypeParameter) {
return tp.constraint === unknownType ? tp : tp.restrictiveInstantiation || (
return !tp.constraint && !getConstraintDeclaration(tp) || tp.constraint === noConstraintType ? tp : tp.restrictiveInstantiation || (
tp.restrictiveInstantiation = createTypeParameter(tp.symbol),
(tp.restrictiveInstantiation as TypeParameter).constraint = unknownType,
(tp.restrictiveInstantiation as TypeParameter).constraint = 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 think you mentioned it in person, but remind me again: Why does this need to be noConstraintType? AFAIK, noConstraintTypeshould mostly just be used as a marker to makegetBaseConstraintOfTypereturnundefined, but a type parameter constrained to unknownand a type parameter without a constraint are supposed to be functionally identical nowadays. So if this change was required somewhere, I think it implies something is subtly broken. In any case, we should probably makenoConstraintTypeanunknown` variant at some point (since it's still an empty object), even if it's never supposed to actually be used as a type.

Copy link
Member Author

@ahejlsberg ahejlsberg Aug 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The one difference I ran into is in getConstraintOfDistributiveConditionalType. It does nothing for type parameters declared without a constraint, but applies a distributive conditional type to a declared type parameter constraint, even if that constraint is unknown. Changing this would definitely be a breaking change.

Making matters worse, for a restrictive type parameter we used to jam in an unknown constraint even for a type parameter declared without a constraint--which effectively changed the observed constraint for a distributive conditional type.

Copy link
Member

Choose a reason for hiding this comment

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

Ew, icky. Distributive constraints behaving differently for T vs T extends unknown is definitely pretty bad - do we know which direction's behavior is better? Given the direction of this PR, I'm led to think we should adopt the unconstrained T behavior for a T extends unknown.

tp.restrictiveInstantiation
);
}
Expand Down Expand Up @@ -17407,17 +17408,18 @@ namespace ts {
return getConditionalTypeInstantiation(type as ConditionalType, combineTypeMappers((type as ConditionalType).mapper, mapper), aliasSymbol, aliasTypeArguments);
}
if (flags & TypeFlags.Substitution) {
const maybeVariable = instantiateType((type as SubstitutionType).baseType, mapper);
if (maybeVariable.flags & TypeFlags.TypeVariable) {
return getSubstitutionType(maybeVariable as TypeVariable, instantiateType((type as SubstitutionType).substitute, mapper));
const newBaseType = instantiateType((type as SubstitutionType).baseType, mapper);
const newConstraint = instantiateType((type as SubstitutionType).constraint, mapper);
// A substitution type originates in the true branch of a conditional type and can be resolved
// to just the base type in the same cases as the conditional type resolves to its true branch
// (because the base type is then known to satisfy the constraint).
if (newBaseType.flags & TypeFlags.TypeVariable && isGenericType(newConstraint)) {
return getSubstitutionType(newBaseType, newConstraint);
}
else {
const sub = instantiateType((type as SubstitutionType).substitute, mapper);
if (sub.flags & TypeFlags.AnyOrUnknown || isTypeAssignableTo(getRestrictiveInstantiation(maybeVariable), getRestrictiveInstantiation(sub))) {
return maybeVariable;
}
return sub;
if (newConstraint.flags & TypeFlags.AnyOrUnknown || isTypeAssignableTo(getRestrictiveInstantiation(newBaseType), getRestrictiveInstantiation(newConstraint))) {
return newBaseType;
}
return newBaseType.flags & TypeFlags.TypeVariable ? getSubstitutionType(newBaseType, newConstraint) : getIntersectionType([newConstraint, newBaseType]);
}
return type;
}
Expand Down Expand Up @@ -18456,7 +18458,7 @@ namespace ts {
const t = isFreshLiteralType(type) ? (type as FreshableType).regularType :
getObjectFlags(type) & ObjectFlags.Reference ? (type as TypeReference).node ? createTypeReference((type as TypeReference).target, getTypeArguments(type as TypeReference)) : getSingleBaseForNonAugmentingSubtype(type) || type :
type.flags & TypeFlags.UnionOrIntersection ? getNormalizedUnionOrIntersectionType(type as UnionOrIntersectionType, writing) :
type.flags & TypeFlags.Substitution ? writing ? (type as SubstitutionType).baseType : (type as SubstitutionType).substitute :
type.flags & TypeFlags.Substitution ? writing ? (type as SubstitutionType).baseType : getSubstitutionIntersection(type as SubstitutionType) :
type.flags & TypeFlags.Simplifiable ? getSimplifiedType(type, writing) :
type;
if (t === type) return t;
Expand Down Expand Up @@ -19539,7 +19541,11 @@ namespace ts {
}
}
if (sourceFlags & TypeFlags.Substitution) {
return isRelatedTo((source as SubstitutionType).substitute, (target as SubstitutionType).substitute, RecursionFlags.Both, /*reportErrors*/ false);
if (result = isRelatedTo((source as SubstitutionType).baseType, (target as SubstitutionType).baseType, RecursionFlags.Both, /*reportErrors*/ false)) {
if (result &= isRelatedTo((source as SubstitutionType).constraint, (target as SubstitutionType).constraint, RecursionFlags.Both, /*reportErrors*/ false)) {
return result;
}
}
}
if (!(sourceFlags & TypeFlags.Object)) {
return Ternary.False;
Copy link
Member

Choose a reason for hiding this comment

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

This is a meaningful break in behavior, right? Previously type FilterNever<T> = T extends never ? T : never would have allowed FilterNever<A> to be assignable to FilterNever<B> (where A and B are both type parameters), since T & never is just never, and eliminated the need to relate the type parameter part, no?

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 is in the code path for identityRelation, so hardly matters. In your particular example the types wouldn't relate anyway because the type parameters aren't identical.

Copy link
Member

Choose a reason for hiding this comment

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

That's what would technically make it a break, right? Previously this related substitutions only, so A sub A & never and B sub B & never both looked identically like never?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's true. But they definitely shouldn't be related, so I guess this fixes a bug as well.

Expand Down Expand Up @@ -22677,7 +22683,7 @@ namespace ts {
}
else if (source.flags & TypeFlags.Substitution) {
inferFromTypes((source as SubstitutionType).baseType, target);
inferWithPriority((source as SubstitutionType).substitute, target, InferencePriority.SubstituteSource); // Make substitute inference at a lower priority
inferWithPriority(getSubstitutionIntersection(source as SubstitutionType), target, InferencePriority.SubstituteSource); // Make substitute inference at a lower priority
}
else if (target.flags & TypeFlags.Conditional) {
invokeOnce(source, target, inferToConditionalType);
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/tracing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ namespace ts { // eslint-disable-line one-namespace-per-file
const substitutionType = type as SubstitutionType;
substitutionProperties = {
substitutionBaseType: substitutionType.baseType?.id,
substituteType: substitutionType.substitute?.id,
constraintType: substitutionType.constraint?.id,
amcasey marked this conversation as resolved.
Show resolved Hide resolved
};
}

Expand Down
4 changes: 2 additions & 2 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6077,8 +6077,8 @@ namespace ts {
// types disappear upon instantiation (just like type parameters).
export interface SubstitutionType extends InstantiableType {
objectFlags: ObjectFlags;
baseType: Type; // Target type
substitute: Type; // Type to substitute for type parameter
baseType: Type; // Target type
constraint: Type; // Constraint that target type is known to satisfy
}

/* @internal */
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/api/tsserverlibrary.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2832,7 +2832,7 @@ declare namespace ts {
export interface SubstitutionType extends InstantiableType {
objectFlags: ObjectFlags;
baseType: Type;
substitute: Type;
constraint: Type;
}
export enum SignatureKind {
Call = 0,
Expand Down
2 changes: 1 addition & 1 deletion tests/baselines/reference/api/typescript.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2832,7 +2832,7 @@ declare namespace ts {
export interface SubstitutionType extends InstantiableType {
objectFlags: ObjectFlags;
baseType: Type;
substitute: Type;
constraint: Type;
}
export enum SignatureKind {
Call = 0,
Expand Down