-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 destructuring and narrowing interaction #47337
Merged
Merged
Changes from 13 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
e9b9f2d
WIP: pass in checkmode to getNarrowableTypeForReference
gabritto a89fa72
add tests
gabritto b4eab71
Merge branch 'main' into gabritto/issue45762
gabritto 789c249
another pass through new check mode argument
gabritto 1675422
rename new check mode
gabritto e385038
only use rest flag for rest elements in objects
gabritto a831ccd
add and update tests
gabritto 16c8a2d
Merge branch 'main' into gabritto/issue45762
gabritto 68276e4
change check mode flag name
gabritto 5383454
restore package-lock.json
gabritto 30b7589
fix comments
gabritto 28f667d
get rid of fourslash tests
gabritto 23c38be
fix caching in checkExpressionCached when checkMode is not normal
gabritto aad8852
Don't distinguish between object and array rest elements
gabritto e8ba399
Merge branch 'main' into gabritto/issue45762
gabritto 20bd98c
get rid of undefined check mode
gabritto 6ba2667
don't make includeOptionality into checkmode flag
gabritto File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -172,12 +172,16 @@ namespace ts { | |
} | ||
|
||
const enum CheckMode { | ||
Normal = 0, // Normal type checking | ||
Contextual = 1 << 0, // Explicitly assigned contextual type, therefore not cacheable | ||
Inferential = 1 << 1, // Inferential typing | ||
SkipContextSensitive = 1 << 2, // Skip context sensitive function expressions | ||
SkipGenericFunctions = 1 << 3, // Skip single signature generic functions | ||
IsForSignatureHelp = 1 << 4, // Call resolution for purposes of signature help | ||
Normal = 0, // Normal type checking | ||
Contextual = 1 << 0, // Explicitly assigned contextual type, therefore not cacheable | ||
Inferential = 1 << 1, // Inferential typing | ||
SkipContextSensitive = 1 << 2, // Skip context sensitive function expressions | ||
SkipGenericFunctions = 1 << 3, // Skip single signature generic functions | ||
IsForSignatureHelp = 1 << 4, // Call resolution for purposes of signature help | ||
ObjectRestBindingElement = 1 << 5, // Checking a type that is going to be used to determine the type of a rest binding element | ||
// e.g. in `const { a, ...rest } = foo`, when checking the type of `foo` to determine the type of `rest`, | ||
// we need to preserve generic types instead of substituting them for constraints | ||
IncludeOptionality = 1 << 6, // >> TODO: description, replace bool param with flag | ||
} | ||
|
||
const enum SignatureCheckMode { | ||
|
@@ -8446,9 +8450,12 @@ namespace ts { | |
|
||
// Return the type of a binding element parent. We check SymbolLinks first to see if a type has been | ||
// assigned by contextual typing. | ||
function getTypeForBindingElementParent(node: BindingElementGrandparent) { | ||
function getTypeForBindingElementParent(node: BindingElementGrandparent, checkMode: CheckMode | undefined) { | ||
if (checkMode && checkMode !== CheckMode.Normal) { | ||
return getTypeForVariableLikeDeclaration(node, /*includeOptionality*/ false, checkMode); | ||
} | ||
const symbol = getSymbolOfNode(node); | ||
return symbol && getSymbolLinks(symbol).type || getTypeForVariableLikeDeclaration(node, /*includeOptionality*/ false); | ||
return symbol && getSymbolLinks(symbol).type || getTypeForVariableLikeDeclaration(node, /*includeOptionality*/ false, checkMode); | ||
} | ||
|
||
function getRestType(source: Type, properties: PropertyName[], symbol: Symbol | undefined): Type { | ||
|
@@ -8579,7 +8586,8 @@ namespace ts { | |
|
||
/** Return the inferred type for a binding element */ | ||
function getTypeForBindingElement(declaration: BindingElement): Type | undefined { | ||
const parentType = getTypeForBindingElementParent(declaration.parent.parent); | ||
const checkMode = declaration.dotDotDotToken && declaration.parent.kind !== SyntaxKind.ArrayBindingPattern ? CheckMode.ObjectRestBindingElement : CheckMode.Normal; | ||
const parentType = getTypeForBindingElementParent(declaration.parent.parent, checkMode); | ||
return parentType && getBindingElementTypeFromParentType(declaration, parentType); | ||
} | ||
|
||
|
@@ -8652,9 +8660,9 @@ namespace ts { | |
if (getEffectiveTypeAnnotationNode(walkUpBindingElementsAndPatterns(declaration))) { | ||
// In strict null checking mode, if a default value of a non-undefined type is specified, remove | ||
// undefined from the final type. | ||
return strictNullChecks && !(getFalsyFlags(checkDeclarationInitializer(declaration)) & TypeFlags.Undefined) ? getNonUndefinedType(type) : type; | ||
return strictNullChecks && !(getFalsyFlags(checkDeclarationInitializer(declaration, /* checkMode */ undefined)) & TypeFlags.Undefined) ? getNonUndefinedType(type) : type; | ||
} | ||
return widenTypeInferredFromInitializer(declaration, getUnionType([getNonUndefinedType(type), checkDeclarationInitializer(declaration)], UnionReduction.Subtype)); | ||
return widenTypeInferredFromInitializer(declaration, getUnionType([getNonUndefinedType(type), checkDeclarationInitializer(declaration, /* checkMode */ undefined)], UnionReduction.Subtype)); | ||
} | ||
|
||
function getTypeForDeclarationFromJSDocComment(declaration: Node) { | ||
|
@@ -8680,11 +8688,15 @@ namespace ts { | |
} | ||
|
||
// Return the inferred type for a variable, parameter, or property declaration | ||
function getTypeForVariableLikeDeclaration(declaration: ParameterDeclaration | PropertyDeclaration | PropertySignature | VariableDeclaration | BindingElement | JSDocPropertyLikeTag, includeOptionality: boolean): Type | undefined { | ||
function getTypeForVariableLikeDeclaration( | ||
declaration: ParameterDeclaration | PropertyDeclaration | PropertySignature | VariableDeclaration | BindingElement | JSDocPropertyLikeTag, | ||
includeOptionality: boolean, // >> TODO: change this to use a flag in check mode; figure out how this interacts with caching | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we decide to move forward with this PR, I'm going to replace the use of |
||
checkMode: CheckMode | undefined, | ||
): Type | undefined { | ||
// A variable declared in a for..in statement is of type string, or of type keyof T when the | ||
// right hand expression is of a type parameter type. | ||
if (isVariableDeclaration(declaration) && declaration.parent.parent.kind === SyntaxKind.ForInStatement) { | ||
const indexType = getIndexType(getNonNullableTypeIfNeeded(checkExpression(declaration.parent.parent.expression))); | ||
const indexType = getIndexType(getNonNullableTypeIfNeeded(checkExpression(declaration.parent.parent.expression, /*checkMode*/ checkMode))); | ||
return indexType.flags & (TypeFlags.TypeParameter | TypeFlags.Index) ? getExtractStringType(indexType) : stringType; | ||
} | ||
|
||
|
@@ -8769,7 +8781,7 @@ namespace ts { | |
return containerObjectType; | ||
} | ||
} | ||
const type = widenTypeInferredFromInitializer(declaration, checkDeclarationInitializer(declaration)); | ||
const type = widenTypeInferredFromInitializer(declaration, checkDeclarationInitializer(declaration, checkMode)); | ||
return addOptionality(type, isProperty, isOptional); | ||
} | ||
|
||
|
@@ -9162,7 +9174,7 @@ namespace ts { | |
// contextual type or, if the element itself is a binding pattern, with the type implied by that binding | ||
// pattern. | ||
const contextualType = isBindingPattern(element.name) ? getTypeFromBindingPattern(element.name, /*includePatternInType*/ true, /*reportErrors*/ false) : unknownType; | ||
return addOptionality(widenTypeInferredFromInitializer(element, checkDeclarationInitializer(element, contextualType))); | ||
return addOptionality(widenTypeInferredFromInitializer(element, checkDeclarationInitializer(element, /*checkMode*/ undefined, contextualType))); | ||
} | ||
if (isBindingPattern(element.name)) { | ||
return getTypeFromBindingPattern(element.name, includePatternInType, reportErrors); | ||
|
@@ -9254,7 +9266,7 @@ namespace ts { | |
// binding pattern [x, s = ""]. Because the contextual type is a tuple type, the resulting type of [1, "one"] is the | ||
// tuple type [number, string]. Thus, the type inferred for 'x' is number and the type inferred for 's' is string. | ||
function getWidenedTypeForVariableLikeDeclaration(declaration: ParameterDeclaration | PropertyDeclaration | PropertySignature | VariableDeclaration | BindingElement | JSDocPropertyLikeTag, reportErrors?: boolean): Type { | ||
return widenTypeForVariableLikeDeclaration(getTypeForVariableLikeDeclaration(declaration, /*includeOptionality*/ true), declaration, reportErrors); | ||
return widenTypeForVariableLikeDeclaration(getTypeForVariableLikeDeclaration(declaration, /*includeOptionality*/ true, /*checkMode*/ undefined), declaration, reportErrors); | ||
} | ||
|
||
function isGlobalSymbolConstructor(node: Node) { | ||
|
@@ -24927,12 +24939,16 @@ namespace ts { | |
return !!(type.flags & TypeFlags.Instantiable && !maybeTypeOfKind(getBaseConstraintOrType(type), TypeFlags.Nullable)); | ||
} | ||
|
||
function hasNonBindingPatternContextualTypeWithNoGenericTypes(node: Node) { | ||
function hasContextualTypeWithNoGenericTypes(node: Node, checkMode: CheckMode | undefined) { | ||
// Computing the contextual type for a child of a JSX element involves resolving the type of the | ||
// element's tag name, so we exclude that here to avoid circularities. | ||
// If check mode has `CheckMode.ObjectRestBindingElement`, we skip binding pattern contextual types, | ||
// as we want the type of a rest element to be generic when possible. | ||
const contextualType = (isIdentifier(node) || isPropertyAccessExpression(node) || isElementAccessExpression(node)) && | ||
!((isJsxOpeningElement(node.parent) || isJsxSelfClosingElement(node.parent)) && node.parent.tagName === node) && | ||
getContextualType(node, ContextFlags.SkipBindingPatterns); | ||
(checkMode && checkMode & CheckMode.ObjectRestBindingElement ? | ||
getContextualType(node, ContextFlags.SkipBindingPatterns) | ||
: getContextualType(node)); | ||
return contextualType && !isGenericType(contextualType); | ||
} | ||
|
||
|
@@ -24946,7 +24962,7 @@ namespace ts { | |
// 'string | undefined' to give control flow analysis the opportunity to narrow to type 'string'. | ||
const substituteConstraints = !(checkMode && checkMode & CheckMode.Inferential) && | ||
someType(type, isGenericTypeWithUnionConstraint) && | ||
(isConstraintPosition(type, reference) || hasNonBindingPatternContextualTypeWithNoGenericTypes(reference)); | ||
(isConstraintPosition(type, reference) || hasContextualTypeWithNoGenericTypes(reference, checkMode)); | ||
return substituteConstraints ? mapType(type, t => t.flags & TypeFlags.Instantiable && !isMappedTypeGenericIndexedAccess(t) ? getBaseConstraintOrType(t) : t) : type; | ||
} | ||
|
||
|
@@ -25018,7 +25034,7 @@ namespace ts { | |
const links = getNodeLinks(location); | ||
if (!(links.flags & NodeCheckFlags.InCheckIdentifier)) { | ||
links.flags |= NodeCheckFlags.InCheckIdentifier; | ||
const parentType = getTypeForBindingElementParent(parent); | ||
const parentType = getTypeForBindingElementParent(parent, /*checkMode*/ undefined); | ||
links.flags &= ~NodeCheckFlags.InCheckIdentifier; | ||
if (parentType && parentType.flags & TypeFlags.Union && !(parent.kind === SyntaxKind.Parameter && isSymbolAssigned(symbol))) { | ||
const pattern = declaration.parent; | ||
|
@@ -25951,7 +25967,7 @@ namespace ts { | |
const parent = declaration.parent.parent; | ||
const name = declaration.propertyName || declaration.name; | ||
const parentType = getContextualTypeForVariableLikeDeclaration(parent) || | ||
parent.kind !== SyntaxKind.BindingElement && parent.initializer && checkDeclarationInitializer(parent); | ||
parent.kind !== SyntaxKind.BindingElement && parent.initializer && checkDeclarationInitializer(parent, declaration.dotDotDotToken && declaration.parent.kind !== SyntaxKind.ArrayBindingPattern ? CheckMode.ObjectRestBindingElement : CheckMode.Normal); | ||
if (!parentType || isBindingPattern(name) || isComputedNonLiteralName(name)) return undefined; | ||
if (parent.name.kind === SyntaxKind.ArrayBindingPattern) { | ||
const index = indexOfNode(declaration.parent.elements, declaration); | ||
|
@@ -31730,7 +31746,7 @@ namespace ts { | |
const links = getSymbolLinks(parameter); | ||
if (!links.type) { | ||
const declaration = parameter.valueDeclaration as ParameterDeclaration; | ||
links.type = type || getWidenedTypeForVariableLikeDeclaration(declaration, /*includeOptionality*/ true); | ||
links.type = type || getWidenedTypeForVariableLikeDeclaration(declaration, /*reportErrors*/ true); | ||
if (declaration.name.kind !== SyntaxKind.Identifier) { | ||
// if inference didn't come up with anything but unknown, fall back to the binding pattern if present. | ||
if (links.type === unknownType) { | ||
|
@@ -33647,11 +33663,11 @@ namespace ts { | |
} | ||
|
||
function checkExpressionCached(node: Expression | QualifiedName, checkMode?: CheckMode): Type { | ||
if (checkMode && checkMode !== CheckMode.Normal) { | ||
return checkExpression(node, checkMode); | ||
} | ||
const links = getNodeLinks(node); | ||
if (!links.resolvedType) { | ||
if (checkMode && checkMode !== CheckMode.Normal) { | ||
return checkExpression(node, checkMode); | ||
} | ||
// When computing a type that we're going to cache, we need to ignore any ongoing control flow | ||
// analysis because variables may have transient types in indeterminable states. Moving flowLoopStart | ||
// to the top of the stack ensures all transient types are computed from a known point. | ||
|
@@ -33673,10 +33689,16 @@ namespace ts { | |
isJSDocTypeAssertion(node); | ||
} | ||
|
||
function checkDeclarationInitializer(declaration: HasExpressionInitializer, contextualType?: Type | undefined) { | ||
function checkDeclarationInitializer( | ||
declaration: HasExpressionInitializer, | ||
checkMode: CheckMode | undefined, | ||
contextualType?: Type | undefined | ||
) { | ||
const initializer = getEffectiveInitializer(declaration)!; | ||
const type = getQuickTypeOfExpression(initializer) || | ||
(contextualType ? checkExpressionWithContextualType(initializer, contextualType, /*inferenceContext*/ undefined, CheckMode.Normal) : checkExpressionCached(initializer)); | ||
(contextualType ? | ||
checkExpressionWithContextualType(initializer, contextualType, /*inferenceContext*/ undefined, checkMode || CheckMode.Normal) | ||
: checkExpressionCached(initializer, checkMode)); | ||
return isParameter(declaration) && declaration.name.kind === SyntaxKind.ArrayBindingPattern && | ||
isTupleType(type) && !type.target.hasRestElement && getTypeReferenceArity(type) < declaration.name.elements.length ? | ||
padTupleType(type, declaration.name) : type; | ||
|
@@ -36911,7 +36933,8 @@ namespace ts { | |
|
||
// check private/protected variable access | ||
const parent = node.parent.parent; | ||
const parentType = getTypeForBindingElementParent(parent); | ||
const parentCheckMode = node.dotDotDotToken && node.parent.kind !== SyntaxKind.ArrayBindingPattern ? CheckMode.ObjectRestBindingElement : CheckMode.Normal; | ||
const parentType = getTypeForBindingElementParent(parent, parentCheckMode); | ||
const name = node.propertyName || node.name; | ||
if (parentType && !isBindingPattern(name)) { | ||
const exprType = getLiteralTypeFromPropertyName(name); | ||
|
@@ -38309,7 +38332,7 @@ namespace ts { | |
const declaration = catchClause.variableDeclaration; | ||
const typeNode = getEffectiveTypeAnnotationNode(getRootDeclaration(declaration)); | ||
if (typeNode) { | ||
const type = getTypeForVariableLikeDeclaration(declaration, /*includeOptionality*/ false); | ||
const type = getTypeForVariableLikeDeclaration(declaration, /*includeOptionality*/ false, /*checkMode*/ undefined); | ||
if (type && !(type.flags & TypeFlags.AnyOrUnknown)) { | ||
grammarErrorOnFirstToken(typeNode, Diagnostics.Catch_clause_variable_type_annotation_must_be_any_or_unknown_if_specified); | ||
} | ||
|
@@ -41288,7 +41311,7 @@ namespace ts { | |
} | ||
|
||
if (isBindingPattern(node)) { | ||
return getTypeForVariableLikeDeclaration(node.parent, /*includeOptionality*/ true) || errorType; | ||
return getTypeForVariableLikeDeclaration(node.parent, /*includeOptionality*/ true, /*checkMode*/ undefined) || errorType; | ||
} | ||
|
||
if (isInRightSideOfImportOrExportAssignment(node as Identifier)) { | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
//// [narrowingDestructuring.ts] | ||
type X = { kind: "a", a: string } | { kind: "b", b: string } | ||
|
||
function func<T extends X>(value: T) { | ||
if (value.kind === "a") { | ||
value.a; | ||
const { a } = value; | ||
} else { | ||
value.b; | ||
const { b } = value; | ||
} | ||
} | ||
|
||
type Z = { kind: "f", f: { a: number, b: string, c: number } } | ||
| { kind: "g", g: { a: string, b: number, c: string }}; | ||
|
||
function func2<T extends Z>(value: T) { | ||
if (value.kind === "f") { | ||
const { f: f1 } = value; | ||
const { f: { a, ...spread } } = value; | ||
value.f; | ||
} else { | ||
const { g: { c, ...spread } } = value; | ||
value.g; | ||
} | ||
} | ||
|
||
function func3<T extends { kind: "a", a: string } | { kind: "b", b: number }>(t: T) { | ||
if (t.kind === "a") { | ||
const { kind, ...r1 } = t; | ||
const r2 = (({ kind, ...rest }) => rest)(t); | ||
} | ||
} | ||
|
||
function farr<T extends [number, string, string] | [string, number, number]>(x: T) { | ||
const [head, ...tail] = x; | ||
if (x[0] === 'number') { | ||
const [head, ...tail] = x; | ||
} | ||
} | ||
|
||
//// [narrowingDestructuring.js] | ||
var __rest = (this && this.__rest) || function (s, e) { | ||
var t = {}; | ||
for (var p in s) if (Object.prototype.hasOwnProperty.call(s, p) && e.indexOf(p) < 0) | ||
t[p] = s[p]; | ||
if (s != null && typeof Object.getOwnPropertySymbols === "function") | ||
for (var i = 0, p = Object.getOwnPropertySymbols(s); i < p.length; i++) { | ||
if (e.indexOf(p[i]) < 0 && Object.prototype.propertyIsEnumerable.call(s, p[i])) | ||
t[p[i]] = s[p[i]]; | ||
} | ||
return t; | ||
}; | ||
function func(value) { | ||
if (value.kind === "a") { | ||
value.a; | ||
var a = value.a; | ||
} | ||
else { | ||
value.b; | ||
var b = value.b; | ||
} | ||
} | ||
function func2(value) { | ||
if (value.kind === "f") { | ||
var f1 = value.f; | ||
var _a = value.f, a = _a.a, spread = __rest(_a, ["a"]); | ||
value.f; | ||
} | ||
else { | ||
var _b = value.g, c = _b.c, spread = __rest(_b, ["c"]); | ||
value.g; | ||
} | ||
} | ||
function func3(t) { | ||
if (t.kind === "a") { | ||
var kind = t.kind, r1 = __rest(t, ["kind"]); | ||
var r2 = (function (_a) { | ||
var kind = _a.kind, rest = __rest(_a, ["kind"]); | ||
return rest; | ||
})(t); | ||
} | ||
} | ||
function farr(x) { | ||
var head = x[0], tail = x.slice(1); | ||
if (x[0] === 'number') { | ||
var head_1 = x[0], tail_1 = x.slice(1); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add this flag instead of the
includeOptionality
parameter in another commit, once we settle on the other changes of this PRThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should definitely get this change in the PR as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since replacing the
includeOptionality
by aCheckMode
flag affects our caching behavior incheckExpressionCached
(or we'll have to add an exception for that flag only), I'm gonna merge this without that part for the 4.6 RC, and I can add this change in another PR if we really want this.