Skip to content

Add type guard for in keyword #15256

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 4 commits into from
Dec 6, 2017
Merged

Add type guard for in keyword #15256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9c3c2ad
Add initial support for 'in' typeguarding
IdeaHunter Apr 18, 2017
a5d6be1
Remove whitespace
IdeaHunter Apr 18, 2017
069f73d
Change type narrowing for optional properties
IdeaHunter Apr 20, 2017
04b9b30
Change isTypePresencePossible, accept baseline
IdeaHunter Dec 5, 2017
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
6 changes: 6 additions & 0 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,10 @@ namespace ts {
return expr1.kind === SyntaxKind.TypeOfExpression && isNarrowableOperand((<TypeOfExpression>expr1).expression) && expr2.kind === SyntaxKind.StringLiteral;
}

function isNarrowableInOperands(left: Expression, right: Expression) {
return left.kind === SyntaxKind.StringLiteral && isNarrowingExpression(right);
}

function isNarrowingBinaryExpression(expr: BinaryExpression) {
switch (expr.operatorToken.kind) {
case SyntaxKind.EqualsToken:
Expand All @@ -761,6 +765,8 @@ namespace ts {
isNarrowingTypeofOperands(expr.right, expr.left) || isNarrowingTypeofOperands(expr.left, expr.right);
case SyntaxKind.InstanceOfKeyword:
return isNarrowableOperand(expr.left);
case SyntaxKind.InKeyword:
return isNarrowableInOperands(expr.left, expr.right);
case SyntaxKind.CommaToken:
return isNarrowingExpression(expr.right);
}
Expand Down
22 changes: 22 additions & 0 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12722,6 +12722,22 @@ namespace ts {
return type;
}

function isTypePresencePossible(type: Type, propName: __String, assumeTrue: boolean) {
const prop = getPropertyOfType(type, propName);
if (prop) {
return (prop.flags & SymbolFlags.Optional) ? true : assumeTrue;
}
return !assumeTrue;
}

function narrowByInKeyword(type: Type, literal: LiteralExpression, assumeTrue: boolean) {
if ((type.flags & (TypeFlags.Union | TypeFlags.Object)) || (type.flags & TypeFlags.TypeParameter && (type as TypeParameter).isThisType)) {
const propName = escapeLeadingUnderscores(literal.text);
return filterType(type, t => isTypePresencePossible(t, propName, /* assumeTrue */ assumeTrue));
}
return type;
}

function narrowTypeByBinaryExpression(type: Type, expr: BinaryExpression, assumeTrue: boolean): Type {
switch (expr.operatorToken.kind) {
case SyntaxKind.EqualsToken:
Expand Down Expand Up @@ -12757,6 +12773,12 @@ namespace ts {
break;
case SyntaxKind.InstanceOfKeyword:
return narrowTypeByInstanceof(type, expr, assumeTrue);
case SyntaxKind.InKeyword:
const target = getReferenceCandidate(expr.right);
if (expr.left.kind === SyntaxKind.StringLiteral && isMatchingReference(reference, target)) {
return narrowByInKeyword(type, <LiteralExpression>expr.left, assumeTrue);
}
break;
case SyntaxKind.CommaToken:
return narrowType(type, expr.right, assumeTrue);
}
Expand Down
5 changes: 4 additions & 1 deletion tests/baselines/reference/fixSignatureCaching.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ tests/cases/conformance/fixSignatureCaching.ts(284,10): error TS2339: Property '
tests/cases/conformance/fixSignatureCaching.ts(293,10): error TS2339: Property 'FALLBACK_PHONE' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(294,10): error TS2339: Property 'FALLBACK_TABLET' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(295,10): error TS2339: Property 'FALLBACK_MOBILE' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(301,17): error TS2339: Property 'isArray' does not exist on type 'never'.
tests/cases/conformance/fixSignatureCaching.ts(330,74): error TS2339: Property 'mobileDetectRules' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(369,10): error TS2339: Property 'findMatch' does not exist on type '{}'.
tests/cases/conformance/fixSignatureCaching.ts(387,10): error TS2339: Property 'findMatches' does not exist on type '{}'.
Expand Down Expand Up @@ -71,7 +72,7 @@ tests/cases/conformance/fixSignatureCaching.ts(982,23): error TS2304: Cannot fin
tests/cases/conformance/fixSignatureCaching.ts(983,37): error TS2304: Cannot find name 'window'.


==== tests/cases/conformance/fixSignatureCaching.ts (71 errors) ====
==== tests/cases/conformance/fixSignatureCaching.ts (72 errors) ====
// Repro from #10697

(function (define, undefined) {
Expand Down Expand Up @@ -383,6 +384,8 @@ tests/cases/conformance/fixSignatureCaching.ts(983,37): error TS2304: Cannot fin
isArray = 'isArray' in Array
? function (value) { return Object.prototype.toString.call(value) === '[object Array]'; }
: Array.isArray;
~~~~~~~
!!! error TS2339: Property 'isArray' does not exist on type 'never'.

function equalIC(a, b) {
return a != null && b != null && a.toLowerCase() === b.toLowerCase();
Expand Down
2 changes: 0 additions & 2 deletions tests/baselines/reference/fixSignatureCaching.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,9 +358,7 @@ define(function () {
>value : Symbol(value, Decl(fixSignatureCaching.ts, 299, 20))

: Array.isArray;
>Array.isArray : Symbol(ArrayConstructor.isArray, Decl(lib.d.ts, --, --))
>Array : Symbol(Array, Decl(lib.d.ts, --, --), Decl(lib.d.ts, --, --))
>isArray : Symbol(ArrayConstructor.isArray, Decl(lib.d.ts, --, --))

function equalIC(a, b) {
>equalIC : Symbol(equalIC, Decl(fixSignatureCaching.ts, 300, 24))
Expand Down
10 changes: 5 additions & 5 deletions tests/baselines/reference/fixSignatureCaching.types
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,9 +893,9 @@ define(function () {
>'[object Array]' : "[object Array]"

isArray = 'isArray' in Array
>isArray = 'isArray' in Array ? function (value) { return Object.prototype.toString.call(value) === '[object Array]'; } : Array.isArray : (value: any) => boolean
>isArray = 'isArray' in Array ? function (value) { return Object.prototype.toString.call(value) === '[object Array]'; } : Array.isArray : any
>isArray : any
>'isArray' in Array ? function (value) { return Object.prototype.toString.call(value) === '[object Array]'; } : Array.isArray : (value: any) => boolean
>'isArray' in Array ? function (value) { return Object.prototype.toString.call(value) === '[object Array]'; } : Array.isArray : any
>'isArray' in Array : boolean
>'isArray' : "isArray"
>Array : ArrayConstructor
Expand All @@ -916,9 +916,9 @@ define(function () {
>'[object Array]' : "[object Array]"

: Array.isArray;
>Array.isArray : (arg: any) => arg is any[]
>Array : ArrayConstructor
>isArray : (arg: any) => arg is any[]
>Array.isArray : any
>Array : never
>isArray : any

function equalIC(a, b) {
>equalIC : (a: any, b: any) => boolean
Expand Down
159 changes: 159 additions & 0 deletions tests/baselines/reference/inKeywordTypeguard.errors.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
tests/cases/compiler/inKeywordTypeguard.ts(6,11): error TS2339: Property 'b' does not exist on type 'A'.
tests/cases/compiler/inKeywordTypeguard.ts(8,11): error TS2339: Property 'a' does not exist on type 'B'.
tests/cases/compiler/inKeywordTypeguard.ts(14,11): error TS2339: Property 'b' does not exist on type 'A'.
tests/cases/compiler/inKeywordTypeguard.ts(16,11): error TS2339: Property 'a' does not exist on type 'B'.
tests/cases/compiler/inKeywordTypeguard.ts(27,11): error TS2339: Property 'b' does not exist on type 'AWithOptionalProp | BWithOptionalProp'.
Property 'b' does not exist on type 'AWithOptionalProp'.
tests/cases/compiler/inKeywordTypeguard.ts(42,11): error TS2339: Property 'b' does not exist on type 'AWithMethod'.
tests/cases/compiler/inKeywordTypeguard.ts(49,11): error TS2339: Property 'a' does not exist on type 'never'.
tests/cases/compiler/inKeywordTypeguard.ts(50,11): error TS2339: Property 'b' does not exist on type 'never'.
tests/cases/compiler/inKeywordTypeguard.ts(52,11): error TS2339: Property 'a' does not exist on type 'AWithMethod | BWithMethod'.
Property 'a' does not exist on type 'BWithMethod'.
tests/cases/compiler/inKeywordTypeguard.ts(53,11): error TS2339: Property 'b' does not exist on type 'AWithMethod | BWithMethod'.
Property 'b' does not exist on type 'AWithMethod'.
tests/cases/compiler/inKeywordTypeguard.ts(62,11): error TS2339: Property 'b' does not exist on type 'A | C | D'.
Property 'b' does not exist on type 'A'.
tests/cases/compiler/inKeywordTypeguard.ts(64,11): error TS2339: Property 'a' does not exist on type 'B'.
tests/cases/compiler/inKeywordTypeguard.ts(72,32): error TS2339: Property 'b' does not exist on type 'A'.
tests/cases/compiler/inKeywordTypeguard.ts(74,32): error TS2339: Property 'a' does not exist on type 'B'.
tests/cases/compiler/inKeywordTypeguard.ts(82,39): error TS2339: Property 'b' does not exist on type 'A'.
tests/cases/compiler/inKeywordTypeguard.ts(84,39): error TS2339: Property 'a' does not exist on type 'B'.
tests/cases/compiler/inKeywordTypeguard.ts(94,26): error TS2339: Property 'a' does not exist on type 'never'.


==== tests/cases/compiler/inKeywordTypeguard.ts (17 errors) ====
class A { a: string; }
class B { b: string; }

function negativeClassesTest(x: A | B) {
if ("a" in x) {
x.b = "1";
~
!!! error TS2339: Property 'b' does not exist on type 'A'.
Copy link

@Jessidhia Jessidhia Dec 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'd really like to be able to use in for narrowing, but I'm not sure this is it...

Consider:

const foo = { a: true, b: '' }
negativeClassesTest(foo)

foo is only valid as a B, but it would pass the in check, and then be treated as an A; but its a key is not of type string.

This means that, within the branch, the type of x should still be A|B, but you are allowed to access .a; I just don't know what type .a should have. It probably should be any, which should trip --noImplicitAny if it's not used with a type narrowing expression.

Scenarios:

function negativeClassesTest(x: A | B) {
  if ("a" in x) {
    x.a // --noImplicitAny error
    const y: string = x.a // ideally a --noImplicitAny error, if it's possible to have "signalling any"s
    const z = x.a as string // accepted because it's a cast
    const u: string = typeof x.a === 'string' ? x.a : '' // accepted because it's narrowed
  }
}

It doesn't seem to be all that useful to work like this, but it at least allows you to access .a at all.

Bonus points, but probably hard to actually do in practice:

function negativeClassesTest(x: A | B) {
  if ("a" in x && typeof x.a === "string") {
    // x _should_ be an A!
  }
}

Alternative solution: Have a way to indicate that B has "not this key". Maybe, for example, interface B { a: never; b: string } should mean that B is not allowed to have an a key at all; even if it's set to a value of type never such as undefined!.

Copy link
Member

@RyanCavanaugh RyanCavanaugh Dec 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

We considered the soundness aspect and it's not very different from existing soundness issues around aliased objects with undeclared properties. IOW you can already write this code, error-free (even in flow!), which observably fails:

interface T { x: string, y?: number };
const s = { x: "", y: "uh oh" };
const not_t: { x: string } = s;
const unsound: T = not_t;
(unsound.y && unsound.y.toFixed());

The reality is that most unions are already correctly disjointed and don't alias enough to manifest the problem. Someone writing an in test is not going to write a "better" check; all that really happens in practice is that people add type assertions or move the code to an equally-unsound user-defined type predicate. On net I don't think this is any worse than the status quo (and is better because it induces fewer user-defined type predicates, which are error-prone if written using in due to a lack of typo-checking).

For automatically disjointed unions, see #14094.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kovensky im agree, this is nasty case, but it grows from the fact we have structural type system in place. If we, for example, had nominal type system, the info we provided in function definitions would exactly matching all its usages and we never had this problem in place.

Workaround is to give compiler more complete info about types given, like this:

class A { a: string }
class B { b: string }
class C { a: number; b: string }
function z(x: A|B|C){
	if("a" in x){
		var num_a: number = x.a; // error TS2322: Type 'string | number' is not assignable to type 'number'.
                                         // Type 'string' is not assignable to type 'number'.
		var str_a: string = x.a; // error TS2322: Type 'string | number' is not assignable to type 'string'.
                                         // Type 'number' is not assignable to type 'string'.
	}
}

} else {
x.a = "1";
~
!!! error TS2339: Property 'a' does not exist on type 'B'.
}
}

function positiveClassesTest(x: A | B) {
if ("a" in x) {
x.b = "1";
~
!!! error TS2339: Property 'b' does not exist on type 'A'.
} else {
x.a = "1";
~
!!! error TS2339: Property 'a' does not exist on type 'B'.
}
}

class AWithOptionalProp { a?: string; }
class BWithOptionalProp { b?: string; }

function positiveTestClassesWithOptionalProperties(x: AWithOptionalProp | BWithOptionalProp) {
if ("a" in x) {
x.a = "1";
} else {
x.b = "1";
~
!!! error TS2339: Property 'b' does not exist on type 'AWithOptionalProp | BWithOptionalProp'.
!!! error TS2339: Property 'b' does not exist on type 'AWithOptionalProp'.
}
}

class AWithMethod {
a(): string { return ""; }
}

class BWithMethod {
b(): string { return ""; }
}

function negativeTestClassesWithMembers(x: AWithMethod | BWithMethod) {
if ("a" in x) {
x.a();
x.b();
~
!!! error TS2339: Property 'b' does not exist on type 'AWithMethod'.
} else {
}
}

function negativeTestClassesWithMemberMissingInBothClasses(x: AWithMethod | BWithMethod) {
if ("c" in x) {
x.a();
~
!!! error TS2339: Property 'a' does not exist on type 'never'.
x.b();
~
!!! error TS2339: Property 'b' does not exist on type 'never'.
} else {
x.a();
~
!!! error TS2339: Property 'a' does not exist on type 'AWithMethod | BWithMethod'.
!!! error TS2339: Property 'a' does not exist on type 'BWithMethod'.
x.b();
~
!!! error TS2339: Property 'b' does not exist on type 'AWithMethod | BWithMethod'.
!!! error TS2339: Property 'b' does not exist on type 'AWithMethod'.
}
}

class C { a: string; }
class D { a: string; }

function negativeMultipleClassesTest(x: A | B | C | D) {
if ("a" in x) {
x.b = "1";
~
!!! error TS2339: Property 'b' does not exist on type 'A | C | D'.
!!! error TS2339: Property 'b' does not exist on type 'A'.
} else {
x.a = "1";
~
!!! error TS2339: Property 'a' does not exist on type 'B'.
}
}

class ClassWithUnionProp { prop: A | B }

function negativePropTest(x: ClassWithUnionProp) {
if ("a" in x.prop) {
let y: string = x.prop.b;
~
!!! error TS2339: Property 'b' does not exist on type 'A'.
} else {
let z: string = x.prop.a;
~
!!! error TS2339: Property 'a' does not exist on type 'B'.
}
}

class NegativeClassTest {
protected prop: A | B;
inThis() {
if ("a" in this.prop) {
let z: number = this.prop.b;
~
!!! error TS2339: Property 'b' does not exist on type 'A'.
} else {
let y: string = this.prop.a;
~
!!! error TS2339: Property 'a' does not exist on type 'B'.
}
}
}

class UnreachableCodeDetection {
a: string;
inThis() {
if ("a" in this) {
} else {
let y = this.a;
~
!!! error TS2339: Property 'a' does not exist on type 'never'.
}
}
}
Loading