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

[mattmccutchen]

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]

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]

@typescript-bot test this

[typescript-bot]

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]

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]

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

[DanielRosenwasser]

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

[RyanCavanaugh]

@typescript-bot test this again!

[typescript-bot]

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]

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

[RyanCavanaugh]

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

[mattmccutchen]

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]

@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]

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

[RyanCavanaugh]

Well I broke it...

[mattmccutchen]

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.

[weswigham]

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

[typescript-bot]

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]

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]

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

[sandersn]

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.