8000 Improve narrowing of generic types in control flow analysis by ahejlsberg · Pull Request #43183 · microsoft/TypeScript · GitHub
[go: up one dir, main page]
Skip to content

Improve narrowing of generic types in control flow analysis #43183

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

Merged
merged 15 commits into from
Mar 20, 2021
Merged

Improve narrowing of generic types in control flow analysis #43183

Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a3028a2
Narrow type variables with union constraints when merited by contextu…
ahejlsberg Mar 9, 2021
df7d7be
Narrow generics with union type constraints as indicated by contextua…
ahejlsberg Mar 10, 2021
13cf64e
Accept new baselines
ahejlsberg Mar 10, 2021
a0fbf5c
Add tests
ahejlsberg Mar 10, 2021
1a695e9
Fix circularity for JSX elements
ahejlsberg Mar 10, 2021
a05ff0b
Remove unnecessary isConstraintPosition information from flow cache key
ahejlsberg Mar 11, 2021
0e76720
Update comment
ahejlsberg Mar 11, 2021
1f720d3
Add additional tests
ahejlsberg Mar 11, 2021
a96cbab
Rename to getNarrowableTypeForReference, remove getConstraintForLocation
ahejlsberg Mar 13, 2021
d1c23ae
Add comment
ahejlsberg Mar 13, 2021
1648bcd
Fix removal of undefined in destructurings with initializers
ahejlsberg Mar 19, 2021
acdf986
Merge branch 'master' into fix13995
ahejlsberg Mar 19, 2021
4e0baf3
Use getContextFreeTypeOfExpression in discriminateContextualTypeByObj…
ahejlsberg Mar 19, 2021
4f3fd56
In obj[x], use constraint of obj's type only when x's type is non-gen…
ahejlsberg Mar 19, 2021
3c6616b
Add comment
ahejlsberg Mar 19, 2021
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
Prev Previous commit
Next Next commit
Narrow generics with union type constraints as indicated by contextua… 
…l type
  • Loading branch information
@ahejlsberg
ahejlsberg committed Mar 10, 2021
commit df7d7be2c6479154d3b9405be33930d7ac09263e
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 @@ -20532,7 +20532,7 @@ namespace ts {

function isStringLiteralTypeValueParsableAsType(s: StringLiteralType, target: Type): boolean {
if (target.flags & TypeFlags.Union) {
return !!forEachType(target, t => isStringLiteralTypeValueParsableAsType(s, t));
return someType(target, t => isStringLiteralTypeValueParsableAsType(s, t));
}
switch (target) {
case stringType: return true;
Expand Down Expand Up @@ -21975,6 +21975,10 @@ namespace ts {
return type.flags & TypeFlags.Union ? forEach((<UnionType>type).types, f) : f(type);
}

function someType(type: Type, f: (t: Type) => boolean): boolean {
return type.flags & TypeFlags.Union ? some((<UnionType>type).types, f) : f(type);
}

function everyType(type: Type, f: (t: Type) => boolean): boolean {
return type.flags & TypeFlags.Union ? every((<UnionType>type).types, f) : f(type);
}
Expand Down Expand Up @@ -23553,7 +23557,7 @@ namespace ts {
}

function typeHasNullableConstraint(type: Type) {
return type.flags & TypeFlags.InstantiableNonPrimitive && maybeTypeOfKind(getBaseConstraintOfType(type) || unknownType, TypeFlags.Nullable);
return !!(type.flags & TypeFlags.InstantiableNonPrimitive) && maybeTypeOfKind(getBaseConstraintOfType(type) || unknownType, TypeFlags.Nullable);
}

function getConstraintForLocation(type: Type, node: Node): Type;
Expand All @@ -23563,36 +23567,38 @@ namespace ts {
// and the type of the node includes type variables with constraints that are nullable, we fetch the
// apparent type of the node *before* performing control flow analysis such that narrowings apply to
// the constraint type.
if (type && isConstraintPosition(node) && forEachType(type, typeHasNullableConstraint)) {
if (type && isConstraintPosition(node) && someType(type, typeHasNullableConstraint)) {
return mapType(getWidenedType(type), getBaseConstraintOrType);
}
return type;
}

function containsTypeVariable(type: Type, typeVariable: TypeVariable): boolean {
return type === typeVariable ||
!!(type.flags & TypeFlags.UnionOrIntersection) && some((<UnionOrIntersectionType>type).types, t => containsTypeVariable(t, typeVariable));
function isTypeVariableWithUnionConstraint(type: Type) {
return !!(type.flags & TypeFlags.Instantiable && getBaseConstraintOrType(type).flags & TypeFlags.Union);
}

function containsTypeVariable(type: Type): boolean {
return !!(type.flags & TypeFlags.Instantiable || type.flags & TypeFlags.UnionOrIntersection && some((<UnionOrIntersectionType>type).types, containsTypeVariable));
}

function hasContextualTypeWithNoTypeVariables(node: Expression) {
Copy link
Member

Choose a reason for hiding this comment

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

Asking for the contextual type sometimes leads to circularities, and I think this function is called in a couple of places that wouldn't otherwise do that.

Uh...I'm not sure how to figure out if this is a problem, it's just worth thinking about.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, I had to exclude JSX elements for exactly that reason. I added a comment to that effect.

const contextualType = getContextualType(node);
return contextualType && !someType(contextualType, containsTypeVariable);
}
Copy link
Member

Choose a reason for hiding this comment

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

what's the difference between a location and a reference? If it's really getConstraintOld and getConstraintNew, maybe this should be combined into one function.

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, not much of a difference really. I've merged it all into one function and renamed it to getNarrowableTypeForReference. Also renamed some of the helpers to better reflect what they do.


function getConstraintForReference(type: Type, reference: Identifier | ElementAccessExpression | PropertyAccessExpression | QualifiedName, checkMode: CheckMode | undefined) {
// When the type of a reference is a type variable with a union type constraint, and when the reference
// is contextually typed by a type that doesn't contain that type variable, we substitute the union type
// constraint for the type variable to give control flow analysis an opportunity to narrow it further.
// (This transformation causes no type errors because a type variable with a union constraint is only
// assignable to itself and its constraint, and we already know the type variable doesn't occur in the
// contextual type.) For example, for a reference of a type parameter type 'T extends string | undefined'
// with a contextual type 'string', we substitute 'string | undefined' to give control flow analysis the
// opportunity to narrow to type 'string'.
if (type.flags & TypeFlags.TypeVariable && reference.kind !== SyntaxKind.QualifiedName && !(checkMode && checkMode & CheckMode.Inferential)) {
const constraint = getConstraintOfType(type);
if (constraint && constraint.flags & TypeFlags.Union) {
const contextualType = getContextualType(reference);
if (contextualType && !containsTypeVariable(contextualType, <TypeVariable>type)) {
return constraint;
}
}
}
return getConstraintForLocation(type, reference);
// When the type of a reference is or contains an instantiable type with a union type constraint, and
// when the reference is in a constraint position (where it is known we'll obtain the apparent type) or
// has a contextual type containing no instantiables (meaning constraints will determine assignability),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// has a contextual type containing no instantiables (meaning constraints will determine assignability),
// has a contextual type containing no top-level instantiables (meaning constraints will determine assignability),

or

Suggested change
// has a contextual type containing no instantiables (meaning constraints will determine assignability),
// has a contextual type containing no immediate instantiables (meaning constraints will determine assignability),

// we substitute constraints for all instantiables in the type of the reference to give control flow
// analysis an opportunity to narrow it further. For example, for a reference of a type parameter type
// 'T extends string | undefined' with a contextual type 'string', we substitute 'string | undefined'
// to give control flow analysis the opportunity to narrow to type 'string'.
const substituteConstraints = reference.kind !== SyntaxKind.QualifiedName &&
!(checkMode && checkMode & CheckMode.Inferential) &&
someType(type, isTypeVariableWithUnionConstraint) &&
(isConstraintPosition(reference) || hasContextualTypeWithNoTypeVariables(reference));
return substituteConstraints ? mapType(type, t => t.flags & TypeFlags.Instantiable ? getBaseConstraintOrType(t) : t) : type;
}

function isExportOrExportExpression(location: Node) {
Expand Down Expand Up @@ -25470,7 +25476,7 @@ namespace ts {
if (inDestructuringPattern) {
return createTupleType(elementTypes, elementFlags);
}
if (forceTuple || inConstContext || contextualType && forEachType(contextualType, isTupleLikeType)) {
if (forceTuple || inConstContext || contextualType && someType(contextualType, isTupleLikeType)) {
return createArrayLiteralType(createTupleType(elementTypes, elementFlags, /*readonly*/ inConstContext));
}
return createArrayLiteralType(createArrayType(elementTypes.length ?
Expand Down Expand Up @@ -25956,7 +25962,7 @@ namespace ts {
// If there are children in the body of JSX element, create dummy attribute "children" with the union of children types so that it will pass the attribute checking process
const childrenPropSymbol = createSymbol(SymbolFlags.Property, jsxChildrenPropertyName);
childrenPropSymbol.type = childrenTypes.length === 1 ? childrenTypes[0] :
childrenContextualType && forEachType(childrenContextualType, isTupleLikeType) ? createTupleType(childrenTypes) :
childrenContextualType && someType(childrenContextualType, isTupleLikeType) ? createTupleType(childrenTypes) :
createArrayType(getUnionType(childrenTypes));
// Fake up a property declaration for the children
childrenPropSymbol.valueDeclaration = factory.createPropertySignature(/*modifiers*/ undefined, unescapeLeadingUnderscores(jsxChildrenPropertyName), /*questionToken*/ undefined, /*type*/ undefined);
Expand Down Expand Up @@ -41383,7 +41389,7 @@ namespace ts {
}

function findBestTypeForObjectLiteral(source: Type, unionTarget: UnionOrIntersectionType) {
if (getObjectFlags(source) & ObjectFlags.ObjectLiteral && forEachType(unionTarget, isArrayLikeType)) {
if (getObjectFlags(source) & ObjectFlags.ObjectLiteral && someType(unionTarget, isArrayLikeType)) {
return find(unionTarget.types, t => !isArrayLikeType(t));
}
}
Expand Down
0