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

Loss of type inference converting to named parameters object #29791

Closed
yortus opened this issue Feb 7, 2019 · 5 comments
Closed

Loss of type inference converting to named parameters object #29791

yortus opened this issue Feb 7, 2019 · 5 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@yortus
Copy link
Contributor

yortus commented Feb 7, 2019

Working on a strongly-typed wrapper for vuex modules, I've encountered an issue where I can't get type inference to work if I use a named parameters object, but it works perfectly if I use separate parameters.

The following is a cut-down repro showing two functions that are identical apart from how they take their parameters. I'd really prefer to use the named parameters object approach but no matter what I've tried, I can't get the type inference to work like it does with separate parameters. So for now I'm just having to pass separate parameters with comment labels (as shown below), which is far from ideal.

I'd like to know whether this is a bug, or I'm failing to understand some subtlety that accounts for the different treatment of tsc of separate parameters vs named parameter objects. If this behaviour is intended, is there any way to use named parameter objects without losing type inference?

TypeScript Version: 3.4.0-dev.20190206

Search Terms: named parameters object

Code

// takes separate parameters `state`, `mutations`, and `actions`
declare function separateParams<State, Mutations>(
    state: State,
    mutations: Mutations & Record<string, (_: State) => void>, // NB: contextual type uses State
    actions: Record<string, (_: Mutations) => void>            // NB: contextual type uses Mutations
): void;

// takes a named parameters object `{state: ..., mutations: ..., actions: ...}`
declare function namedParamsObj<State, Mutations>(obj: {
    state: State,
    mutations: Mutations & Record<string, (_: State) => void>, // NB: contextual type uses State
    actions: Record<string, (_: Mutations) => void>            // NB: contextual type uses Mutations
}): void;


// infers    State = {s1: number}   Mutations = {m1: (s: {s1: number}) => void}
separateParams(/*state:*/ {s1: 42}, /*mutations:*/ {m1: s => {}}, /*actions:*/ {a1: m => !m.m1});

// infers    State = {s1: number}   Mutations = {}
namedParamsObj({   state: {s1: 42},     mutations: {m1: s => {}},     actions: {a1: m => !m.m1}});
//                                       ERROR TS2339: 'm1' does not exist on type '{}' ----^^

Expected behavior:

Same type inference for both separateParams and namedParamsObj.

Actual behavior:

separateParams is perfectly typed. But type inference is much less useful in namedParamsObj despite its almost identical looking declaration.

Playground Link: here

Related Issues:

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Feb 7, 2019

So it's late and I haven't played around with this deeply, but I think this is likely because we defer contextual typing on "contextually sensitive" arguments to get better inferences from non-sensitive arguments. In your first example, that non-sensitive argument is usually passed in for the state parameter.

In your refactored version, there's no other argument to try first for better inferences. Any callback for mutations will pull on the type of State, causing inference to fix that type parameter - basically freezing the inference process for a State and going with whatever it's inferred so far. Since no inference candidates have been found, it goes with the constraint of State ({} by default, as you're seeing here).

But in short, we can no longer utilize this concept of "contextually sensitive arguments" with named parameter patterns. I wonder if there's a world where you could imagine contextually sensitive properties?

@DanielRosenwasser
Copy link
Member

CC @ahejlsberg

@ahejlsberg
Copy link
Member

We've run into this issue before. It works with discrete parameters because we specifically arrange to allow inferences from previous parameters to be fixed and applied in subsequent parameters in a left-to-right manner. Once you convert to a single parameter there is no longer a left-to-right phasing of the inferences. We could explore some sort of one-property-at-a-time scheme for object literals, but it gets complicated because they can be arbitrarily nested. Furthermore, it seems somewhat more suspect to have a left-to-right ordering dependency in object literals since there really isn't a way (nor should there be) for an API to indicate such a requirement.

@yortus
Copy link
Contributor Author

yortus commented Feb 9, 2019

Thank you both for the insights. So if I change the order of the parameters in separateParams then it behaves just like namedParamsObj, with impaired type inference (playground link).

I take it this left-to-right order-dependence exists to simplify tsc's implementation? Because order shouldn't matter for type inference in principle, right?

@RyanCavanaugh RyanCavanaugh added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Feb 14, 2019
@RyanCavanaugh
Copy link
Member

@yortus order matters in the absence of a full unification algorithm. Consider something like this, where the type parameters create a full circular dependency graph but we're still able to do the right thing:

function chain<I, R1, R2>(arg: I, f1: (arg: I) => R1, f2: (r: R1) => R2, f3: (r: R2) => I): I {
  return f3(f2(f1(arg)));
}

chain(
  new Date(),
  p => p.toString(),
  s => s.length,
  n => new Date(n));

jeremija added a commit to rondomoon/rondo-framework that referenced this issue Jan 10, 2020
After spending almost two days in finding the issue, I ran across a few
TypeScript issues on their GitHub page:

- Loss of type inference converting to named parameters object
  microsoft/TypeScript#29791

- Parameter of a callback without a specified type next to it breaks code.
  microsoft/TypeScript#29799

- Convert to named parameters
  microsoft/TypeScript#30089

It became clear that TypeScript is unable to infer method return
arguments if a generic type is used more than once in generic parameter
object. Instead it returns {}.

For example, the following would fail on line 28:

    type Convert<A, B> = (value: A) => B

    interface IParams<C, D> {
      value: C
      convert: Convert<C, D>
      doConvert: (value: C, convert: this['convert']) => D
    }

    function doSomething<E, F>(value: E, convert: Convert<E, F>) {
      return convert(value)
    }

    function build<G, H>(params: IParams<G, H>) {
      const {value, convert} = params
      return params.doConvert(value, convert)
    }

    const outerResult = build({
      value: {
        a: {
          value: 1,
        },
        b: 'string',
      },
      convert: value => value.a,
      doConvert: (value, convert) => {
        const innerResult = doSomething(value, convert)
        innerResult.value
        console.log('innerResult:', innerResult)
        return innerResult
      },
    })

    console.log('outerResult:', outerResult)

With the message:

    Property 'value' does not exist on type '{}'.

If we replace parameter object IParams with regular ordered function
parameters, the compilation succeeds.

RyanCavanough (TS project lead) from GitHub commented:

> We don't have a separate pass to say "Go dive into the function and
> check to see if all its return statements don't rely on its parameter
> type" - doing so would be expensive in light of the fact that extremely
> few real-world functions actually behave like that in practice.

Source: microsoft/TypeScript#29799 (comment)

These modifications bring type safety to TestUtils.tsx, and therefore
client-side tests of React components, while keeping almost the same
ease of use as before.
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

4 participants