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

Remove diagnostic dependent output in structuredTypeRelatedTo #29817

Merged

Conversation

weswigham
Copy link
Member

Fixes #29393

Also unified the special-case (void covariant param) handling between aliases and interfaces, and removed the early bail on no strictFunctionTypes from getVariances so we can still measure independent type parameters when strictFunctionTypes isn't on (as now that the variance result is always consistently reported, this flaw was pretty obvious).

@phiresky
Copy link

Just tried this, and it does not seem to fix my issue (#29393) (same error as before)

here's a clonable example: https://gist.github.com/phiresky/4584e5d36d5fbae37e6afb07e3076536

@RyanCavanaugh
Copy link
Member

@weswigham please check if this fixes #29901 as well

@weswigham
Copy link
Member Author

#29901 seems to be an unrelated issue to do with our subtype relationship.

@weswigham
Copy link
Member Author

weswigham commented Feb 19, 2019

@phiresky

Just tried this, and it does not seem to fix my issue

By that I assume you mean "now no matter what I try there's a type error"? #29698 tracks altering the change that makes that ToA comparison an error. I was focused on making the result consistent here (since adding an "unrelated" line of code should never change the error result of another line). 😄

@weswigham weswigham merged commit eafff75 into microsoft:master Feb 19, 2019
@weswigham weswigham deleted the remove-diagnostic-dependent-output branch February 19, 2019 19:39
@phiresky
Copy link

phiresky commented Feb 19, 2019

@weswigham

Just tried this, and it does not seem to fix my issue

By that I assume you mean "now no matter what I try there's a type error"? #29698 tracks altering the change that makes that ToA comparison an error. I was focused on making the result consistent here (since adding an "unrelated" line of code should never change the error result of another line).

Not that that would be good for me either (since then I'm still stuck on 3.0), but no I actually mean that with this branch, it's still exactly the same as before, it shows an error only when commenting that other line.

@weswigham weswigham mentioned this pull request Feb 19, 2019
@weswigham
Copy link
Member Author

@phiresky I'm not seeing it?:
deletecomment

You sure you have the typescript.tsdk workspace setting set up correctly to point at the new version you build off this PR?

@phiresky
Copy link

phiresky commented Feb 19, 2019

I actually tried it from the command line not VSCode, just like I did the bisect

(git clone https://github.com/Microsoft/TypeScript; cd TypeScript; g fetch origin pull/29817/head:zz; git checkout zz; yarn; gulp tsc; cd ..; git clone https://gist.github.com/phiresky/4584e5d36d5fbae37e6afb07e3076536; cd 45*; yarn; node ../TypeScript/lib/tsc.js)

but i'm not 100% confident I did it correctly

@phiresky
Copy link

phiresky commented Feb 19, 2019

@weswigham
Nevermind, I used ../TypeScript/lib/tsc.js instead of ../TypeScript/built/local/tsc.js which I guess is a stable version of the compiler.

So regarding my actual issue #29393 @weswigham , should I open a new one or is that maybe the same as #29698?

Here's my library that is broken by these changes btw: https://github.com/phiresky/typed-socket.io (errors in this file in TS>3.0)

@weswigham
Copy link
Member Author

weswigham commented Feb 19, 2019

Actually, the error is in some respects correct... io-ts's Type type is invariant in it's first argument - meaning Type<A> is never related to Type<B> (and vice-versa) if A !== B. This is the case because that type parameter is, used in both an argument position (encode) and an output position (validate) (also, once this PR is in, it is used in a type predicate (is), which forces it to be invariant). This invariance is transferred to T in your ToA helper by the usage of T in T[K] in Type<T[K]>.

Long story short, a Type<A> and a Type<B> don't share the same relationship A and B have because of all the helper callbacks on Type, which, in turn, means a ToA<A> and a ToA<B> are also unrelated.

"strictFunctionTypes": false can silence the issue; but it is an issue - a NeededInfo<ToB<typeof ASchema>> can't be assignable to a NeededInfo<{}>, since as two different instantiations of the internal ToA are necessarily unrelated, two differing instantiations of NeededInfo are unrelated.

And that's why the error is "correct".

Now, I say that, but the mapped type assignability rules in higher order aren't keen on one case: mapping over the empty object always produces the empty object (so the variances of the type parameters no longer matter once the key set being mapped over is empty). This simplification/decomposition isn't reflected in mapped type higher order relationships (this would require expressing a dependent variance of some kind), so ends up not being reflected by variance measures. At it's heart, that is the same underlying issue as #29698 (though admittedly slightly different, since it relies on an edgecase around {}, rather than the behavior of index signature assignability).

@phiresky
Copy link

phiresky commented Feb 20, 2019

@weswigham

Actually, the error is in some respects correct... io-ts's Type type is invariant in it's first argument - meaning Type<A> is never related to Type<B> (and vice-versa) if A !== B. This is the case because that type parameter is, used in both an argument position (encode) and an output position (validate)

You are right. I managed to solve my problem by replacing instances of where I had {[name: string]: any} (which were {} in the minimal example because it gives the same results) with any, and replacing my generics defaults with any instead of the expected base type. Then the inference works as expected on the current TypeScript version. Pretty hard to find that solution [1], but it does make sense in the end.

I reason I wrote them as {[name: string]: any} in the first place is to avoid any as much as possible, because any loves to propagate everywhere when making a tiny mistake in this kind of type soup. But i guess TS now (correctly) knows that those generics need to be invariant not just covariant, and the only type TS accepts as invariant to any generic is any.


[1] because this is the error message i get on the actual instance of the problem:

Type 'NeededInfo<MyServerDefinition, SimpleNamespace<ToCompiletime<{ ServerMessages: { chatMessage: Type<ChatMessage, ChatMessage, unknown>; history: Type<ChatMessage[], ChatMessage[], unknown>; }; ClientRPCs: { postMessage: { request: ExactC<...>; response: Type<...>; }; }; ClientMessages: {}; }>>>' does not satisfy the constraint 'NeededInfo<ServerDefinition, FullNamespaceSchema>'.
  Types of property 'RuntimeSchema' are incompatible.
    Type 'FromCompiletime<SimpleNamespace<ToCompiletime<{ ServerMessages: { chatMessage: Type<ChatMessage, ChatMessage, unknown>; history: Type<ChatMessage[], ChatMessage[], unknown>; }; ClientRPCs: { postMessage: { request: ExactC<TypeC<{ message: StringC; channel: UnionC<...>; }>>; response: Type<...>; }; }; ClientMessages:...' is not assignable to type 'FromCompiletime<FullNamespaceSchema>'.
      Type 'FullNamespaceSchema' is not assignable to type 'SimpleNamespace<ToCompiletime<{ ServerMessages: { chatMessage: Type<ChatMessage, ChatMessage, unknown>; history: Type<ChatMessage[], ChatMessage[], unknown>; }; ClientRPCs: { postMessage: { request: ExactC<TypeC<{ message: StringC; channel: UnionC<...>; }>>; response: Type<...>; }; }; ClientMessages: {}; }>>'.ts(2344)

@sandersn
Copy link
Member

This breaks ember's types on DT in a very confusing way (which may be correct!). Go to ~/DefinitelyTyped/types/ember__application and run tsc. I'll provide a repro shortly once I understand the problem better.

@sandersn
Copy link
Member

ember wants to do the unsound thing where they have:

class Base {
  resolver: BaseResolver
}
class Derived extends Base {
  resolver: DerivedResolver
}

where DerivedResolver extends BaseResolver. But resolver is now invariant.

Here's a minified repro. I'm not sure how interesting it is:

declare class ComputedProperty<Get, Set=Get> {
    // Necessary in order to avoid losing type information
    //    see: https://github.com/typed-ember/ember-cli-typescript/issues/246#issuecomment-414812013
    private ______getType: Get;
    private ______setType: Set;
}

type UnwrapComputedPropertyGetter<T> = T extends ComputedProperty<infer U, any> ? U : T;
type UnwrapComputedPropertyGetters<T> = { [P in keyof T]: UnwrapComputedPropertyGetter<T[P]> };

type UnwrapComputedPropertySetter<T> = T extends ComputedProperty<any, infer V> ? V : T;
type UnwrapComputedPropertySetters<T> = {[P in keyof T]: UnwrapComputedPropertySetter<T[P]>};

interface Observable {
    /**
     * To get the values of multiple properties at once, call `getProperties`
     * with a list of strings or an array:
     */
    getProperties<K extends keyof this>(list: K[]): Pick<UnwrapComputedPropertyGetters<this>, K>;
    setProperties<K extends keyof this>(hash: Pick<this, K>): Pick<UnwrapComputedPropertySetters<this>, K>;
}

type Ctor<T> = new (...args: any[]) => T;
declare class Mixin<T> { }
declare function ext<T>(arg1: Mixin<T>): Ctor<T>;

declare const MixedO: Mixin<Observable>;
declare class Base extends ext(MixedO) {}
declare class Derived extends Base {
    p: number;
}
declare let r: Base
declare let dr: Derived
r = dr // unexpected error here

@sandersn
Copy link
Member

jasmine is also broken because of this, with code

interface Foo {
    a: number;
    b: number;
    bar: string;
}
interface ObjectContaining<T> {
  new (sample: Partial<T>): Partial<T>
}
declare let cafoo: ObjectContaining<{ a: number, foo: number }>;
declare let cfoo: ObjectContaining<Foo>;
cfoo = cafoo;
cafoo = cfoo;

I'm not sure why this worked before.

@weswigham
Copy link
Member Author

As for the ember bit I'm pretty sure it's because UnwrapComputedPropertyGetter is invariant on its parameter type as we now require check and extends types to be identical. That bubbles up to Observable, and since Observable uses a this type and is then used as the mixin base, affects the classes. As for "why wasn't this an error before" - because an instantiation inside an extends clause and subsequent constraint comparison caused us to perform the comparison with diagnostic reporting off, producing an incorrect result that didn't rely on variances an cacheing it (in effect triggering this bug, exactly).

@weswigham
Copy link
Member Author

I'm not sure why this worked before

Same. Shouldn't weak type checking have at least forbidden the one direction of that? T is obviously invariant, since it's in both an argument and return position. I think I can understand how it triggered this bug tho - assignments are a place where we do the assignment check with no error reporting first (so we can decompose the expression for a better error), so that'd definitely be why it cached a structural result and ignored variances.

@sandersn
Copy link
Member

Some other DT breaks:

  • material-ui
  • react-jss
  • recompose

I haven't investigated these, but they fail at exactly the same bisected commit.

@chriskrycho
Copy link

I'm not positive, but this may be the same issue I've seen in a library, reproduction here. If it's not the same, it's another thing hit by changes in variance—but the symptoms are very similar, and the issue definitely one of invariance. I can see how minimal the definitions for True Myth's Maybe have to be to trigger this, if that's helpful: it's definitely not triggered with a minimal subset of that Maybe type.

@fictitious
Copy link

Here is another example, similar to #29698

type Base = Record<string, {}>;

interface Wrapper<B extends Base> {
    b: B;
    f(): keyof B; // makes Wrapper covariant in keyof B
//    in reality it was map: Map<Extract<keyof B, string>, {}>;
}

declare function createWrapper<B extends Base>(b: B): Wrapper<B>;

declare function acceptRecordOfWrappers<R extends Record<string, Wrapper<Base>>>(r: R): void;

acceptRecordOfWrappers({
    w: createWrapper({a: {}})
});

// Type 'Wrapper<{ a: {}; }>' is not assignable to type 'Wrapper<Record<string, {}>>'.
//  Property 'a' is missing in type 'Record<string, {}>' but required in type '{ a: {}; }'.

Some notes:

  1. I'm getting this error even with --strictFunctionTypes turned off

  2. When I started getting this error, it would be helpful if the message said this error happened while trying to uphold the variance for keyof B as stated in Wrapper or something like that

  3. Checking assignability to Wrapper<Record<string, {}>> is not really what I want, because when I say

R extends Record<string, Wrapper<Base>>

I actually mean R extends Record<string, Something that extends Wrapper<Base>> where Something is going to be different type extending Wrapper with different B for different keys, and I want TypeScript to infer strict types for keyof R and actual value types from the argument passed to acceptRecordOfWrappers.

@sandersn
Copy link
Member

Pretty sure yargs has the problem too:

declare let yb: yargs.Argv<{ b: number }>
declare let yab: yargs.Argv<{ a: string, b: number }>
yb = yab // seems fine...

...Except that one of Argv's methods uses T in a contravariant position.

@sandersn
Copy link
Member

sandersn commented Feb 25, 2019

Poking at material-ui, the break there is on React's ComponentClass. I'm not sure why no other react tests break -- it's hard to believe that all other uses are invariant.

Edit: it's because material-ui has strictFunctionTypes: false, so all (most?) other react dependents have been fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants