microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.87k stars 12.47k forks source link

Incorrect inference when using an optional argument #57021

Closed ShayDavidson closed 9 months ago

ShayDavidson commented 9 months ago

🔎 Search Terms

Inference, Return, Arguments, Parentheses, Identity Function.

🕗 Version & Regression Information

This reproduces in all available playground versions (tried down to 3.3.3), and also in 5.3.3.

⏯ Playground Link

Link

💻 Code

This is a followup on https://github.com/microsoft/TypeScript/issues/49951 which @Andarist asked me to open a follow-up issue. Also relates to https://github.com/microsoft/TypeScript/issues/47599

Probably better to check the playground for code examples, but here's the TL;DR:

TYPES DECLERATIONS:

type Schema = Record<string, unknown> // modified from original for the sake of the example, if it doesn't make sense

type StepFunction<
  TSchema extends Schema = Schema,
> = (anything: unknown) => {
  readonly schema: TSchema
  readonly toAnswers?: (keys: keyof TSchema) => unknown
}

function step<TSchema extends Schema = Schema>(
    stepVal: StepFunction<TSchema>,
  ): StepFunction<TSchema> {
    return stepVal
  }

EXAMPLES: Notice the returned object of all functions is the same! The difference is in whether we have the argument for the step function or not. Note that if I do Parameters<typeof myStepValue> even when the argument is missing, it's inferred correctly (!)

// WORKS: `keys` is inferred based on the `schema`
// - no argument for `step` function
// - no `return` keyword
const workingExample = step(() => ({
  schema: {
    attribute: 'anything',
  },
  toAnswers: keys => {
    // RESULT: `keys` inferred successfully as `attribute`
    type Test = string extends typeof keys ? never : 'true'
    const test: Test = 'true'
    return { test }
  },
}))
// FAILS: `keys` is not inferred based on the `schema`
// - has argument for `step` function
const nonWorkingA = step(_something => ({
  schema: {
    attribute: 'anything',
  },
  toAnswers: keys => {
    // RESULT: `keys` failed to be inferred hence defaults to `string`
    type Test = string extends typeof keys ? never : 'true'
    const test: Test = 'true'
    return { test }
  },
}))

🙁 Actual behavior

Nested value's function argument cannot be inferred correctly.

If I change the property (toAnswers) from a function to a simple property, there are no inference issues.

🙂 Expected behavior

Nested value function argument should be inferred correctly regardless of the return keyword or declaring the arguments.

Additional information about the issue

Like mentioned above, this is a followup on https://github.com/microsoft/TypeScript/issues/49951 and https://github.com/microsoft/TypeScript/issues/47599

Andarist commented 9 months ago

I have some idea how this might get fixed. Stay tuned 😉 I wonder though - what's anything in your real code? Is it really meant to always be unknown?

ShayDavidson commented 9 months ago

It's an inferable schema object (something like Zod):

schema: {
    attribute: string(),
    anotherAttribute: oneOf([1,2,3])
}

We infer the type from this schema library and thus instead of keys we actually get the actual shape of the object. In this case:

{ attribute: string, anotherAttribute: 1 | 2 | 3 }
Andarist commented 9 months ago

Ye, I get that - but I wonder what's the nature of the outer anything parameter. It stays unused in this example.

I think the fix for this is introducing an extra inference pass on return expressions from context-sensitive functions. I was thinking first about extending the intra-expression inference to handle this but I think that would be a worse solution. It wouldn't handle { toAnswers, schema } (consumer before producer).

It's funny-ish that the intra-expression inference is responsible for making this work:

type Schema = Record<string, unknown>;

type StepFunction<TSchema extends Schema = Schema> = (anything: unknown) => {
  readonly schema: (thing: number) => TSchema;
  readonly toAnswers?: (keys: keyof TSchema) => unknown;
};

function step<TSchema extends Schema = Schema>(
  stepVal: StepFunction<TSchema>,
): StepFunction<TSchema> {
  return stepVal;
}

const nowItsOk = step((_something) => ({
  schema: (thing) => ({
    attribute: "anything",
  }),
  toAnswers: (keys) => {
    keys;
    // ^?

    type Test = string extends typeof keys ? never : "true";
    const test: Test = "true"; // ok
    return { test };
  },
}));

Usually, context-sensitive functions make things to be inferred worse (like in your original example) - but this example above infers better when an extra context-sensitive function is included. This is a little bit unusual and purely related to how all of this is already nested in one context-sensitive function.

ShayDavidson commented 9 months ago

Oh, I thought you meant for the inner anything.

It's a store that allows to get data based on the inference of the schema, something like:

type StoreFor<TSchema extends Schema> = {
  get: <T extends keyof TSchema>(key: T) => TSchema[T]
}

type StepFunction<
  TSchema extends Schema = Schema,
> = (store: StoreFor<TSchema>) => {
  readonly schema: TSchema
  readonly toAnswers?: (keys: keyof TSchema) => unknown
}

Still fails and demonstrated in this playground link.

Update:

See below, this is not a good example. The store's schema should be different. Elaborated later in the thread but keeping the original so the context for the conversation remains.

ShayDavidson commented 9 months ago

As for adding a function - well, I could add it - but it's a breaking change for the consumers of my library hence I prefer to refrain from it 😅

Andarist commented 9 months ago

As for adding a function - well, I could add it - but it's a breaking change for the consumers of my library hence I prefer to refrain from it 😅

Oh, I certainly didn't imply that you should change this anyhow. I just forked ur example to demonstrate a TS behavior 😉 it was more convenient than having to retype the same thing with some dummy function names etc.

It's a store that allows to get data based on the inference of the schema, something like:

Do you have any example of when this store would actually be used and when its type argument would be inferred? My hot take on this example is that there is no easy way to make this one work. It's a circular dependency of sorts. Your return type might depend on this parameter but its type depends on that return type.

ShayDavidson commented 9 months ago

Yes, I'll demonstrate how this is used in actual code more or less - I understand why this is confusing.

The schema defined in my example is not the same schema defined for the store.

Imagine I'm writing a wizard flow, and I have a parent global schema, that I can get previously answered values through the store. Each step in the wizard can define a sub-schema for transient answers (which is defined through the schema attribute). The toAnswers function simply takes these transients answers and needs to return an object that satisfies the parent schema partially.

step((store) => ({
  schema: {
    fullName: string(),
  },
  toAnswers: (transientAnswers) => {
     return { firstName: transientAnswers.get('fullName').split(' ')[0] }
  },
}));

e.g. in this example you can assume I have a store argument that knows it supports only the parent-schema, which has firstName has an attribute, but is completely decoupled from the schema inside the step definition.

ShayDavidson commented 9 months ago

To make things simpler - the step function is actually generated through something I call StepCreator, which literally accepts the parent-schema as argument (so no need to do hard inference there):

const creator = new StepCreator({ parentSchema: { firstName: string(), lastName: string() }})

const step = creator.step((store) => {...}) // like the `step` function above. the `store` argument is typed based on the constructor params
Andarist commented 9 months ago

The schema defined in my example is not the same schema defined for the store.

Ah, ok - then the snipper in https://github.com/microsoft/TypeScript/issues/57021#issuecomment-1888949418 doesn't exactly show the real usage. Thanks for the explanation.

If those are two different schemas then ye, it might work.

RyanCavanaugh commented 9 months ago

This is basically #47599, specifically the part about

Perform a more robust check to determine "true" context-sensitivity -- for example, produce: _a => 0 should not be contextually sensitive because the relational target (n: number) => T does not use T in a covariant position. There are multiple ways we could do this

Andarist commented 9 months ago

I don't quite understand this quote here. Doesn't _a => 0 use T in a covariant position? This 0 is a potential inference source for T.

Regardless, I think this particular issue here can be solved. The fix wouldn't have to rely on any change to context-sensitivity. It's just that returns from context-sensitive functions are not inferred from today (whereas annotated parameters are inferred from). Inferring from such returns recursively would fix this. I plan to make a PR for it soon-ish.

https://github.com/microsoft/TypeScript/issues/47599 was already mentioned in this thread but I find this umbrella issue to not be that helpful. All of the called-out examples in the first post work today. I understand that some of the ones posted below might still not work but it's quite hard to understand at a glance what still doesn't work. At the very least that first post could be revised to include those other broken examples.

RyanCavanaugh commented 9 months ago

Doesn't _a => 0 use T in a covariant position?

Poor wording on my part. What I mean there is that because _a's corresponding position in (n: number) => T is n: number, which has no occurrence of T, then we can be sure that treating _a => 0 as not-context-sensitive for the purposes of inferring T is safe.

typescript-bot commented 9 months ago

This issue has been marked as "Duplicate" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

ShayDavidson commented 9 months ago

@Andarist should have it been closed?

Andarist commented 9 months ago

It has been marked as a duplicate of that umbrella issue - and many issues were linked to it as its duplicates. I think this one was nice since it focused on a more specific problem. I still think this one is fixable (I fixed it locally without breaking any test cases). It was a busy weekend for me though and I'm investigating a better fix than what I have so far. I'll report back when I'm done but it might take some days. TLDR: I still plan to open a PR fixing this 😉

ShayDavidson commented 9 months ago

Amazing, thanks for your effort!

ShayDavidson commented 8 months ago

Hey @Andarist - did you have any chance to look into this?