-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Error when LHS of instanceof is Union of Primitives #18519 #19063
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
Error when LHS of instanceof is Union of Primitives #18519 #19063
Conversation
@DanielRosenwasser can you review this change. |
@DanielRosenwasser @mhegazy Just wanted to check if there were any issues with this PR, so I can make any needed changes before it goes stale. |
src/compiler/checker.ts
Outdated
@@ -17604,7 +17604,9 @@ namespace ts { | |||
// and the right operand to be of type Any, a subtype of the 'Function' interface type, or have a call or construct signature. | |||
// 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 (!isTypeAny(leftType) && isTypeAssignableToKind(leftType, TypeFlags.Primitive)) { | |||
if (!isTypeAny(leftType) && | |||
(isTypeAssignableToKind(leftType, TypeFlags.Primitive) || |
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 wonder if it makes sense to have isTypeAssignableToKind
just work for all constituents if given a union type. Perhaps make an allTypesAssignableToKind
helper below it to keep the change scoped.
Otherwise, this seems fine.
@DanielRosenwasser I like keeping the change scoped, since Also updated the test to include the case |
src/compiler/checker.ts
Outdated
@@ -17587,6 +17587,16 @@ namespace ts { | |||
(kind & TypeFlags.NonPrimitive && isTypeAssignableTo(source, nonPrimitiveType)); | |||
} | |||
|
|||
function allTypesAssignableToKind(source: Type, kind: TypeFlags, strict?: boolean): boolean { | |||
if (source.flags & TypeFlags.Union) { |
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.
return source.flags & TypeFlags.Union ?
every((source as UnionType).types, subType => allTypesAssignableToKind(subType, kind, strict)) :
isTypeAssignableToKind(source, kind, strict);
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.
Updated
@mhegazy @DanielRosenwasser Any additional concerns? |
Sorry for the delay. |
Fixes #18519
Added additional check for if the left-hand operand of an
instanceof
is a union of primitives and raise an error in that case as well.