Make type guard function types invariant in the type guarded for. #27686

mattmccutchen · 2018-10-10T15:45:56Z

Fix one break in the compiler, and add a workaround to test case
thisPredicateFunctionQuickInfo01, which used a code pattern that no
longer works.

Fixes #26981.

mattmccutchen · 2018-10-10T17:54:23Z

So we have a break in @types/vinyl. TypeScript team, will you run the RWC suite to get a sense of whether this change is going to be viable before I spend more time working on the @types/vinyl issue?

@typescript-bot test this

RyanCavanaugh · 2018-10-10T18:54:57Z

@typescript-bot test this

typescript-bot · 2018-10-10T18:55:00Z

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 1ac1528. You can monitor the build here. It should now contribute to this PR's status checks.

RyanCavanaugh · 2018-10-10T22:33:43Z

RWC breaks which were all from OSS projects:

mobx

    /* 'var' fixes small-build issue */
    export var isObservableMap = createInstanceofPredicate("ObservableMap", ObservableMap) as (
                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        thing: any
    ~~~~~~~~~~~~~~
    ) => thing is ObservableMap<any>
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2352: Conversion of type '(x: any) => x is ObservableMap<{}>' to type '(thing: any) => thing is ObservableMap<any>' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.
!!! error TS2352:   Type predicate 'x is ObservableMap<{}>' is not assignable to 'thing is ObservableMap<any>'.

rxjs

10 hits on this problem:

      // if the first and only other argument besides the resultSelector is an array
      // assume it's been called with `combineLatest([obs1, obs2, obs3], project)`
      if (observables.length === 1 && isArray(observables[0])) {
                                      ~~~~~~~~~~~~~~~~~~~~~~~
!!! error TS2349: Cannot invoke an expression whose type lacks a call signature. Type '((arg: any) => arg is any[]) | (<T>(x: any) => x is T[])' has no compatible call signatures.
        observables = <Array<Observable<any>>>observables[0];
      }

where isArray was

export const isArray = Array.isArray || (<T>(x: any): x is T[] => x && typeof x.length === 'number');

vs code

    export function createMonacoBaseAPI(): typeof monaco {
    	return {
    		editor: undefined,
    		languages: undefined,
    		CancellationTokenSource: CancellationTokenSource,
    		Emitter: Emitter,
    		KeyCode: KeyCode,
    		KeyMod: KeyMod,
    		Position: Position,
    		Range: Range,
    		Selection: Selection,
    		SelectionDirection: SelectionDirection,
    		Severity: Severity,
    		Promise: TPromise,
    		~~~~~~~
!!! error TS2322: Type 'typeof TPromise' is not assignable to type 'typeof Promise'.
!!! error TS2322:   Types of property 'is' are incompatible.
!!! error TS2322:     Type '(value: any) => value is Thenable<any>' is not assignable to type '(value: any) => value is monaco.Thenable<any>'.
!!! error TS2322:       Type predicate 'value is Thenable<any>' is not assignable to 'value is Thenable<any>'.
!!! related TS6500 vs/monaco.d.ts:62:15: The expected type comes from property 'Promise' which is declared here on type 'typeof monaco'
    		Uri: URI,
    		~~~
!!! error TS2322: Type 'typeof URI' is not assignable to type 'typeof Uri'.
!!! error TS2322:   Types of property 'isUri' are incompatible.
!!! error TS2322:     Type '(thing: any) => thing is URI' is not assignable to type '(thing: any) => thing is Uri'.
!!! error TS2322:       Type predicate 'thing is URI' is not assignable to 'thing is Uri'.
!!! related TS6500 vs/monaco.d.ts:131:15: The expected type comes from property 'Uri' which is declared here on type 'typeof monaco'
    		Token: Token
    	};
    }

mattmccutchen · 2018-10-14T13:14:17Z

What's the conclusion? Should I be working on contributing fixes for all the RWC breaks?

src/compiler/checker.ts

DanielRosenwasser · 2018-10-30T22:38:07Z

We'll need to review this in a backlog slog or design meeting.

RyanCavanaugh · 2019-01-25T19:59:14Z

@typescript-bot test this again!

typescript-bot · 2019-01-25T19:59:15Z

Heya @RyanCavanaugh, I've started to run the extended test suite on this PR at 7702c41. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2019-01-25T22:39:24Z

@RyanCavanaugh this branch needs a sync with master for an accurate rwc run 💓

RyanCavanaugh · 2019-02-01T21:48:53Z

@mattmccutchen if you can get merged with master + CI green we can do an RWC run and then hopefully merge. Thanks!

- 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.

mattmccutchen · 2019-02-10T19:53:21Z

Updated to master. For reasons I haven't investigated, the @types/vinyl breakage that we previously saw in the CI is no longer triggered by the CI even if I set USE_BUILT=true, but I can still reproduce it if I manually run node built/local/tsc.js --noEmit node_modules/\@types/vinyl/index.d.ts. For all I know, this change may affect DefinitelyTyped packages other than this one that happened to previously be involved in the CI. Does the RWC suite cover all of DefinitelyTyped, or is there another procedure to evaluate the impact on DefinitelyTyped? It's annoying for the next contributor to a DefinitelyTyped package to have to fix an unrelated problem about which they have no prior knowledge in order to pass the test with typescript@next; I've been on the receiving end of that before.

weswigham · 2019-03-12T21:39:59Z

@mattmccutchen sorry to continue kicking this slowly down the line; we probably won't ship this in 3.4 at this point; but could you resync this branch so we can run this branch against DT on our new infra?

RyanCavanaugh · 2019-03-13T17:25:03Z

@weswigham @mattmccutchen I pushed a new baseline for the conflict

RyanCavanaugh · 2019-03-14T16:58:34Z

Well I broke it...

mattmccutchen · 2019-03-14T17:04:41Z

I don't know why we didn't see this CI failure before; I haven't investigated what changed. Anyway, the problem is with this code:

interface Crate<T> {
    // ...
    isPackedTight(): this is (this & {extraContents: T});
}

The this on the right side of the type predicate is invariant, which causes lots of trouble: it makes a subclass fail to be assignable to a superclass and makes all type parameters of the class invariant. Fortunately, it can just be removed because narrowing automatically takes an intersection when appropriate.

We're left with the invariant occurrence of T in {extraContents: T}, where the test wants T to be covariant. Let's recall how covariance here would lead to unsoundness. Assuming that B is a subtype of A, someone could create an implementation of Crate<B> & {extraContents: A} where the object in the extraContents property is an A that is not a B, and accordingly, the isPackedTight method returns false. If a user could upcast this object to Crate<A> & {extraContents: A} | SomeOtherType and then narrow based on isPackedTight, which still returns false, they would get SomeOtherType, which is wrong.

One solution would be to use a one-sided type guard (#15048), if and when those are available. Another is to write:

interface Crate<T> {
    // ...
    isPackedTight<U>(this: this & Crate<U>): this is {extraContents: U};
}

Now the type predicate no longer refers to T, so T is covariant again. And the isPackedTight method of an implementing class is required to conform to the this is {extraContents: U} two-sided type predicate for every U such that the object is a Crate<U>; since the type parameter of Crate is covariant, this means any supertype of the T in the implements clause of the class. This rules out the contrived unsound scenario above while allowing the common scenario where a false return from isPackedTight means that the extraContents property doesn't exist at all.

FWIW, I applied the second solution to fix the CI. Of course, the more important thing is what we find in the RWC and DefinitelyTyped tests.

The Crate<T> class had a "this" type predicate that referenced the type parameter T, preventing T from being covariant, which caused undesired behavior elsewhere. Apply the workaround described here: microsoft#27686 (comment)

weswigham · 2019-04-15T20:13:14Z

@typescript-bot run dt & @typescript-bot test this

typescript-bot · 2019-04-15T20:13:17Z

Heya @weswigham, I've started to run the extended test suite on this PR at 8de6b18. You can monitor the build here. It should now contribute to this PR's status checks.

typescript-bot · 2019-04-15T20:13:17Z

Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at 8de6b18. You can monitor the build here. It should now contribute to this PR's status checks.

weswigham · 2019-04-15T22:13:25Z

This seems to break vinyl on DT, which is in pretty wide usage.

sandersn · 2020-03-05T21:51:35Z

This PR hasn't seen any activity for quite a while, so I'm going to close it to keep the number of open PRs manageable. Feel free to open a fresh PR or continue the discussion here.

mattmccutchen mentioned this pull request Oct 10, 2018

Covariant assignability of type guard functions is unsound #26981

Open

weswigham reviewed Oct 16, 2018

View reviewed changes

src/compiler/checker.ts Outdated Show resolved Hide resolved

mattmccutchen force-pushed the issue-26981 branch from 1ac1528 to 7702c41 Compare October 16, 2018 23:45

RyanCavanaugh self-assigned this Jan 24, 2019

DanielRosenwasser mentioned this pull request Jan 25, 2019

Design Meeting Notes, 1/25/2019 #29588

Closed

mattmccutchen force-pushed the issue-26981 branch from 7702c41 to 07cf82a Compare February 10, 2019 19:32

weswigham mentioned this pull request Feb 19, 2019

Remove diagnostic dependent output in structuredTypeRelatedTo #29817

Merged

Regen baseline

6f0fcef

Merge remote-tracking branch 'upstream/master' into issue-26981

d47dbb7

Fix thisPredicateFunction{Completions,QuickInfo}02 tests.

8de6b18

The Crate<T> class had a "this" type predicate that referenced the type parameter T, preventing T from being covariant, which caused undesired behavior elsewhere. Apply the workaround described here: microsoft#27686 (comment)

mattmccutchen force-pushed the issue-26981 branch from 76d0ba5 to 8de6b18 Compare March 16, 2019 01:17

RyanCavanaugh assigned weswigham and unassigned RyanCavanaugh Apr 23, 2019

sandersn added the For Backlog Bug PRs that fix a backlog bug label Feb 1, 2020

sandersn closed this Mar 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make type guard function types invariant in the type guarded for. #27686

Make type guard function types invariant in the type guarded for. #27686

mattmccutchen commented Oct 10, 2018

mattmccutchen commented Oct 10, 2018

RyanCavanaugh commented Oct 10, 2018

typescript-bot commented Oct 10, 2018 •

edited

Loading

RyanCavanaugh commented Oct 10, 2018 •

edited

Loading

mattmccutchen commented Oct 14, 2018

DanielRosenwasser commented Oct 30, 2018

RyanCavanaugh commented Jan 25, 2019

typescript-bot commented Jan 25, 2019 •

edited

Loading

weswigham commented Jan 25, 2019

RyanCavanaugh commented Feb 1, 2019

mattmccutchen commented Feb 10, 2019

weswigham commented Mar 12, 2019

RyanCavanaugh commented Mar 13, 2019

RyanCavanaugh commented Mar 14, 2019

mattmccutchen commented Mar 14, 2019 •

edited

Loading

weswigham commented Apr 15, 2019

typescript-bot commented Apr 15, 2019 •

edited

Loading

typescript-bot commented Apr 15, 2019 •

edited

Loading

weswigham commented Apr 15, 2019

sandersn commented Mar 5, 2020

Make type guard function types invariant in the type guarded for. #27686

Make type guard function types invariant in the type guarded for. #27686

Conversation

mattmccutchen commented Oct 10, 2018

mattmccutchen commented Oct 10, 2018

RyanCavanaugh commented Oct 10, 2018

typescript-bot commented Oct 10, 2018 • edited Loading

RyanCavanaugh commented Oct 10, 2018 • edited Loading

mobx

rxjs

vs code

mattmccutchen commented Oct 14, 2018

DanielRosenwasser commented Oct 30, 2018

RyanCavanaugh commented Jan 25, 2019

typescript-bot commented Jan 25, 2019 • edited Loading

weswigham commented Jan 25, 2019

RyanCavanaugh commented Feb 1, 2019

mattmccutchen commented Feb 10, 2019

weswigham commented Mar 12, 2019

RyanCavanaugh commented Mar 13, 2019

RyanCavanaugh commented Mar 14, 2019

mattmccutchen commented Mar 14, 2019 • edited Loading

weswigham commented Apr 15, 2019

typescript-bot commented Apr 15, 2019 • edited Loading

typescript-bot commented Apr 15, 2019 • edited Loading

weswigham commented Apr 15, 2019

sandersn commented Mar 5, 2020

typescript-bot commented Oct 10, 2018 •

edited

Loading

RyanCavanaugh commented Oct 10, 2018 •

edited

Loading

typescript-bot commented Jan 25, 2019 •

edited

Loading

mattmccutchen commented Mar 14, 2019 •

edited

Loading

typescript-bot commented Apr 15, 2019 •

edited

Loading

typescript-bot commented Apr 15, 2019 •

edited

Loading