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.

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 instanceof narrowing #10194

Merged
merged 6 commits into from
Aug 8, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
54 changes: 44 additions & 10 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8084,6 +8084,25 @@ namespace ts {
return source.flags & TypeFlags.Union ? !forEach((<UnionType>source).types, t => !contains(types, t)) : contains(types, source);
}

function isTypeSubsetOf(source: Type, target: Type) {
return source === target || target.flags & TypeFlags.Union && isTypeSubsetOfUnion(source, <UnionType>target);
}

function isTypeSubsetOfUnion(source: Type, target: UnionType) {
if (source.flags & TypeFlags.Union) {
for (const t of (<UnionType>source).types) {
if (!containsType(target.types, t)) {
return false;
}
}
return true;
}
if (source.flags & TypeFlags.EnumLiteral && target.flags & TypeFlags.Enum && (<EnumLiteralType>source).baseType === target) {
return true;
}
return containsType(target.types, source);
}

function filterType(type: Type, f: (t: Type) => boolean): Type {
return type.flags & TypeFlags.Union ?
getUnionType(filter((<UnionType>type).types, f)) :
Expand Down Expand Up @@ -8230,6 +8249,7 @@ namespace ts {

function getTypeAtFlowBranchLabel(flow: FlowLabel): FlowType {
const antecedentTypes: Type[] = [];
let subtypeReduction = false;
let seenIncomplete = false;
for (const antecedent of flow.antecedents) {
const flowType = getTypeAtFlowNode(antecedent);
Expand All @@ -8244,11 +8264,17 @@ namespace ts {
if (!contains(antecedentTypes, type)) {
antecedentTypes.push(type);
}
// If an antecedent type is not a subset of the declared type, we need to perform
// subtype reduction. This happens when a "foreign" type is injected into the control
// flow using the instanceof operator or a user defined type predicate.
if (!isTypeSubsetOf(type, declaredType)) {
subtypeReduction = true;
}
if (isIncomplete(flowType)) {
seenIncomplete = true;
}
}
return createFlowType(getUnionType(antecedentTypes), seenIncomplete);
return createFlowType(getUnionType(antecedentTypes, subtypeReduction), seenIncomplete);
}

function getTypeAtFlowLoopLabel(flow: FlowLabel): FlowType {
Expand All @@ -8274,6 +8300,7 @@ namespace ts {
// Add the flow loop junction and reference to the in-process stack and analyze
// each antecedent code path.
const antecedentTypes: Type[] = [];
let subtypeReduction = false;
flowLoopNodes[flowLoopCount] = flow;
flowLoopKeys[flowLoopCount] = key;
flowLoopTypes[flowLoopCount] = antecedentTypes;
Expand All @@ -8290,14 +8317,20 @@ namespace ts {
if (!contains(antecedentTypes, type)) {
antecedentTypes.push(type);
}
// If an antecedent type is not a subset of the declared type, we need to perform
// subtype reduction. This happens when a "foreign" type is injected into the control
// flow using the instanceof operator or a user defined type predicate.
if (!isTypeSubsetOf(type, declaredType)) {
subtypeReduction = true;
}
// If the type at a particular antecedent path is the declared type there is no
// reason to process more antecedents since the only possible outcome is subtypes
// that will be removed in the final union type anyway.
if (type === declaredType) {
break;
}
}
return cache[key] = getUnionType(antecedentTypes);
return cache[key] = getUnionType(antecedentTypes, subtypeReduction);
}

function isMatchingPropertyAccess(expr: Expression) {
Expand Down Expand Up @@ -8494,9 +8527,7 @@ namespace ts {

function getNarrowedType(type: Type, candidate: Type, assumeTrue: boolean) {
if (!assumeTrue) {
return type.flags & TypeFlags.Union ?
getUnionType(filter((<UnionType>type).types, t => !isTypeSubtypeOf(t, candidate))) :
type;
return filterType(type, t => !isTypeSubtypeOf(t, candidate));
}
// If the current type is a union type, remove all constituents that aren't assignable to
// the candidate type. If one or more constituents remain, return a union of those.
Expand All @@ -8506,13 +8537,16 @@ namespace ts {
return getUnionType(assignableConstituents);
}
}
// If the candidate type is assignable to the target type, narrow to the candidate type.
// Otherwise, if the current type is assignable to the candidate, keep the current type.
// Otherwise, the types are completely unrelated, so narrow to the empty type.
// If the candidate type is a subtype of the target type, narrow to the candidate type.
// Otherwise, if the target type is assignable to the candidate type, keep the target type.
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 there's a little more precision than the wording lends to here. If the candidate isn't a subtype, but is assignable, we keep the original type in place. (e.g. with Array.isArray, you preserve the original type instead of giving back Array<any>).

// Otherwise, if the candidate type is assignable to the target type, narrow to the candidate
// type. Otherwise, the types are completely unrelated, so narrow to an intersection of the
// two types.
const targetType = type.flags & TypeFlags.TypeParameter ? getApparentType(type) : type;
return isTypeAssignableTo(candidate, targetType) ? candidate :
return isTypeSubtypeOf(candidate, targetType) ? candidate :
isTypeAssignableTo(type, candidate) ? type :
Copy link
Member

Choose a reason for hiding this comment

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

Is there a test where this condition being true is witnessed? You need something where

  • the candidate isn't a subtype of the original type
  • the original type can't be assigned to the candidate type
  • the candidate is assignable to the original type

Copy link
Member Author

Choose a reason for hiding this comment

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

Added an extra test to cover that case.

getIntersectionType([type, candidate]);
isTypeAssignableTo(candidate, targetType) ? candidate :
getIntersectionType([type, candidate]);
}

function narrowTypeByTypePredicate(type: Type, callExpression: CallExpression, assumeTrue: boolean): Type {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ if (isNodeList(sourceObj) || isHTMLCollection(sourceObj)) {
>sourceObj : Symbol(sourceObj, Decl(controlFlowBinaryOrExpression.ts, 23, 3))

sourceObj.length;
>sourceObj.length : Symbol(length, Decl(controlFlowBinaryOrExpression.ts, 10, 27), Decl(controlFlowBinaryOrExpression.ts, 14, 33))
>sourceObj.length : Symbol(NodeList.length, Decl(controlFlowBinaryOrExpression.ts, 10, 27))
>sourceObj : Symbol(sourceObj, Decl(controlFlowBinaryOrExpression.ts, 23, 3))
>length : Symbol(length, Decl(controlFlowBinaryOrExpression.ts, 10, 27), Decl(controlFlowBinaryOrExpression.ts, 14, 33))
>length : Symbol(NodeList.length, Decl(controlFlowBinaryOrExpression.ts, 10, 27))
}

Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ if (isNodeList(sourceObj) || isHTMLCollection(sourceObj)) {

sourceObj.length;
>sourceObj.length : number
>sourceObj : NodeList | HTMLCollection | ({ a: string; } & HTMLCollection)
>sourceObj : NodeList
>length : number
}

184 changes: 184 additions & 0 deletions tests/baselines/reference/controlFlowInstanceof.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,184 @@
//// [controlFlowInstanceof.ts]

// Repros from #10167

function f1(s: Set<string> | Set<number>) {
s = new Set<number>();
s; // Set<number>
if (s instanceof Set) {
s; // Set<number>
}
s; // Set<number>
s.add(42);
}

function f2(s: Set<string> | Set<number>) {
s = new Set<number>();
s; // Set<number>
if (s instanceof Promise) {
s; // Set<number> & Promise<any>
}
s; // Set<number>
s.add(42);
}

function f3(s: Set<string> | Set<number>) {
s; // Set<string> | Set<number>
if (s instanceof Set) {
s; // Set<string> | Set<number>
}
else {
s; // never
}
}

function f4(s: Set<string> | Set<number>) {
s = new Set<number>();
s; // Set<number>
if (s instanceof Set) {
s; // Set<number>
}
else {
s; // never
}
}

// More tests

class A { a: string }
class B extends A { b: string }
class C extends A { c: string }

function foo(x: A | undefined) {
x; // A | undefined
if (x instanceof B || x instanceof C) {
x; // B | C
}
x; // A | undefined
if (x instanceof B && x instanceof C) {
x; // B & C
}
x; // A | undefined
if (!x) {
return;
}
x; // A
if (x instanceof B) {
x; // B
if (x instanceof C) {
x; // B & C
}
else {
x; // B
}
x; // B
}
else {
x; // A
}
x; // A
}

// X is neither assignable to Y nor a subtype of Y
// Y is assignable to X, but not a subtype of X

interface X {
x?: string;
}

class Y {
y: string;
}

function goo(x: X) {
x;
if (x instanceof Y) {
x.y;
}
x;
}

//// [controlFlowInstanceof.js]
// Repros from #10167
function f1(s) {
s = new Set();
s; // Set<number>
if (s instanceof Set) {
s; // Set<number>
}
s; // Set<number>
s.add(42);
}
function f2(s) {
s = new Set();
s; // Set<number>
if (s instanceof Promise) {
s; // Set<number> & Promise<any>
}
s; // Set<number>
s.add(42);
}
function f3(s) {
s; // Set<string> | Set<number>
if (s instanceof Set) {
s; // Set<string> | Set<number>
}
else {
s; // never
}
}
function f4(s) {
s = new Set();
s; // Set<number>
if (s instanceof Set) {
s; // Set<number>
}
else {
s; // never
}
}
// More tests
class A {
}
class B extends A {
}
class C extends A {
}
function foo(x) {
x; // A | undefined
if (x instanceof B || x instanceof C) {
x; // B | C
}
x; // A | undefined
if (x instanceof B && x instanceof C) {
x; // B & C
}
x; // A | undefined
if (!x) {
return;
}
x; // A
if (x instanceof B) {
x; // B
if (x instanceof C) {
x; // B & C
}
else {
x; // B
}
x; // B
}
else {
x; // A
}
x; // A
}
class Y {
}
function goo(x) {
x;
if (x instanceof Y) {
x.y;
}
x;
}
Loading