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

Generic index access on target type and return based on discriminated types do not work. #32698

Closed
archfz opened this issue Aug 3, 2019 · 22 comments
Labels
Duplicate An existing issue was already created

Comments

@archfz
Copy link

archfz commented Aug 3, 2019

TypeScript Version: ^3.5.0

Search Terms: index access target type discriminated types

Code

interface TypeA {a: string; }
interface TypeB {b: string; }

class A implements TypeA {public a: string = ""; }
class B implements TypeB {public b: string = ""; }

enum Types {
  A= "a", B = "b",
}

interface TypeInterfaces {
  [Types.A]: TypeA;
  [Types.B]: TypeB;
}

function create<T extends Types>(type: T): TypeInterfaces[T] {
  if (type === Types.A) {
    return new A;
  } else if (type === Types.B) {
    return new B;
  }

  throw new Error();
}

Expected behavior:
The compiler knows with discriminated types exactly what it needs to return and there are no errors as in 3.4 or lower.

Actual behavior:
The compiler gives an error that we cannot assign the returned A or B to TypeA & TypeB regardless of the fact that we actually know for sure with discriminated types that we need to return either TypeA or TypeB.

Playground Link:
Try it on playground

Related Issues:
This issue was specifically introduced here for v3.5 #30769 by @ahejlsberg when it was decided to have the target type on index types as intersections.

@jcalz
Copy link
Contributor

jcalz commented Aug 4, 2019

Given that #30769 isn't going anywhere, this looks like a duplicate of #13995 (which has been made harder to deal with since #30769 landed). Possible fixes or paths to fixes: #25879, #27808.

@archfz
Copy link
Author

archfz commented Aug 4, 2019

@jcalz thanks for the fast answer. So I dont quite understand why index access results target type should be an intersection, but I guess your suggesting that this issue is actually with union narrowing. I taught those were working with discriminated types.

Either way I looked into the proposals, the one with oneof seems nice. But do you think there is a workaround for this atm? Otherwise I will revert typescript.

@jack-williams
Copy link
Collaborator

jack-williams commented Aug 4, 2019

I don't think #13995 is a duplicate for the reason that a fix for that would be unlikely to fix this. Those examples really focus on narrowing the type of a value that happens to be of generic type; this example requires the arbitrary narrowing of a type variable in scope, specifically the return type of the function.

The 'one of' solution is IMO the wrong fix for this. The problem is that prior to #30769 it was possible to say "this value should be assignable to the type you getting accessing object T with key K" and did not assume that a write to an object of type T was happening. The current behaviour assumes that an index access type appearing on the RHS of a relation implies a write, which is not true in general. In my opinion the correct fix would be to review when the read and write types of an index access are used.

For a short term fix you will probably need a cast, or a modification to how you use create.

function create<T extends Types>(type: T) {
  return constructors[type];
}

const mkA = create(Types.A);
const a: A = new mkA();

@archfz
Copy link
Author

archfz commented Aug 4, 2019

@jack-williams Well the workaround provided actually would defeat the purpose, purpose being a factory: the one that instantiates.

@jack-williams
Copy link
Collaborator

jack-williams commented Aug 4, 2019

This approach will not scale once the constructors need arguments that must be supplied to create.

class Factory {
  get [Types.A]() {
    return new A();
  }

  get [Types.B]() {
    return new B();
  }
}

const f = new Factory();

function create<T extends Types>(key: T) {
  return f[key];
}

const a: A = create(Types.A);

@archfz
Copy link
Author

archfz commented Aug 4, 2019

I have actually realized that it was flawed in version prior to 3.5 since you could have returned in any case any of the types, which actually means that it was not properly type guarded. So 3.5 improves upon this, but doesn't fix it yet.

@fatcerberus
Copy link

fatcerberus commented Aug 4, 2019

@jack-williams Just to clear up confusion:

The current behaviour assumes that an index access type appearing on the RHS of a relation implies a write

Did you actually mean LHS here? In an assignment x = y, the LHS is the side being written to and the RHS is the side being read from. Or did you mean something else here?

@archfz Yes, prior to 3.5 and #30769, this was possible:

interface Foo {
    a: string;
    b: number;
}
declare const x: Foo;
declare const y: Foo;
declare const xk: keyof Foo;  // = 'a'
declare const yk: keyof Foo;  // = 'b'
x[xk] = y[yk];

Because you don't know which property x[xk] will write to (note that the compiler is only looking at the types here), the only thing you can provide that guarantees the assignment is typesafe is string & number--an intersection of the types selected by the key type. Unfortunately, while perfectly typesafe, this very often ends up being restrictive in practice because there's not always a nice way to narrow the key (e.g. looping over keys, or generics). 🙁

@archfz
Copy link
Author

archfz commented Aug 4, 2019

@fatcerberus I don't quite understand how the example provided is relevant as it's missing the narrowing based on discriminated types.

@jcalz
Copy link
Contributor

jcalz commented Aug 4, 2019

@jack-williams I interpret #13995 as asking generic type parameters themselves to be narrowed via control flow analysis, not merely that the particular value checked be narrowed. "I am just using extends to mean AB'A' | 'B'"... the proper subset symbol implies that the desired behavior is that the generic type AB can be resolved either with the concrete type A or the concrete type B but cannot be resolved with the concrete type A | B. The example code is a bit ambiguous, but subsequent comments only make sense if people want the actual type parameter to be narrowed and not just the values.

@archfz The workarounds here are the same as the workarounds in #13995, I think. The easiest one is to use type assertions:

function create<T extends Types>(type: T): TypeInterfaces[T] {
  if (type === Types.A) {
    return new A as TypeInterfaces[T]; // assertion
  } else if (type === Types.B) {
    return new B as TypeInterfaces[T]; // assertion
  }
  throw new Error();
}

or a single-call-signature overload, which is morally equivalent to a bunch of type assertions on the parameters and return values:

function create<T extends Types>(type: T): TypeInterfaces[T]; // call signature
function create(type: Types): TypeInterfaces[Types] { // impl signature
  if (type === Types.A) {
    return new A;
  } else if (type === Types.B) {
    return new B;
  }
  throw new Error();
}

@fatcerberus
Copy link

@archfz Right, that's why I said it gets more complicated with generics - you can't narrow a type parameter. I was merely explaining where the intersection was originating from.

@jack-williams
Copy link
Collaborator

@fatcerberus No I mean RHS. An expression x = y is legal if the type of y is related to the type of x, so here the 'relation' is the assignability relation and the thing being written to appears on the right. It's a good distinction to make, so thanks for the pointer.

@jcalz I think the fact that there is ambiguity over the interpretation of what the issue actually requires suggests, to me at least, that this should not just be subsumed. Many examples in that issue, including the original, could be fixed by narrowing a generic value to T & A. The code here only works in the most liberal case because the narrowed value doesn't even appear in the body of conditional.

@jcalz
Copy link
Contributor

jcalz commented Aug 4, 2019

@jack-williams I've certainly been wrong before, I will likely be wrong again soon, and I may be wrong right now... but I sincerely cannot see how @RyanCavanaugh's comment:

TL;DR from design discussion: It's somewhat obvious what the "right" thing to do is, but would require a large rework of how we treat type parameters, with concordant negative perf impacts, without a corresponding large positive impact on the actual user-facing behavior side.

makes any sense at all if the "right" thing he's talking about is just to intersect the generic type of the checked value with the guarded type, as you get with user-defined type guards like this:

const isStringLiteral = <L extends string>(l: L, v: string): v is L => l === v;

declare function takeA(val: "A"): void;
export function bounceAndTakeIfA<AB extends "A" | "B">(value: AB): AB {
  if (isStringLiteral("A", value)) {
    // value is now AB & "A"
    takeA(value); // okay
    return value;
  } else {
    return value;
  }
}

But since you read the same issue I did and have come to a different conclusion, I guess it's not settled. Hopefully someone will come and speak authoritatively about this and put the matter to rest. Cheers!

@jack-williams
Copy link
Collaborator

I do not expect that the solution would simply be intersecting with the constraint; rather, the solution would likely be narrowing the base constraint of the type parameter. This approach has been tried before and closed, in part, because composing narrowed variables at join points produces awkward unions as discussed here.

There is a large spectrum of solutions: narrowing the constraint of an identifier is at one end, abitrarily narrowing a type variable in scope is at the other. I don't think the scope of #13995 is very well defined.

@fatcerberus
Copy link

@jack-williams Thanks, I thought you meant the RHS of the assignment statement and got very confused. You’re indeed correct that “can type Y act in place of type X” is not necessarily the same question as “can we write a value of type Y to type X”, though there is significant overlap.

I suspect the only typesafe narrowing that could be done with a generic type parameter, is if there were a way internally to represent a lower bound. Assuming x :: T then if typeof x === "string" that means we should be able to conclude that T >: string. But that may be tricky too because it’s always possible for the caller to provide a type parameter manually. So yeah, generics make this situation quite hairy indeed.

@archfz
Copy link
Author

archfz commented Aug 4, 2019

I am not an expert on typeing, but to me seems to be two issues here with the example I provided in the summary:

  1. It makes to me no sense logically that the caller will expect always a single type to be returned but the callee is expected to return the intersection of all possible return types. This is just wrong.
  2. While for the case of narrowing actually doesn't really matter if the return type is union or intersection of all possible types, since here the problem is that it's just not narrowed, because if it would be narrowed it would always be one specific type regardless whether the whole is a union or intersection.

@fatcerberus
Copy link

It makes to me no sense logically that the caller will expect always a single type to be returned but the callee is expected to return the intersection of all possible return types. This is just wrong.

This is because inside the function, the compiler doesn’t know how wide T will be, therefore has to fall back on its constraint and thus TypeInterfaces[T] will necessarily select all applicable keys. Your problem then arises because you can easily narrow values of type T, but can’t—in the general case—narrow T itself. It’s simply not safe to do so.

At the call site, the exact type of T is known (because it’s been provided explicitly as part of the call) and TypeInterfaces[T] selects exactly what you ask for, so there’s no issue there.

@archfz
Copy link
Author

archfz commented Aug 4, 2019

@fatcerberus But in case T is discriminated union doesn't that mean you can only have exactly one type of T at the end, and that this should be known inside the function? Anyway it seems to me more logical to have a union return type in this case rather than an intersection.

@jack-williams
Copy link
Collaborator

@fatcerberus Even a lower bound would not be sufficient I think. A truthy type test of typeof x === "string" does not rule out T = "some string literal", so a lower bound of string would be incorrect. I don't think there is anyway to get the right behaviour with bounding, and you really need a new predicate like: #25879, #27808, #28430.

@fatcerberus
Copy link

TS doesn’t have any kind of exhaustiveness checking AFAIK so if it were a union instead of an intersection, nothing would stop you from returning the wrong thing in an else-clause, for example. Prior to TS 3.5 (when it was a union) you could just replace your entire function with return new A; and it would typecheck fine (try it in the playground!). And then be wrong at the call site that asked for a B.

Trust me—to make this work you need some way to narrow T, which isn’t currently possible. 😢

@archfz
Copy link
Author

archfz commented Aug 4, 2019

Anyway it seems to me more logical to have a union return type in this case rather than an intersection.

Actually I am wrong here. None of them really make sense, but actually intersection provides more type safety in this case, most likely in others as well, because union would allow any of the types to be returned while there is only one specific awaited.

@archfz
Copy link
Author

archfz commented Aug 4, 2019

I am vouching for extends oneof, that seems simple and concise. So I guess this issue can be marked as duplicate. The return type being intersection is clear.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Aug 5, 2019
@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' 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
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

6 participants