Skip to content

property accessor creates intersection type instead of union #31694

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

Closed
simonhaenisch opened this issue May 31, 2019 · 8 comments
Closed

property accessor creates intersection type instead of union #31694

simonhaenisch opened this issue May 31, 2019 · 8 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@simonhaenisch
Copy link

simonhaenisch commented May 31, 2019

TypeScript Version: 3.5.1 (also happens with @next)

Search Terms: ternary, ternary intersection/union, property accessor intersection/union

Code

interface FooBar {
  foo: string;
  bar: number;
}

const set = (foobar: FooBar, key: 'foo' | 'bar', value: string) => {
  foobar[key] = key === 'foo' ? value : Number(value);
};

or

class FooBar {
  foo?: string;
  bar?: number;

  set(key: 'foo' | 'bar', value: string) {
    this[key] = key === 'foo' ? value : Number(value);
  }
}

Expected behavior:

Works (it did up to version 3.4.5).

Actual behavior:

An intersection type is inferred for foobar[key] (or this[key]), however I think it should be a union type.

index.ts:7:3 - error TS2322: Type 'string | number' is not assignable to type 'string & number'.
  Type 'string' is not assignable to type 'string & number'.
    Type 'string' is not assignable to type 'number'.

7   foobar[key] = key === 'foo' ? value : Number(value);
    ~~~~~~~~~~~
index.ts:6:3 - error TS2322: Type 'string | number' is not assignable to type 'undefined'.
  Type 'string' is not assignable to type 'undefined'.

6   this[key] = key === 'foo' ? value : Number(value);
    ~~~~~~~~~

Playground Link:

Useless because this used to work up to version 3.4.5 (and the playground is stuck at version 3.3.3).

Related Issues:

Maybe #9239

If I use an if/else instead of the ternary operator, it works because it's easier to figure out the type of this[key]:

if (key === 'foo') {
  this[key] = value;
} else {
  this[key] = Number(value);
}
@krryan
Copy link

krryan commented May 31, 2019

Can narrow the versions down here more: this worked correctly in 3.5.0-dev.20190523 and fails in 3.5.0-dev.20190525.

The conditional workaround does not work in our case because we’re dealing with a generic, so we can’t narrow like that. We have resorted to @ts-ignore which makes us very uncomfortable.

@jack-williams
Copy link
Collaborator

jack-williams commented May 31, 2019

This is caused by #30769, and I think this is working as intended.

An intersection is inferred for this[key] because it occurs in a write position, and therefore you must be able to safely write to all possible keys denoted by key.

The narrowing you apply to key occurs in the RHS expression of the assignment, and is therefore invisible to the assignment itself. The checker simply sees the type of the RHS expression which is string | number.

The reason the if works is because the narrowing of key contains the assignment. At the point of the assignment key will have been narrowed to a single key and therefore you know precisely what value is safe to assign - there is no need to be conservative and assume you could write to all possible keys.

@krryan
Copy link

krryan commented May 31, 2019

Then this is a huge problem, because it’s “sound” and “conservative” to the point that we can’t use it.
We have a generic function that takes something like <In extends { kind: string }, Out extends { kind: string }, OutKind extends Out['kind']>(in: In, handle: (value: { kind: In }) => Out[], outKinds: OutKind[]). The whole point of the constraint is to ensure that the final array actually covers the result of the callback function (we need to know at runtime, before calling the callback, what results we can expect from it). But now Out['kind'] is being treated as an impossible intersection of strings, and no possible value can be substituted there. We can’t control TS seeing this as RHS or LHS. We can’t even cast something in a useful way that would maintain TS checking things for us.
This kind of change only makes sense if TS has some system where developers can indicate how values are being used, so it can be indicated as LHS or RHS or whatever. Which is a complicated, unpleasant idea, one the TS devs (that I have seen) have shied away from—which I agree with. But without that, this level of strictness is simply restrictive. There’s no good alternative or opt-out option here.

But if we reverse the order of things to avoid property access—<In extends { kind: string }, OutKind extends string, Out extends { kind: OutKind }>—it works. Which is infuriating, because there is no particular reason why this should work better than the other, and—as has happened often with Typescript updates lately—we feel as though we’re just trying permutations of code until we find one that TS likes, with little ability to predict what forms those are going to be, and little understanding of why they stop working and some new permutation is now required.

@simonhaenisch
Copy link
Author

simonhaenisch commented May 31, 2019

@jack-williams thanks for explaining, that makes sense.

The narrowing you apply to key occurs in the RHS expression of the assignment, and is therefore invisible to the assignment itself.

So, the problem is that the expression returns a union type string | number, whereas it should return something like string if key === 'foo' | number if key === 'bar' (or maybe something more like string if key === 'foo' else number), which the assignment could then take into account. Doesn't sound like it's easy to implement though.

BTW is it expected to have breaking changes in minor version changes? Then I need to read the changelog more carefully from now on 🤓

@nmain
Copy link

nmain commented May 31, 2019

BTW is it expected to have breaking changes in minor version changes? Then I need to read the changelog more carefully from now on 🤓

See #14116

@jack-williams
Copy link
Collaborator

jack-williams commented May 31, 2019

Those two signatures are significantly different, and the error in the first is warranted.

The first says that the OutKind is assignable to the property kind in Out, but the converse is not necessary true. For example: take Out = { kind : "a" } and OutKind = never. The resulting array Outkind[] must be the empty array that you can never write to.

The second says that Out is assignable to the type { kind: OutKind }, and implicitly Out['kind'] is assignable to OutKind.

The reason the second works is because the direction of Out and OutKind are reversed, and given that you are writing to something of OutKind, it is necessary that OutKind appears on the right hands side of a constraint. I think TypeScript has got this correct here: the only issue is that this breaks previously ill-typed code.

@jack-williams
Copy link
Collaborator

@simonhaenisch

So, the problem is that the expression returns a union type string | number, whereas it should return something like string if key === 'foo' | number if key === 'bar' (or maybe something more like string if key === 'foo' else number), which the assignment could then take into account. Doesn't sound like it's easy to implement though.

Yes exactly! You would need a dependent type or stronger reasoning with conditional types.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label May 31, 2019
@typescript-bot
Copy link
Collaborator

This issue has been marked 'Working as Intended' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

6 participants