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

Probable bug: Functional properties don't get inferred with circular type parameter constraints like T extends M<T> #40439

Open
devanshj opened this issue Sep 8, 2020 · 14 comments · May be fixed by #51511
Labels
Needs Investigation This issue needs a team member to investigate its status.
Milestone

Comments

@devanshj
Copy link

devanshj commented Sep 8, 2020

TypeScript Version:
4.0.2

Search Terms:
Circular type parameter constraint, functional property inference, function parameter inference, function argument inference

Code:

declare const m: <T extends M<T>>(m: T) => T
type M<Self, K = Exclude<keyof Self, "k" | "t">> =
  { a?: number
  , b?: number
  , c?: number
  , d?: number
  , k?: K
  , t?: (k: K) => void
  }

// :)
// Case 1
m({
  a: 1,
  b: 2,
  k: "a"
})

// :)
// Case 2
m({
  a: 1,
  b: "x", // expected error
  k: "c" // expected error
})

// :(
// Case 3
m({ // type parameter becomes `unknown` (probably because t's first parameter is `any` initially kinda?)
  a: 1,
  b: 2,
  k: "a", 
  t: k => {} // k is inferred as `never` (instead of `"a" | "b"`)
})

// :|
// Case 4
m({
  a: 1,
  b: 2,
  k: "a", 
  t: (k: "a" | "b") => {} // have to explicitly type `k` which could have been inferred :(
})

Expected behavior:
The parameter k of t should be inferred as "a" | "b". As property k already gets inferred in Case 1 & 2, so why not the k parameter of t

Actual behavior:
The parameter of t is inferred as never and T in inferred as unknown. I assume first T gets resolved to unknown as t is initially inferred as (k: any) => void which does not satisfy the constraint of m.

Playground

@devanshj devanshj changed the title Functional properties don't get inferred with circular type parameter constraints like T extends M<T> Probable bug: Functional properties don't get inferred with circular type parameter constraints like T extends M<T> Sep 8, 2020
@devanshj
Copy link
Author

devanshj commented Sep 9, 2020

@jcalz if you could help in anyway it would be great! 🙏🏻

@devanshj
Copy link
Author

devanshj commented Sep 9, 2020

Workaround:

- declare const m: <T extends M<T>>(m: T) => T
+ declare const m: <T extends M<T>>(m: Identity<T>) => T

+  type Identity<T> =
+    {  [K in keyof T]:
+          IsPlainObject<T[K]> extends true
+            ? Identity<T[K]>
+            : T[K]
+    }
+  
+  type IsPlainObject<T> =
+    T extends object
+      ? T extends (...a: any[]) => any
+        ? false
+        : true
+      : false

* tears of joy *

I'll keep this issue open to know if this is the "official" way to deal with it or not. I mean it kinda makes sense to me why does it work but still.

@devanshj
Copy link
Author

FWIW the #44700 variation makes this seem actually a bug

@RyanCavanaugh
Copy link
Member

Pasting in that repro for reference

declare const m: <T extends M<T>>(m: T) => T

type M<Self, V = Prop<Self, "v">> =
  { v: V
  , t?: (v: V) => void
  }

type Prop<T, K> = K extends keyof T ? T[K] : "error"

// expected : M<{ v: number, t?: (v: number) => void }>
// actual   : M<unknown>
m({
  v: 1,
  t: k => {}
})

// expected : M<{ v: number }>
// actual   : M<{ v: number }>
m({
  v: 1
})

@RyanCavanaugh
Copy link
Member

What you have here is equivalent to the much simpler, and also working,

declare const m: <T>(m: M<T>) => T

type M<T> = {
  v: T
  t?: (v: T) => void
}

// expected : M<{ v: number, t?: (v: number) => void }>
// actual   : M<unknown>
m({
  v: 1,
  t: k => {}
})

// expected : M<{ v: number }>
// actual   : M<{ v: number }>
m({
  v: 1
})

If I had to wager about the confusion, it's that the default for V is not an existential, but rather an actual default that gets instantiated in-place when not specified.

If you have a description of m that can't be typed straightforwardly, I'd be happy to take another look

@devanshj
Copy link
Author

devanshj commented Jun 22, 2021

First off I was utterly confused because in the code snippet you wrote // actual : M<unknown> instead of // actual : M<{ v: number, t?: (v: number) => void }> xD It's only when I read "and also working" and tried it in the playground, I realized it indeed works.

If you have a description of m that can't be typed straightforwardly, I'd be happy to take another look

First to give you some context - The thing is I'm using these T extends M<T> style types (I call them "Self Reconstructing" types) to do a variety of cool things (like show custom errors) and mainly strongly type xstate in my project called txstate (which will hopefully be merged with xstate itself). Some of these style of types have already landed xstate like here.

Though to give you a concrete problem and as minimal as I offer right now (while I think of a more minimal problem), you can refer this PR (to another xstate-like library called useStateMachine) and precisely this test case. To make it work you'd have to add a phantom property _: null beside the effect property like so...

useStateMachine({
  initial: "a",
  states: {
    a: {
      _: null, // Remove this and the inference stops working
      effect: parameter => {}
    }
  }
})

This same is the problem with xstate too. So it'd be really cool if you can take another look and see if TypeScript can do a better job of inference. Thanks a lot for your time! :)

TLDR: Open this test case. Remove @ts-ignore. Verify if you can see errors. Add a phantom _: null as a sibling property to effect (like shown above). Verify if the error is gone. State machine libraries like xstate and useStateMachine can be only strongly typed with this "non straightforward" way.

cc @davidkpiano @Andarist @cassiozen

@RyanCavanaugh
Copy link
Member

Sorry for leaving in the bad comments, good reminder for me for next time 😅

@devanshj
Copy link
Author

devanshj commented Jun 22, 2021

Still finding a minimal real world repro for the variation where you need a phantom "_", though I have one for the original problem...

Playground

useStateMachine({
  initial: "a",
  context: { foo: 1 },
  states: {
    a: {
      on: {
        X: "b"
      }
    },
    b: {
      effect: p => {
        p.context.foo === 2 
        // @ts-expect-error (expected error for testing)
        p.context.foo === "a" 
        
        p.event.type === "X"
        // @ts-expect-error (expected error for testing)
        p.event.type === "Y"
      }
    }
  }
})

declare const useStateMachine: <D extends Machine<D>>(definition: InferNarrowestObject<D>) => void

type Machine<Self> = 
  { initial: keyof Get<Self, "states">
  , context: Get<Self, "context">
  , states: 
    { [S in keyof Get<Self, "states">]:
        StateNode<Self, Get<Self, ["states", S]>>
    }
  }

type StateNode<Machine, Self, On = Get<Self, "on">> =
  { on?:
    { [E in keyof On]: MachineStateValue<Machine>
    }
  , effect?:
      (parameter:
        { context: MachineContext<Machine>
        , event: MachineEvent<Machine>
        }
      ) => void
  }

type MachineContext<Machine> = Get<Machine, "context">
type MachineStateValue<Machine> = keyof Get<Machine, "states">
type MachineEvent<Machine> =
  { type:
      { [S in keyof Get<Machine, "states">]:
          keyof Get<Machine, ["states", S, "on"]>
      }[keyof Get<Machine, "states">]
  }

type Get<T, P, F = undefined> =
  P extends keyof T
    ? T[P] :
  P extends [] ?
    T extends undefined ? F : T :
  P extends [infer K1, ...infer Kr] ?
    K1 extends keyof T ?
      Get<T[K1], Kr, F> :
    F :
  never

type InferNarrowest<T> =
  T extends any
    ? ( T extends (...a: any[]) => any ? T :
        T extends object ? InferNarrowestObject<T> :
        T
      )
    : never

type InferNarrowestObject<T> =
  { readonly [K in keyof T]: InferNarrowest<T[K]> }

Notice we need InferNarrowestObject<D> workaround (same as the Identity<T> workaround that I mention in a comment above) to make it work. There isn't a straightforward to strongly type this afaict (if one is even successful these libraries have sophisticated requirements than this snippet)

This is the workaround to be clear

- declare const useStateMachine: <D extends Machine<D>>(definition: D) => void
+ declare const useStateMachine: <D extends Machine<D>>(definition: InferNarrowestObject<D>) => void

Also to tell you what I mean by "custom errors" here's a hypothetical example. The requirement is that if the node to which initial points to is final, then such value for initial shouldn't be allowed and an error should be shown why so.
There's a hell lot of things I'm doing with these types that I can keep going, for example for xstate enforcing ids are unique throughout the whole tree.

@devanshj
Copy link
Author

devanshj commented Jul 6, 2021

Repro for the phantom "_" bug. Playground. I know you can type this in a straight forward way but that's besides the point for the above reasons. It's only the case here because this is a minimal repro, in reality the requirements are much more.

// Initial issue
createMachine1({
  initial: "a",
  context: { foo: 1 },
  states: {
    a: {
      entry: p => { // p is any :(
      }
    }
  }
})

// with InferNarrowestObject workaround
createMachine2({
  initial: "a",
  context: { foo: 1 },
  states: {
    a: {
      entry: p => { // p is { foo: number }
      },
      _: null, // tho we need this phantom property...
    }
  }
})

// ...doesn't work without "_"
createMachine2({
  initial: "a",
  context: { foo: 1 },
  states: {
    a: {
      entry: p => {
      }
    }
  }
})

declare const createMachine1: <D extends Machine<D>>(definition: D) => void
declare const createMachine2: <D extends Machine<D>>(definition: InferNarrowestObject<D>) => void
type Machine<Self> = 
  { initial: keyof Prop<Self, "states">
  , context: Prop<Self, "context">
  , states:
    { [S in keyof Prop<Self, "states">]:
        { entry?: (context: Prop<Self, "context">) => void
        , _?: null
        }
    }
  }

type InferNarrowest<T> =
  T extends any
    ? ( T extends (...a: any[]) => any ? T :
        T extends object ? InferNarrowestObject<T> :
        T
      )
    : never

type InferNarrowestObject<T> =
  { readonly [K in keyof T]: InferNarrowest<T[K]> }

type Prop<T, P> =
  P extends keyof T ? T[P] : never

@devanshj
Copy link
Author

devanshj commented Jul 19, 2021

Here's something somwhat real-world that can't be compiled in a "straight forward way". Playground. This is as straight forward as I can manage.

// compiles
createMachine({
  initial: "a",
  states: {
    a: {},
    b: {}
  }
})

// add entry and it doesn't compile
createMachine({
  initial: "a",
  states: {
    a: {
      entry: k => {} // because k doesn't get inferred
    },
    b: {}
  }
})

// annotate entry that could have been inferred and it compiles
createMachine({
  initial: "a",
  states: {
    a: {
      entry: (k: "a") => {}
    },
    b: {}
  }
})

declare const createMachine:
  < Definition extends
      { initial: keyof Definition["states"]
      , states:
          { [K in keyof Definition["states"]]:
              { entry?: (k: K) => void
              }
          }
      }
  >
    (definition: Definition) =>
      { definition: Definition
      }

@devanshj
Copy link
Author

devanshj commented Jul 26, 2021

@RyanCavanaugh what are your thoughts on this issue? Adding a functional property toppling the inference is fishy if not buggy. Especially when parameters are explicitly typed as what were expected to be inferred, it compiles.

And how likely/unlikely is that future TypeScript versions would break this workaround or in other words how reliable is it?

I'm asking especially because I'll be using this workaround in @cassiozen/usestatemachine1 so want to let users know how risky it is or is not. And I'll also provide a workaround-free degraded version of types so vigilant users could use that.

1 which is decently popular to say the least and with my types it's literally going to be the best state machine library out there in terms of types :P so I'm expecting a lot of people are going to use it

@RyanCavanaugh
Copy link
Member

@devanshj unless I'm missing an existing trick we could re-apply here, the example above is squarely in the realm of "need a unification-based algorithm". The difficulty here lies in the self-same mapping between the keys and callback values of states.

This definition is possibly better depending on your use cases?

type SelfStates<Keys extends string> = {[K in Keys]: { entry?: (k: K) => void } }
type Definition<Initial, States> = { initial: Initial, states: States };
declare function createMachine2<StateNames extends keyof States, States extends SelfStates<StateNames & string>>(def: Definition<StateNames, States>): Definition<StateNames, States>;

@devanshj
Copy link
Author

devanshj commented Aug 16, 2021

unless I'm missing an existing trick we could re-apply here, the example above is squarely in the realm of "need a unification-based algorithm". The difficulty here lies in the self-same mapping between the keys and callback values of states

I'm a total novice but I think that's right. But more precisely I think what is happening is (perhaps that's what you meant) is that K is both what is meant to be "passed to the createMachine" AND what is meant to "come from createMachine" via the callback parameter, so because of the heuristic Anders mentioned here the compiler decides to give callback K the priority and resolves it to any -- same thing happens in every variant of this issue. So it being a key is not super relevant as long as it's something that both "comes" and "goes" to the parent generic function, hence even though keys are not involved here the issue still remains.

So I think providing a LowInfer which I suggested here would be simpler than a unification algorithm, though I'm not sure if it'll solve the issue.

This definition is possibly better depending on your use cases?

It does seem to behave better (I think I did try this) beside the fact if you have more than one states it doesn't work but can be solved if you assert initial to be union of states (I did try NoInfer<Initial> but apparently doesn't work).

But ultimately it still doesn't cover my use-case of having the access to the whole tree, it turns out the my last repro that you just solved is too minimal. Let me try to increase it's requirement to the original a tad bit...

// compiles
createMachine({
  initial: "a",
  states: {
    a: {
      onNext: "#someId"
    },
    b: {
      id: "someId" as const
    }
  }
})

// add entry and it doesn't compile
createMachine({
  initial: "a",
  states: {
    a: {
      onNext: "#someId",
      entry: k => {} // because k doesn't get inferred
    },
    b: {
      id: "someId" as const
    }
  }
})

// annotate entry that could have been inferred and it compiles
createMachine({
  initial: "a",
  states: {
    a: {
      onNext: "#someId",
      entry: (k: "a") => {}
    },
    b: {
      id: "someId" as const
    }
  }
})

declare const createMachine:
  < Definition extends
      { initial: keyof Definition["states"]
      , states:
          { [K in keyof Definition["states"]]:
              { id?: string
              , onNext?: 
                  | keyof Definition["states"]
                  | { [S in keyof Definition["states"]]:
                        Definition["states"][S] extends { id: infer Id } ? `#${Id & string}` : never
                    }[keyof Definition["states"]]
              , entry?: (k: K) => void
              }
          }
      }
  >
    (definition: Definition) =>
      { definition: Definition
      }

I can take this a step further and not require the user to write as const but let me keep it as minimal as possible :P

@Andarist
Copy link
Contributor

This suffers from a similar problem as #48798 . It is somewhat different though because this one here doesn't contain any reverse mapped types.

m({ // type parameter becomes `unknown`
  a: 1,
  b: 2,
  k: "a", 
  t: k => {} // k is inferred as `never` (instead of `"a" | "b"`)
})
  1. this arrow function is "context-sensitive" because it contains unannotated params
  2. so checkFunctionExpressionOrObjectLiteralMethod returns anyFunctionType for it and that has ObjectFlags.NonInferrableType on it
  3. this flag is one of the PropagatingFlags so the parent object (the argument itself) becomes non-inferrable
  4. the signature gets inferred as const m: <M<unknown, never>>(m: M<unknown, never>) => M<unknown, never> because of that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants