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

Problem with inference for this['prop'] with conditional types #27014

Open
dfreeman opened this issue Sep 10, 2018 · 5 comments
Open

Problem with inference for this['prop'] with conditional types #27014

dfreeman opened this issue Sep 10, 2018 · 5 comments
Labels
Bug A bug in TypeScript Domain: Conditional Types The issue relates to conditional types
Milestone

Comments

@dfreeman
Copy link
Contributor

TypeScript Version: typescript@3.1.0-dev.20180907
Search Terms: this type, conditional type

Code

type Wrapped<T> = { ___secret: T };
type Unwrap<T> = T extends Wrapped<infer U> ? U : T;

declare function set<T, K extends keyof T>(obj: T, key: K, value: Unwrap<T[K]>);

class Foo {
    prop: Wrapped<string>;

    method() {
        set(this, 'prop', 'hi'); // <-- type error
    }
}

set(new Foo(), 'prop', 'hi'); // <-- typechecks

Expected behavior:
Both calls to set (the one in the method with this and the external one with new Foo()) pass typechecking.

Actual behavior:
The external call passes, but the one inside method fails with Argument of type '"hi"' is not assignable to parameter of type 'Unwrap<this["prop"]>'..

If I explicitly annotate this: Foo in the method signature or set the type parameters set<Foo, 'prop'>(...), everything passes.

We've run into this in the Ember type declarations, and have had to weaken the types for the time being to work around it.

Playground Link: available here

Related Issues: I wasn't able to find anything that seemed related to this issue.

mike-north added a commit to mike-north/DefinitelyTyped that referenced this issue Sep 15, 2018
mike-north added a commit to mike-north/DefinitelyTyped that referenced this issue Sep 15, 2018
RyanCavanaugh pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this issue Sep 17, 2018
- [x] Use a meaningful title for the pull request. Include the name of the package modified.
- [x] Test the change in your own code. (Compile and run.)
- [x] Add or edit tests to reflect the change. (Run with `npm test`.)
- [x] Follow the advice from the [readme](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#make-a-pull-request).
- [x] Avoid [common mistakes](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/README.md#common-mistakes).
- [x] Run `npm run lint package-name` (or `tsc` if no `tslint.json` is present).

If changing an existing definition:
- [x] Provide a URL to documentation or source code which provides context for the suggested changes: https://www.emberjs.com/api/ember/3.4/functions/@ember%2Fobject/set
- [x] Increase the version number in the header if appropriate.
- [x] If you are making substantial changes, consider adding a `tslint.json` containing `{ "extends": "dtslint/dt.json" }`.

---
- Fixes typed-ember/ember-cli-typescript#308
- Workaround due to breaking change microsoft/TypeScript#26120
- Would much much cleaner w/ a fix for microsoft/TypeScript#27014
@weswigham weswigham added the Bug A bug in TypeScript label Sep 17, 2018
@weswigham
Copy link
Member

weswigham commented Sep 17, 2018

From what I know of conditional types, the root issue is that we don't perform any inferences to conditional types and this in the class body is still generic, so requires successful inference to work correctly. #27012 may help here, but I'm not sure. It's certainly affected by the same area.

@dfreeman
Copy link
Contributor Author

@weswigham thanks for the ping—I pulled and built master since that PR was merged, but still see the same error message with that sample code

@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.1.3 milestone Oct 9, 2018
@weswigham weswigham added the Domain: Conditional Types The issue relates to conditional types label Oct 16, 2018
@weswigham
Copy link
Member

weswigham commented Oct 16, 2018

So, I've looked over it again, and while I do have a "fix" known, the desired behavior is unsound. Given

type Wrapped<T> = { ___secret: T };
type Unwrap<T> = T extends Wrapped<infer U> ? U : T;

declare function set<T, K extends keyof T>(obj: T, key: K, value: Unwrap<T[K]>): void;

class Foo {
    prop: Wrapped<string>;

    method() {
        set(this, 'prop', 'hi'); // <-- type error
    }
}

Consider subtype:

class Bar extends Foo {
  prop: Wrapped<"ohno">;
}
const x = new Bar();
x.method(); // We just set `prop` to `"hi"` even though this component only takes an `"ohno"`!

The "read" case of these types works fine:

type Wrapped<T> = { ___secret: T };
type Unwrap<T> = T extends Wrapped<infer U> ? U : T;

declare function get<T, K extends keyof T>(obj: T, key: K): Unwrap<T[K]>;

class Foo {
    prop: Wrapped<string>;

    method() {
        return get(this, 'prop'); // <-- fine
    }
}

class Bar extends Foo {
    prop: Wrapped<"ohyeah">;
}

const a = get(new Foo(), 'prop'); // <-- also fine

const b = get(new Bar(), 'prop'); // <-- also fine

for the set to be typesafe, you must set a type which varies appropriately with the class:

type Wrapped<T> = { ___secret: T };
type Unwrap<T> = T extends Wrapped<infer U> ? U : T;

declare function set<T, K extends keyof T>(obj: T, key: K, value: Unwrap<T[K]>): void;

class Foo {
    prop: Wrapped<string>;
    defaultProp: Unwrap<this["prop"]>; // note use of generic `this` type

    method() {
        set(this, 'prop', this.defaultProp); // <-- fine
    }
}

class Bar extends Foo {
    prop: Wrapped<"ohyeah">;
}

set(new Foo(), 'prop', "ok"); // <-- also fine

set(new Bar(), 'prop', "ohyeah"); // <-- also fine

so @DanielRosenwasser as is, this is working as intended.

@dfreeman
Copy link
Contributor Author

dfreeman commented Oct 18, 2018

You're right that the specific example I gave was unsound with the subclassing you're describing, but as far as I can tell, that's not unique to anything with conditional types or the wrapping that was going on:

class Foo {
  prop: string;
  method() {
    this.prop = 'hi';
  }
}

class Bar extends Foo {
  prop: 'ohno';
}

const x = new Bar();
x.method(); // We just set `prop` to `"hi"` even though this component only takes an `"ohno"`!

@weswigham
Copy link
Member

weswigham commented Oct 18, 2018

For partly historical reasons, we have a rule that allows the unsound writes for primitive member types since thinking about variance in simple code is a heavy burden (and tbh it predates us tracking variance), however generics and indexed accesses in general track variance strictly and disapprove of the unsound assignments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Domain: Conditional Types The issue relates to conditional types
Projects
None yet
Development

No branches or pull requests

4 participants