-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Type guards should not affect values of type any #1433
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4622,8 +4622,8 @@ module ts { | |
// Get the narrowed type of a given symbol at a given location | ||
function getNarrowedTypeOfSymbol(symbol: Symbol, node: Node) { | ||
var type = getTypeOfSymbol(symbol); | ||
// Only narrow when symbol is variable of a structured type | ||
if (node && (symbol.flags & SymbolFlags.Variable && type.flags & TypeFlags.Structured)) { | ||
// Only narrow when symbol is variable of an object, union, or type parameter type | ||
if (node && symbol.flags & SymbolFlags.Variable && type.flags & (TypeFlags.ObjectType | TypeFlags.Union | TypeFlags.TypeParameter)) { | ||
loop: while (node.parent) { | ||
var child = node; | ||
node = node.parent; | ||
|
@@ -6587,12 +6587,12 @@ module ts { | |
return numberType; | ||
} | ||
|
||
// Return true if type is any, an object type, a type parameter, or a union type composed of only those kinds of types | ||
// Return true if type an object type, a type parameter, or a union type composed of only those kinds of types | ||
function isStructuredType(type: Type): boolean { | ||
if (type.flags & TypeFlags.Union) { | ||
return !forEach((<UnionType>type).types, t => !isStructuredType(t)); | ||
} | ||
return (type.flags & TypeFlags.Structured) !== 0; | ||
return (type.flags & (TypeFlags.ObjectType | TypeFlags.TypeParameter)) !== 0; | ||
} | ||
|
||
function isConstEnumObjectType(type: Type): boolean { | ||
|
@@ -6609,11 +6609,11 @@ module ts { | |
// and the right operand to be of type Any or a subtype of the 'Function' interface type. | ||
// The result is always of the Boolean primitive type. | ||
// NOTE: do not raise error if leftType is unknown as related error was already reported | ||
if (leftType !== unknownType && !isStructuredType(leftType)) { | ||
if (!(leftType.flags & TypeFlags.Any || isStructuredType(leftType))) { | ||
error(node.left, Diagnostics.The_left_hand_side_of_an_instanceof_expression_must_be_of_type_any_an_object_type_or_a_type_parameter); | ||
} | ||
// NOTE: do not raise error if right is unknown as related error was already reported | ||
if (rightType !== unknownType && rightType !== anyType && !isTypeSubtypeOf(rightType, globalFunctionType)) { | ||
if (!(rightType.flags & TypeFlags.Any || isTypeSubtypeOf(rightType, globalFunctionType))) { | ||
error(node.right, Diagnostics.The_right_hand_side_of_an_instanceof_expression_must_be_of_type_any_or_of_a_type_assignable_to_the_Function_interface_type); | ||
} | ||
return booleanType; | ||
|
@@ -6627,7 +6627,7 @@ module ts { | |
if (leftType !== anyType && leftType !== stringType && leftType !== numberType) { | ||
error(node.left, Diagnostics.The_left_hand_side_of_an_in_expression_must_be_of_types_any_string_or_number); | ||
} | ||
if (!isStructuredType(rightType)) { | ||
if (!(rightType.flags & TypeFlags.Any || isStructuredType(rightType))) { | ||
error(node.right, Diagnostics.The_right_hand_side_of_an_in_expression_must_be_of_type_any_an_object_type_or_a_type_parameter); | ||
} | ||
return booleanType; | ||
|
@@ -7975,7 +7975,7 @@ module ts { | |
var exprType = checkExpression(node.expression); | ||
// unknownType is returned i.e. if node.expression is identifier whose name cannot be resolved | ||
// in this case error about missing name is already reported - do not report extra one | ||
if (!isStructuredType(exprType) && exprType !== unknownType) { | ||
if (!(exprType.flags & TypeFlags.Any || isStructuredType(exprType))) { | ||
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. Again, what about unknown? |
||
error(node.expression, Diagnostics.The_right_hand_side_of_a_for_in_statement_must_be_of_type_any_an_object_type_or_a_type_parameter); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1240,7 +1240,6 @@ module ts { | |
StringLike = String | StringLiteral, | ||
NumberLike = Number | Enum, | ||
ObjectType = Class | Interface | Reference | Tuple | Anonymous, | ||
Structured = Any | ObjectType | Union | TypeParameter | ||
} | ||
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. Add comment why 'any' is considered to be a 'structured' type. (for that matter, comment what we mean by a 'structured type' and thus why a TypeParameter is also considered one). 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. This change removes Any from StructuredType, leaving just ObjectType, Union, and TypeParameter. 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. Why doesn't getNarrowedTypeOfSymbol call isStructuredType? Instead it checks the flag directly 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. The TypeFlags.Structured flag is set for a union type regardless of the state of the flag for each of the constituent types. The isStructuredType performs the deep check. In the case of narrowing we only care about the shallow state (e.g. you can narrow a union of primitive types). 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. This is somethign that would compltely break my expectations from reading the code. can you rename the isStructuredType flag to 'isStructuredTypeAtAnyDepth()' or something that can at least indicate that it's doing something extra. 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. I've gotten rid of the confusing TypeFlags.Structured flag and just inlined the actual flags in the few cases it was used. I think isStructuredType is a fine name for what it does and the comment on the function explains it further. 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. Thanks, that is helpful |
||
|
||
// Properties common to all types | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
//// [typeGuardsWithAny.ts] | ||
var x: any = { p: 0 }; | ||
if (x instanceof Object) { | ||
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. Add an else block as well 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. Done. |
||
x.p; // No error, type any unaffected by type guard | ||
} | ||
else { | ||
x.p; // No error, type any unaffected by type guard | ||
} | ||
|
||
|
||
//// [typeGuardsWithAny.js] | ||
var x = { p: 0 }; | ||
if (x instanceof Object) { | ||
x.p; // No error, type any unaffected by type guard | ||
} | ||
else { | ||
x.p; // No error, type any unaffected by type guard | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
=== tests/cases/conformance/expressions/typeGuards/typeGuardsWithAny.ts === | ||
var x: any = { p: 0 }; | ||
>x : any | ||
>{ p: 0 } : { p: number; } | ||
>p : number | ||
|
||
if (x instanceof Object) { | ||
>x instanceof Object : boolean | ||
>x : any | ||
>Object : ObjectConstructor | ||
|
||
x.p; // No error, type any unaffected by type guard | ||
>x.p : any | ||
>x : any | ||
>p : any | ||
} | ||
else { | ||
x.p; // No error, type any unaffected by type guard | ||
>x.p : any | ||
>x : any | ||
>p : any | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
var x: any = { p: 0 }; | ||
if (x instanceof Object) { | ||
x.p; // No error, type any unaffected by type guard | ||
} | ||
else { | ||
x.p; // No error, type any unaffected by type guard | ||
} |
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.
What about unknownType?
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.
TypeFlags.Any includes the unknown type.
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.
Can we rename it to .AnyOrUnknown then?
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'm going to leave it unchanged for now.