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

Covariant assignability of type guard functions is unsound #26981

Open
mattmccutchen opened this issue Sep 8, 2018 · 3 comments
Open

Covariant assignability of type guard functions is unsound #26981

mattmccutchen opened this issue Sep 8, 2018 · 3 comments
Labels
Committed The team has roadmapped this issue Help Wanted You can do this Suggestion An idea for TypeScript
Milestone

Comments

@mattmccutchen
Copy link
Contributor

TypeScript Version: master (af8e44a)

Search Terms: user-defined type guard predicate assignable assignability unsound covariant invariant

Code

class A {
    a: string;
}
class B extends A { 
    b: string;
}
function isB(x: {}): x is B { 
    return x instanceof B;
}
const isA: (x: {}) => x is A = isB;  // allowed, should be compile error

const x = <A | string>new A();
if (!isA(x)) {
    console.log(x.slice());  // runtime error
}

Expected behavior: Assignability error where marked.

Actual behavior: Successful compilation, runtime error.

Playground Link: link

Related Issues: #24865

If we want a kind of type guard function that is covariant, we need to not narrow when it returns false, i.e., a "true" result would be sufficient but not necessary for the value to be of the tested type. And we'd need a different syntax for the type of a type guard function that is necessary and sufficient compared to a type guard function that is sufficient but not necessary. One idea for the latter is (x: {}) => x is A | false. (A type guard function that is necessary but not sufficient would then be (x: {}) => x is A | true.) Getting all these variants supported would help us support conjunctions and disjunctions of type guard calls with other boolean expressions in #24865. If you like, this issue can be for the removal of the unsound assignability rule and I can file a separate suggestion for the new kinds of type guard functions.

@RyanCavanaugh
Copy link
Member

We should actually be invariant here, since a contravariant return type would be provably "wrong" since we assume type guards return true for their entire domain (see #15048 for support of non-exhaustive type guards).

I like the idea of : x is T | false as syntax for #15048.

This would be a breaking change so we'd have to investigate the impact

@RyanCavanaugh RyanCavanaugh added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Sep 17, 2018
@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Oct 9, 2018
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.2 milestone Oct 9, 2018
@RyanCavanaugh
Copy link
Member

@mattmccutchen interested in putting up a PR? We're willing to see how big of a break it would be (expectation is small)

mattmccutchen added a commit to mattmccutchen/TypeScript that referenced this issue Oct 10, 2018
Fix one break in the compiler, and add a workaround to test case
thisPredicateFunctionQuickInfo01, which used a code pattern that no
longer works.

Fixes microsoft#26981.
@mattmccutchen
Copy link
Contributor Author

PR is #27686. Note the interesting problem with the thisPredicateFunctionQuickInfo01 test case.

mattmccutchen added a commit to mattmccutchen/TypeScript that referenced this issue Oct 16, 2018
Fix one break in the compiler, and add a workaround to test case
thisPredicateFunctionQuickInfo01, which used a code pattern that no
longer works.

Fixes microsoft#26981.
@DanielRosenwasser DanielRosenwasser added the In Discussion Not yet reached consensus label Oct 30, 2018
mattmccutchen added a commit to mattmccutchen/TypeScript that referenced this issue Feb 10, 2019
- Fix one break in the compiler.

- Type guards like `isNetworked(): this is (Networked & this)` have been
  obsolete in favor of `isNetworked(): this is Networked` ever since
  narrowing was enhanced to take an intersection in a370908, and the
  first form now causes a problem: a subclass fails to be a subtype of
  its superclass because `this` appears in a non-covariant position.  So
  replace all occurrences of the first form with the second form in the
  test suite.

Fixes microsoft#26981.
@RyanCavanaugh RyanCavanaugh removed this from the TypeScript 3.4.0 milestone Mar 14, 2019
@RyanCavanaugh RyanCavanaugh removed the In Discussion Not yet reached consensus label Mar 14, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Aug 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Help Wanted You can do this Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants