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

Revise 'left' and 'right' typings for typesafety #543

Closed
sledorze opened this issue Aug 10, 2018 · 9 comments
Closed

Revise 'left' and 'right' typings for typesafety #543

sledorze opened this issue Aug 10, 2018 · 9 comments
Labels
Milestone

Comments

@sledorze
Copy link
Collaborator

sledorze commented Aug 10, 2018

Today's typing for 'left' and 'right' are unsafe due to the way typescript does it's inference and causing us some trouble.

Example:

export declare const left: <L, A>(l: L) => Either<L, A>;
export declare const right: <L, A>(a: A) => Either<L, A>;

const res = (cond: boolean) => (cond ? left(42) : right('what?')) // return type: Left<number, {}> | Right<number, {}> | Left<{}, string> | Right<{}, string>
const v = res(true)
if (v.isRight()) {
  v.value // string | {} (!! WRONG)
} else {
  v.value // number | {} (!! WRONG)
}

Thanks to the way union behave with never

type T = number | never // Type is number

With these typings inference works as expected

export declare const left: <L>(l: L) => Either<L, never>;
export declare const right: <A>(a: A) => Either<never, A>;

const res = (cond: boolean) => (cond ? left(42) : right('what?')) // return type: Left<number, never> | Right<number, never> | Left<never, string> | Right<never, string>
const v = res(true)
if (v.isRight()) {
  v.value // string (!! OK)
} else {
  v.value // number (!! OK)
}

I know this would be a breaking change,
so here's a backward compatible non breaking solution:

export declare const left: <L, A = never>(l: L) => Either<L, A>
export declare const right: <L = never, A = any>(a: A) => Either<L, typeof a>

Do you see any issue with it? (Being typesafe is priceless)

@gcanti
Copy link
Owner

gcanti commented Aug 10, 2018

@sledorze thanks, looks good to me.

Maybe we could use never instead of any here (also not sure why you used typeof a)

export declare const right: <L = never, A = never>(a: A) => Either<L, A>

There are other modules that can be improved

  • Validation
  • IOEither
  • TaskEither

@sledorze
Copy link
Collaborator Author

I used 'typeof a' because of an inference fatigue habit (it often has fewer defect).
But actually that does not help here.

The interesting case is someone who wants to take advantage and use that signature:

right<number>('what?')

But you made me realise some issues:

right: <L = never, A = any>(a: A) => Either<L, A>
right<number>('what?') // Type is Either<number, any>  (WRONG)
right: <L = never, A = never>(a: A) => Either<L, A>
right<number>('what?') // Argument of type '"what?"' is not assignable to parameter of type 'never'.  (WRONG)
right: <L = never, A = never>(a: A) => Either<L, typeof a>
right<number>('what?') // Argument of type '"what?"' is not assignable to parameter of type 'never'.  (WRONG)
right: <L = never, A = any>(a: A) => Either<L, typeof a>
right<number>('what?') // Type:  Either<number, any> (WRONG)

So we must prevent any partial Type Param Bindings:

right: <L = never, A = 'reason is you cannot partially bind Type Params to `right`'>(a: A) => Either<L, typeof a>
right<number>('what?') // Argument of type '"what?"' is not assignable to parameter of type '"reason is you cannot partially bind Type Params to `right`"'

That's still really acceptable (and a great improvement over the current status).

@gcanti
Copy link
Owner

gcanti commented Aug 10, 2018

@sledorze this PR seems related, should we wait to see how it pans out?

@sledorze
Copy link
Collaborator Author

@gcanti Agree

@sledorze
Copy link
Collaborator Author

@gcanti actually, I've taken the time to read the PR (yesterday was a busy day).
I think we should not wait for it.

The reason being that what this PR implements is a call site resolution.
While being a great feature, using it for fixing our inference requiers the Library user to know about it and use it.

Otherwise, inference will still generates Wrong types.

Also it requiers to write extra code where it shouldn't do anything in the first place.

To summarise: Still errors by default and more verbose.
So I take back my previous answer.

I've tested the new sign just on those two 'left' and 'right' constructors and rebuild our code base; no problem and I've also been able to remove all explicit typings on the few case I've tried.

@gcanti Maybe we should start with Either module and see how it works? What do you think?

@sledorze
Copy link
Collaborator Author

@gcanti I've made a branch with a POC implementation.

@gcanti
Copy link
Owner

gcanti commented Aug 11, 2018

using it for fixing our inference

No, my concern has more to do with not knowing how the new feature will play with default type arguments, i.e

// x1: Either<never, number>
const x1 = right(1) // ok
// x2: Either<string, number>
const x2 = right<string, number>(1) // ok

// x3: Either<string, number> or Either<string, never> ?
const x3 = right<string, infer>(1) // ok?

That is, will infer take precedence over the default? My guess is yes, but I'm not sure.

So I'd wait for that PR to be merged in order to play with it by installing typescript@next

@sledorze
Copy link
Collaborator Author

sledorze commented Aug 11, 2018

@gcanti you meant:

// x3: Either<string, number> or Either<string, 'reason is you cannot partially bind Type Params to `right`'> ?
const x3 = right<string, infer>(1) // ok?

Note: we should maybe find a better message, it is nice as part of an error message but does not really make sense otherwise.

@sledorze
Copy link
Collaborator Author

This has been solved with version 1.19.1
Closing

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

No branches or pull requests

2 participants