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

Inconsistent Type Inference of Object Property In Union Type #48500

Closed
sullivan-sean opened this issue Mar 31, 2022 · 8 comments
Closed

Inconsistent Type Inference of Object Property In Union Type #48500

sullivan-sean opened this issue Mar 31, 2022 · 8 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@sullivan-sean
Copy link

sullivan-sean commented Mar 31, 2022

Bug Report

🔎 Search Terms

parameter 'x' implicitly has type 'any'
type inference
discriminated union
discriminant property
type narrowing of parent object

🕗 Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ

⏯ Playground Link

Playground link with relevant code

💻 Code

type Closure<T> = { arg: T, fn: (_: T) => void };

const a: Closure<`x${string}`> | Closure<number> = {
  arg: 0,
  // x is inferred as number, because of arg above
  fn: (x) => {}
}

const b: Closure<string> | Closure<number> = {
  arg: 0,
  // x cannot be inferred (parameter 'x' implicitly has type 'any')
  fn: (x) => {}
}

🙁 Actual behavior

Inference of the parameter to the function field fn is inconsistent across the two objects. The only difference is the type T in Closure<T> | Closure<number> for the object type:

  • When T is boolean, "a" | "b", null or even `x${string}`, the type of x as number is correctly inferred when arg is set to 0.
  • When T is string, never, number[], inference of x as type number is not possible.

See TS playground link for more examples of which types inference is possible for.


There is no immediately obvious pattern as to what types result in possible inference and what types preclude it.

I'm no expert on how this inference is accomplished, but if it is at all similar to type narrowing then this comment #30506 (comment) is the closest explanation I can find. Maybe type inference works when T is boolean ~ true | false and "a" | "b" because arg is then discriminant property. Even in this case, it would seem a field of type `x${string}` would be a discriminant property(?) and I can't wrap my head around why a field of type string could not be a discriminant property if one of type `x${string}` can be

🙂 Expected behavior

Type inference of the parameter x to property fn is consistent across types T in Closure<T> | Closure<number> when arg is set to 0 and 0 is not an instantiation of T

OR

If type inference is not meant to be consistent due to a design limitation (e.g. as in #33205 (comment)), this behavior difference and any associated design limitation is better/more clearly documented -- I'd love to see something on discriminant properties in official documentation, as I found the Github threads a little hard to follow

@RyanCavanaugh
Copy link
Member

The current rule is (consistently, I guess?) that a potential discriminant property has to have at least one literal type in it to be a discriminant. boolean counts as a literal type since it's a union of the literal types true and false. The rule used to be less useful than that (in 4.4, none of these examples "work" at all) but was then made to be slightly more useful to include string literal types.

Using non-literals as discriminants is very problematic because once there are multiple discriminants in play (which would be quite common in practice if this weren't the rule), TS would be arbitrarily selecting which properties to complain about in error messages, and you'd end up with examples like

type Message = { kind: "text"; payload: string } | { kind: "money"; payload: number };
declare function sendMessage(m: Message): void
sendMessage(kind: "text", payload: 0);

where the problem is "clearly" that payload is wrong, but it'd be effectively unpredictable which property got picked to be the discriminant since now they're on equal footing.

@sullivan-sean
Copy link
Author

sullivan-sean commented Mar 31, 2022

I still don't quite understand why a field of type `x${string}` is a discriminant property. Is it because the string literal "x" is a member of `x${string}`?

Can't there be multiple discriminants in play already, e.g. in

type Message = { kind: "text"; payload: "a" } | { kind: "money"; payload: "b" };
declare function sendMessage(m: Message): void
sendMessage({ kind: "text", payload: "b" });
sendMessage({ kind: "money", payload: "a" });

lines 3 and 4 complain about different properties (Playground Link). Is the problem that this would just be more prevalent if it were with any property?

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 31, 2022

Whether or not a property is a potential discriminant is about the target type, not the source type.

The rule for providing the contextual type on the function is basically:

  • Check the target type for a discriminant property (technically one or more discriminant properties)
  • If you find one, see if the source type has a matching property
  • If you find that, filter the target type based on the matching constituent of the target union, and if this produces exactly one result, use that to contextually type the source expression

The bug here, if there is one, is that we really shouldn't consider template string types with holes in them to be discriminants since, unlike other types of literals, the same value can inhabit multiple literal types at the same time:

type Closure<T, U> = { arg: T, fn: (_: U) => void };

// Value actually fulfills both branches
const m: `x${string}` = "xz";
const a: Closure<`x${string}`, string> | Closure<`${string}z`, number> = {
  arg: m,
  fn: (x) => {
    // x is string here but we can't correctly disavow number
  }
};

but I don't presume that you want this issue "fixed" by adding more implicit anys rather than fewer

@RyanCavanaugh
Copy link
Member

This is all sort of complicated, though, since you definitely can write string literal types that are visibly disjoint (e.g. a-${string} and b-${string}, and people will definitely expect those to work, but I'm not sure of the existence of a way to easily prove if two arbitrary things you could write like that are knowably overlappy.

@sullivan-sean
Copy link
Author

sullivan-sean commented Mar 31, 2022

Agreed, an ideal solution (for this use case at least) would be fewer explicit anys. And it doesn't make sense to limit disjoint string literal types.

Your explanation of how discriminant properties work was very helpful in understanding how it is consistent that x${string} differs from string.

I think I'm still a little unsure of how/why this is problematic:

Using non-literals as discriminants is very problematic because once there are multiple discriminants in play (which would be quite common in practice if this weren't the rule), TS would be arbitrarily selecting which properties to complain about in error messages, and you'd end up with examples like

given that it seems like this happens (to some degree) already.

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Mar 31, 2022
@RyanCavanaugh
Copy link
Member

If someone has multiple discriminants and complains that an error message isn't the one they wanted, we can legitimately tell them that it's just sort of Too Bad since any error message in that situation is "equally correct" (this has happened).

There are performance implications if any co-present property can be a discriminant, since we will go and try to do all the relational work to make the correct contextual type get selected. Literal checking is very cheap, but if we have to relate arbitrary types to arbitrary types just to find a discriminant that will very often end up not doing anything, it's going to be a big performance problem.

The current rule is consistent and useful; I don't think there's a net improvement to be found here.

@sullivan-sean
Copy link
Author

Thanks, this has cleared up a lot of confusion around discriminant properties for me. I'd love to see more in the official documentation describing them -- I think having a better understanding of discriminant properties makes the notion of consistency here much more apparent 😄


I found this particularly helpful:

The rule for providing the contextual type on the function is basically:

  • Check the target type for a discriminant property (technically one or more discriminant properties)
  • If you find one, see if the source type has a matching property
  • If you find that, filter the target type based on the matching constituent of the target union, and if this produces exactly one result, use that to contextually type the source expression

It is now much easier for me to explain why the following behavior occurs:

type Closure<T> = { arg: T, fn: (_: T) => void };


const a: Closure<null> | Closure<string> | Closure<number> = {
  arg: 0,
  // x can be inferred as number because arg has target type `null | string | number` and source type `number`
  // the target type has a literal `null` and is therefore discriminant
  fn: (x) => {}
}

const b: Closure<string> | Closure<number> = {
  arg: 0,
  // x cannot be inferred as number because arg has target type `string | number` and source type `number`
  // the target type has no literal and is therefore *not* discriminant
  fn: (x) => {}
}

I think it had originally just seemed counterintuitive that the correct contextual type can be selected by comparing the number source type with target type null | string | number, but not with target type string | number. This is unintuitive because the former seems more difficult/less performant to narrow since we are inferring from a larger union of types.

From your last comment I think that actually, it IS more difficult/less performant to select the contextual type in the null | string | number case than the string | number case as intuition suggests BUT if we were to turn on discriminant properties for all properties, we would just end up doing WAY more of these computations on every union of objects with properties and that would be prohibitively expensive.

@fatcerberus
Copy link

fatcerberus commented Apr 4, 2022

From your last comment I think that actually, it IS more difficult/less performant to select the contextual type in the null | string | number case than the string | number case as intuition suggests BUT if we were to turn on discriminant properties for all properties, we would just end up doing WAY more of these computations on every union of objects with properties and that would be prohibitively expensive.

Yep, and if some of those discriminants are object types themselves, things get even more hairy, since then the checker would have to recurse into nested objects too. The performance picture would get ugly fast. There are often requests to have discriminated unions work when the discriminant property is nested 2 or 3 levels deep - this is why that doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

3 participants