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

Composition and argument order: pipe ok, compose errors #92

Closed
wip0 opened this issue Sep 6, 2016 · 7 comments
Closed

Composition and argument order: pipe ok, compose errors #92

wip0 opened this issue Sep 6, 2016 · 7 comments

Comments

@wip0
Copy link
Contributor

wip0 commented Sep 6, 2016

const x = R.cond([
  [R.F, R.F],
  [R.T, R.identity]
]);

const y = R.compose(x, R.inc);

If I use compose or pipe with cond function, there is an compiling error.

Error TS2345: Argument of type 'Function' is not assignable to parameter of type '(x0: {}, x1: {}, x2: {}) => {}'.
Type 'Function' provides no match for the signature '(x0: {}, x1: {}, x2: {}): {}'

at compose/pipe line.

@wip0 wip0 changed the title compose/pipe with cond compose/pipe with cond issues Sep 8, 2016
@KiaraGrouwstra
Copy link
Member

So, I think there are two things going on here; an issue with cond, and an issue with composition.

cond:

We'd like for TS to Infer that x here has the identity type signature. This is currently problematic, in the sense that TypeScript is currently fairly ignorant with regard to values.
Take the example let a = false ? 1 : 'a';. TypeScript is ignorant of the false value, presumes both paths will be used, and infers number | string.
Given it fails on such basic examples, I'm not sure we can make it understand these cond conditions and infer which might hold true. This issue seems hard to fix for the foreseeable future.

composition:

Let's say we work around the above issue and help it with x here, by manually annotating it as <T>(v: T) => T.
At this point, compose should fill in the blanks in terms of connecting number => number and T => T into number => number. It seems to fail at this however, by evaluating the second function's signature before passing in any number info, losing the info on generic T and retyping it as {} => {}. At this point, it concludes the resulting function to be typed number => {}.

This issue is pretty important, and sounds similar to issues with generics in the context of currying brought up here. I don't have a solution yet, but hopefully we can figure something out.

@KiaraGrouwstra
Copy link
Member

KiaraGrouwstra commented Dec 10, 2016

I just realized the definitions for regular/promise versions of pipe/compose are returning curried functions, while they shouldn't. I now fixed that.

This hasn't actually fixed the root cause here. I'm inclined to think though that the actual cause here mirrors the problem in the issue above. Based on that, let's stick to that thread for the moment; we can reopen this if they turn out different after all. Your example is part of the test suite now, so we won't forget.

Thanks again for bringing this up!

@KiaraGrouwstra
Copy link
Member

I've done an odd find -- inference appears to depend on parameter order. Consider the following contrast:

// can't infer cond paths, must annotate:
const x: <T>(v: T) => T = R.cond([
    [R.F, R.F],
    [R.T, R.identity]
]);
// argument order matters for some reason...
const y: (v: number) => number = R.pipe   (R.inc, x); // ok
const z: (v: number) => number = R.compose(x, R.inc); // boom

This is super weird. pipe and compose are identical in their type definitions other than having their params flipped.

Apparently pipe is safer than compose...

@KiaraGrouwstra KiaraGrouwstra changed the title compose/pipe with cond issues Composition and argument order: pipe ok, compose errors Dec 11, 2016
@KiaraGrouwstra
Copy link
Member

Closing as a TypeScript limitation, see microsoft/TypeScript#15680 (comment). Added to the readme to inform users.

@KiaraGrouwstra
Copy link
Member

Actually, in new TS compose now works as well/so-so as pipe. Not sure from which version exactly though, but eventually we may wanna remove this from the readme. @ikatyang

@ikatyang
Copy link
Member

I could still reproduce the issue in TS 2.8-rc, did I miss something?

image

@KiaraGrouwstra
Copy link
Member

Okay, guess I'm gonna have to figure out where it did work then... weird.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants