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

type predicate inference: full union match results in loss of infered predicate "v is T" for a simple "boolean" #60741

Open
akomm opened this issue Dec 11, 2024 · 9 comments
Labels
Not a Defect This behavior is one of several equally-correct options

Comments

@akomm
Copy link

akomm commented Dec 11, 2024

πŸ”Ž Search Terms

type predicate type guard inference union full complete match

πŸ•— Version & Regression Information

  • This is the behavior in every version I tried, and I reviewed the FAQ for entries about 5.5.4 though 5.8.0-dev.20241211

⏯ Playground Link

https://www.typescriptlang.org/play/?ts=5.8.0-dev.20241211#code/C4TwDgpgBAYg9nKBeKBvUkBcUDk844A0UAZgtgHYCuAtgEYQBOAvgFAbQBCAho8mh2w4ejIlDq9sAZ2CMAlhQDmbDrAQj+6cBCH4RYsnBHY6CADYRuFNqwD0t8dwAmUALRQIAD0gBjYBCdsADduMyg5KTVEAB8oEQBuKEYIHwg5IICTc0sKVhIqCj85OAoofxkAQQAKELNsfChYkQBKKFsAKigsuAsrKHaHVFYoJIhgKkZS2oA6VSR5qAAifEXG2Jm5hcWRRdYbeyhFBCc8gqKSsogZThrQ+oRGuN5HvV5Wjq6oWvDIhqbngZoYajcaTL6hWbaZBbFZrcFmSGQaEoba8XbMIA

πŸ’» Code

type Foo = {type: 'Foo', foo: number}
type Bar = {type: 'Bar', bar: string}
type FooBar = {type: 'FooBar', fooBar: boolean}

// bad - expected: val is Foo | Bar; received: boolean
function testA(val: Foo | Bar) /* : boolean */ {
  return val.type === "Foo" || val.type === "Bar"
}

// good
function testB(val: Foo | Bar | FooBar) /* : val is Foo | Bar */ {
  return val.type === "Foo" || val.type === "Bar"
}

πŸ™ Actual behavior

The inferred return type of testA is boolean

πŸ™‚ Expected behavior

The inferred return type of testA should be val is Foo | Bar

Additional information about the issue

No response

@jcalz
Copy link
Contributor

jcalz commented Dec 11, 2024

This is working as designed; TS doesn't bother generating type predicates for things that don't narrow their inputs at all: https://github.com/microsoft/TypeScript/pull/57465/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R37508

What exactly is the utility of a type guard function that doesn't narrow? Do you have some code whose behavior is actually improved when function f(x: X) { return true; } is given the return type of x is X instead of boolean?

@RyanCavanaugh RyanCavanaugh added the Not a Defect This behavior is one of several equally-correct options label Dec 11, 2024
@akomm
Copy link
Author

akomm commented Dec 12, 2024

example effect-ts
Here based on the example above that filters only HttpError, an adjusted one that filters all types:
TS Playground

The issue is that the predicate does not have to be always static, it might also be dynamic so just picking "catchAll" is not a solution.
You have code that type checks correctly and then a variable change elsewhere and suddenly it breaks. And its hard to track it to this location.

I think the behavior is not "consistent" / "linear". Its like having a linear function but suddenly the same function gives you Y outside the line for certain X.

In order for @Effect-TS to solve that they would have to assume that the inferred return type of the Refinement being boolean (conditional type) implies that the predicate did not narrow down at all to produce the correct type result. Sounds like a bold assumption they would have to make there. Not sure if a good idea at all.

@jcalz
Copy link
Contributor

jcalz commented Dec 12, 2024

(I'm not a TS team member so feel free to ignore me)

I don't think I can really commit to understanding Effect-TS to look at an example; do you have a self-contained example of such a "dynamic" predicate?

Even if you provide this, I doubt they plan to modify the existing behavior to work for it, since it would cause lots of type predicates to appear in the wild for code that doesn't even attempt to narrow anything, triggering a whole bunch of extra control flow analysis where it doesn't need to happen. Consistency is nice, but it doesn't outweigh compiler performance.

@akomm
Copy link
Author

akomm commented Dec 12, 2024

While its effectively the same, there is a difference in:
function f(x: X) { return true; }

and:
return val.type === "Foo" || val.type === "Bar".

The first one does not provide any expression for TS to evaluate, while the later does.

Correct me if wrong, but for TS to figure out the input is not narrowed down, it has to evaluate it anyway? So what is left then is for it to decide whether to resolve / infer it as a predicate or not based on already computed context.

If your concern is that the performance cost comes later when wild predicates propagate further throughout the code, then maybe the solution could be to behave like it does now, except the type annotation explicitly defines a type predicate?

Here is how the refinement function is typed in effect-ts example:
Image
added catchIf signature using Refinement for completenes
Image

Yet TS decides to drop the context and make it boolean.

@RyanCavanaugh
Copy link
Member

I'd be happy to look at this if there's a repro that is understandable in a reasonable amount of time (i.e. does not depend on effect-ts and is < 20-40 lines), but the repro as given isn't compelling for the reasons jcalz mentioned.

@akomm
Copy link
Author

akomm commented Dec 12, 2024

A simpler Example without effect-ts: TS Playground

@RyanCavanaugh
Copy link
Member

It seems like someone writing that code has a bug, or at least has code suspicious enough to warrant additional verbiage to indicate that they know what they're doing.

@akomm
Copy link
Author

akomm commented Dec 13, 2024

The code is 1:1 reduction of what @Effect-TS (the example) does.

It takes a set A, a subset (B) of A and subtracts the subset B from set A.

A subset can contain all elements of a set, means be equal. Then the difference is an empty set. In TS that is basically never.

The logic by which you omit the predicate makes only sense in a subset of cases. That are those, where you don't inverse, subtract / reduce. In other cases it breaks.

I can open an issue in @Effect-TS if you still think its a bug or mistake.

PS. The nature of simple example is that its simple and will not reflect real life case.

Effect TS uses that set difference to reduce the set of unhandled errors down the pipe line. This means, that the pipeline is potentially defined in one place, while the handling can vary and be passed as an argument to the function that creates the pipeline. Now changing the input predicate would mean, that also the pipeline implementation needs to be changed once all errors are handled. And this pipeline might even be in another library.

How can I give you an example of that in 20-40 lines?

@RyanCavanaugh
Copy link
Member

60 lines? I don't know. Do what you need to do, but I have never seen a good type system change that couldn't be well-motivated by a single screenful of code.

What does Effect-TS lose if it adds the non-predicate overload?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not a Defect This behavior is one of several equally-correct options
Projects
None yet
Development

No branches or pull requests

3 participants