Skip to content

Infer types in unions #51349

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
5 tasks done
jnordberg opened this issue Oct 29, 2022 · 10 comments
Closed
5 tasks done

Infer types in unions #51349

jnordberg opened this issue Oct 29, 2022 · 10 comments

Comments

@jnordberg
Copy link

Suggestion

This should work:

interface A {
    foo: string
}

interface B {
    bar: string
}

type C = A | B

const a = {foo: 'bar'} as C

if (a.foo) {
    console.log(a.foo)
} else {
    console.log(a.bar)
}

I think the compiler should be able to infer the types here. Its a very common pattern to return e.g. (SuccessResponse | ErrorResponse) and without inference it becomes very inconvenient to handle. I apologize if this has been discussed before, it seems like it should have been but I couldn't find anything with a quick search.

Playground

🔍 Search Terms

Infer union types

✅ Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@MartinJohns
Copy link
Contributor

MartinJohns commented Oct 29, 2022

Duplicate of #39065 (comment).

This is very much intentional. The presence of a foo property at runtime does not mean you're dealing with an A. Object types are not sealed.

In your example of SuccessResponse | ErrorResponse it's best practice to have a discriminator property, which makes this an absolute non-problem: just check the discriminator, and TypeScript will narrow accordingly.

@jnordberg
Copy link
Author

What about allowing use of ! to allow the developer to tell the compiler to infer it anyway?

if (a.foo!) {
    console.log(a.foo)
} else {
    console.log(a.bar)
}

Since in the real world you end up doing this which is just as unsafe runtime and very inconvenient:

if ((a as A).foo) {
    console.log((a as A).foo)
} else {
    console.log((a as B).bar)
}

I've run into this recently both in the @aws-sdk and @fastlify libraries and neither have a discriminator property so it is not an uncommon library design pattern you have to work around in TypeScript.

@MartinJohns
Copy link
Contributor

What about allowing use of ! to allow the developer to tell the compiler to infer it anyway?

This is unreasonable. At the point of the operator the type is already known to not have a foo property. Your suggestion would require the compiler to backtrack and change the previously accessed type based on following operator (change whatever a is). I'd expect this to be a performance disaster (even when not using it, because the compiler has to check for it).

Also, this operator already has a meaning: It's the non-null assertion operator. It removes null and undefined from the accessed type.

You already have a working solution: Using a type assertion (either to the specific type, or to any). Or, hopefully, better designed types that won't require such workarounds.

@jnordberg
Copy link
Author

I don't think that is unreasonable. I think the meaning is very similar to what the non-null assertion operator already does, it allows the developer to tell the compiler that they know what the type will be at this point. If it is too hard to implement at the end of the property access the meaning of a!.foo could instead be expanded to include saying that the compiler may "unsafely infer" as well as "unsafely unwrap".

@MartinJohns
Copy link
Contributor

I don't think that is unreasonable. I think the meaning is very similar to what the non-null assertion operator already does, it allows the developer to tell the compiler that they know what the type will be at this point

The compiler checks, in order: a is a valid identifier with this type, accessing foo checks if the type of a has such a property, which it does not, so an error is issued. With your suggestion it would need to continue checking, does this follow an !? Then the compiler needs to go back and change the type of a to any (or have even more logic to determine the possible type). It needs to look beyond the property access to change the type of the value being accessed.

If it is too hard to implement at the end of the property access the meaning of a!.foo could instead be expanded to include saying that the compiler may "unsafely infer" as well as "unsafely unwrap".

Again, this operator already has a meaning. Expanding on this with such an additional unsafe operation is really not desirable for most people. If at all, I could imagine something like a.!foo.

I'm no member of the TypeScript team, I have no say, but I'm 99.99 % certain that this will be rejected. There is already a well working clean solution, and baking in syntax shortcuts for such a corner case is not practical. And this is a corner case, because usually you have a discriminator property.

Anyway, I said my bits, I'm out.

@jnordberg
Copy link
Author

I disagree that this is such a corner case considering I've just run into it twice using two very popular libraries.

But the workaround of if('property name string' in thing) works well enough even though it is ugly and hard to discover (and just as unsafe ¯\(ツ)/¯) but I think you're right in your assessment that this situation will not be improved considering all the other issues you linked. Thanks for taking your time to discuss.

@fatcerberus
Copy link

@jnordberg in-based type narrowing is indeed unsound and that was a deliberate compromise, but see #49933 (comment) - the truthiness check has additional safety concerns that the in check does not. In fact the example code in the OP hits the very footgun mentioned in that comment - the empty string "" is falsy.

interface A {
    foo: string
}

interface B {
    bar: string
}

type C = A | B

const a = {foo: ""} as C

if (a.foo) {
    console.log(a.foo)
} else {
    console.log(a.bar)  // oops! not actually a B!
}

@jnordberg
Copy link
Author

@fatcerberus True, although it's more of a JavaScript footgun than a TypeScript one IMO.

It would have made more sense to me if it inferred correctly when using if (typeof a.foo == 'string'), that's what I intuitively tried to do when it didn't do it with a simple if (a.foo). That would also not have the problem of a.foo existing but assigned as undefined or similar like could happen with an in check, I suspect more devs would get bitten by something like that than a empty string being coerced to false which is a widely known JS "feature".

@fatcerberus
Copy link

fatcerberus commented Oct 31, 2022

True, although it's more of a JavaScript footgun than a TypeScript one IMO.

FWIW, it’s actually an implicit design goal of the type system to save you from JS footguns and misfeatures, since you’re ultimately just writing JS-with-type-annotations. For instance, you can’t pass anything but a string to parseInt(), even though the spec fully defines coercion behavior, because the spec’d behavior is unlikely to align with expectations - see issues like e.g. #50828 (comment).

@fatcerberus
Copy link

btw the reason typeof a.foo === 'string' doesn’t work is because in general, unions of objects cannot currently be narrowed based on the types of their properties, with the exception of discriminated unions. See #42384 for that.

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

No branches or pull requests

3 participants