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

Parameters<...> should always be spreadable #47615

Closed
J3MPA opened this issue Jan 26, 2022 · 14 comments
Closed

Parameters<...> should always be spreadable #47615

J3MPA opened this issue Jan 26, 2022 · 14 comments
Labels
Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@J3MPA
Copy link

J3MPA commented Jan 26, 2022

Bug Report

🕗 Version & Regression Information

Version: 4.5.4

⏯ Playground Link

Playground link

💻 Code

const doSomething = (arg:any) => !!arg

type ActionCreators = typeof doSomething

const dispatch = (action: ReturnType<ActionCreators>) => {
    console.log(action)
}

const withDispatch =<Action extends ActionCreators>(action:Action)=> (...args: Parameters<Action>)=> dispatch(action(...args)) // A spread argument must either have a tuple type or be passed to a rest parameter. (2556)
const withDispatchApply =<Action extends ActionCreators>(action:Action)=> (...args: Parameters<Action>)=> dispatch(action.apply(null, args)) // no error
const withDispatchCall =<Action extends ActionCreators>(action:Action)=> (...args: Parameters<Action>)=> dispatch(action.call(null, ...args)) // no error

🙁 Actual behaviour

When creating a higher order function that is going to operate on a certain set of functions TSC tells us A spread argument must either have a tuple type or be passed to a rest parameter.(2556) when we try to spread the arguments to it.

🙂 Expected behaviour

As with action.apply(null, args) and action.call(null, ...args) I expect that the inferred type of Parameters to always be spreadable.

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

The type Parameters<Action> is deferred so there's not a way for TS to tell that this is always going to end up producing a type that happens to make a correct call. A type assertion is appropriate here

  /*[...]*/ action(...args as [any]);

@J3MPA
Copy link
Author

J3MPA commented Jan 28, 2022

@RyanCavanaugh Yeah I get that, the issue I do not get is when the hof is a generic on Any function the error goes away.

type AnyFn = (...args: readonly unknown[]) => any
const withDispatch = <Action extends AnyFn>(action: Action) => (...args: Parameters<Action>) => dispatch(action(...args)) // no error

In my first example I would like to argue that ActionCreators is just a subset of the above AnyFn yet when I make the hof generic on that subset it gives me an error.

@fatcerberus
Copy link

I don’t know if this is related but AnyFn does not actually mean “any function”. Because of contravariance anything that extends it must be able to accept any number of parameters of any type. (if I have a function that takes unknown, I can pass it anything).

@J3MPA
Copy link
Author

J3MPA commented Jan 29, 2022

@fatcerberus Indeed, but it's the most common and accurate signature of Any function.

But if we look at it from the statement (statement A):

The type Parameters is deferred so there's not a way for TS to tell that this is always going to end up producing a type that happens to make a correct call.

  1. AnyFn is a function signature.
  2. doSomething is a function.
  3. doSomething extends AnyFn.
  4. doSomething extends ActionCreators.
    Ergo: <Action extends ActionCreators> ... is an implementation of <Action extends AnyFn> ... and should therefore have the same behavior in TS.

See the only difference between the two implementations is that one says that the higher order function is bound to operate on any function and the other is bound to operate on a certain set of functions (ActionCreators). I.e. the second is a subset implementation of the first.

@fatcerberus
Copy link

Variance concerns aside, I think the key to why AnyFn doesn’t exhibit the problem is in the error message:

A spread argument must either have a tuple type or be passed to a rest parameter.

Emphasis mine. Parameters is still deferred, but since AnyFn has a rest parameter, that doesn’t matter and the spread is allowed.

@fatcerberus
Copy link

fatcerberus commented Jan 29, 2022

<Action extends ActionCreators> is an implementation of <Action extends AnyFn>

I do want to note for the record, however, that this doesn’t follow—it’s the other way around. I can’t implement a generic function where <T extends Animal> purely by way of <T extends Cat>. The latter is, at best, a partial implementation and is less capable by definition: I can safely pass dogs only to the former.

@J3MPA
Copy link
Author

J3MPA commented Jan 29, 2022

I do want to note for the record, however, that this doesn’t follow—it’s the other way around.

Yes you are right, my reasoning was wrong. I think what I was trying to say is that ActionCreators is a subset of AnyFn (because any function is an implementation of AnyFn) therefore <Action extends ActionCreators> is one of many implementations of <Action extends AnyFn>.

Parameters is still deferred, but since AnyFn has a rest parameter, that doesn’t matter and the spread is allowed.

I get this, but the fact is that we can deterministically say that Parameters<...> will always give us a tuple type (if the type passed to it extends (...args: any) => any, which we know it does in this case) and that makes this A spread argument must either have a tuple type ... wrong in my opinion, even if Parameters is deferred. Hence the title Parameters<...> should always be spreadable

@fatcerberus
Copy link

Somewhat surprisingly, (...args: string[]) => void extends (i.e. is assignable to) (arg: any) => any but taking its Parameters gives you string[], while the latter gives you [any]. So the spread gets disallowed because it doesn't know whether the (deferred) Parameters will actually be compatible with the call (in practice it will because you're calling a T with a Parameters<T> but that requires higher-level analysis than the compiler is currently capable of). AnyFn bypasses this because it's explicitly declared with a rest parameter--meaning you can always call it with a spread per the second part of the quoted error message.

For what it's worth, I've always felt there should be better error messages for generic cases like this. Rather than an absolute refusal ("is not assignable to", etc.), the compiler should be clearer that it's only preventing the operation because it can't verify whether it's safe.

@ahejlsberg
Copy link
Member

I suggest writing the function like this instead:

const dispatch = <T>(action: T) => {
    console.log(action)
}

const withDispatch = <A extends unknown[], R>(action: (...args: A) => R) => (...args: A) => dispatch(action(...args));

Or, since the dispatch function doesn't seem to care about the type of its operand, just:

const dispatch = (action: unknown) => {
    console.log(action)
}

const withDispatch = <A extends unknown[]>(action: (...args: A) => unknown) => (...args: A) => dispatch(action(...args));

The key difference here is that the types are as specific as possible, revealing to the checker that A is a spreadable type. In the original formulation, the checker isn't able to prove that Parameters<Action> is always a spreadable type.

@J3MPA
Copy link
Author

J3MPA commented Feb 1, 2022

@ahejlsberg For the case of this bug report I've made all functions and types as simple as possible (you should look at all implementations in the example code as arbitrary except where the error is shown). In reality (and as described) I want to allow only a set of specific functions and not any function in the withDispatcher hof.

The reason for the bug report is to highlight a possible issue with the compiler.

@ahejlsberg
Copy link
Member

For the case of this bug report I've made all functions and types as simple as possible

Right, but my point was that you've chosen the representation Action extends ActionCreators for your function parameter type, where the structure of Action is only indirectly observable through the constraint of the type parameter, as opposed to something like (...args: A) => R, where it is immediately clear that the parameter is a variadic function type. So, the checker can immediately know that A is spreadable, whereas it is significantly more complex to deduce that from Parameters<Action>. Now, if the ActionCreators constraint is more specific in your real world scenarion, you could similarly make a function type that is more specific than (...args: A) => R. That's really an orthogonal issue.

@J3MPA
Copy link
Author

J3MPA commented Feb 1, 2022

@ahejlsberg Yes indeed you are right. How ever only functions in the set of ActionCreators will produce anything meaningful. So I agree with you in theory, but in practice at least at this point it can create unexpected behaviors or at worst case blow up.

So, the checker can immediately know that A is spreadable, whereas it is significantly more complex to deduce that from Parameters.

This here is exactly why I created the issue. Because no matter the arguments or non arguments of Action as long as Action extends a function signature we can deterministic say that Parameters<Action> is spreadable. And in my example it clearly extends a function signature. If Action where to extend something that does not satisfy the constraints of (...args: any) => any it will give us an error.

@jcalz
Copy link
Contributor

jcalz commented Nov 9, 2023

#36874 hmm is this a bug or a design limitation?

@RyanCavanaugh
Copy link
Member

What is a bug but a design limitation persisting?

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

5 participants