Fp-ts: Supplying a non-Either value with a type assertion should cause isLeft and isRight to return false

[leilapearson]

🐛 Bug report

Current Behavior

isLeft will return undefined when called like this:

isLeft(('Left' as unknown) as Either<unknown, unknown>);

isRight will return true when called like this:

isRight(('Right' as unknown) as Either<unknown, unknown>);

Expected behavior

When the supplied object is not an Either (neither a Left nor a Right), both isRight and isLeft should return false.

Reproducible example

See above, and see tests in pull request to be submitted shortly.

Suggested solution(s)

Will submit a pull request shortly.

Additional context

Ran into this issue when trying to use isLeft and isRight as type guards for some custom Jest matchers I created for matching against Either values:

expect.toBeEither(received: unknown)
expect.toBeLeft(received: unknown)
expect.toBeRight(received: unknown)

etc...

Your environment

Which versions of fp-ts are affected by this issue?

| Software | Version(s) |
| ---------- | ---------- |
| fp-ts | 2.0.0+ (tested with master and 2.5.3) |
| TypeScript | 3.5+ (tested with 3.7.4) |

The bug exists from the time that isLeft and isRight were introduced in fp-ts 2.0.0.

[leilapearson]

Should I be adding checks for null and undefined too?

The current code will error at runtime if you pass a null or undefined value:

TypeError: Cannot read property '_tag' of null

TypeError: Cannot read property '_tag' of undefined

If it makes sense to reviewers to add these checks, then I can take a look at other is* functions too (such as Option.isSome() and Option.isNone() for example) and make that change for all of them in a separate pull request.

[giogonzo]

Hi @leilapearson, fp-ts is meant to be used in TypeScript (and with a strict: true tsconfig). The code samples above are indeed not valid TS (well.. by casting, TS is basically turned off).

If for any reason you want to allow unknowns as input in a jest matcher, I think you should first make sure that that value is an Either, then run the type guard to check if it's Right.

Using io-ts (and if I got the intent of e.g. toBeRight correctly), it could look something like:

import * as t from 'io-ts'
import * as E from 'fp-ts/lib/Either'
import { either } from 'io-ts-types/lib/either'

function toBeRight(received: unknown): asserts received is E.Right<unknown> {
  if (either(t.unknown, t.unknown).is(received) && E.isRight(received)) {
    return;
  }
  throw new Error('...')
}

// example:

declare const received: unknown
toBeRight(received)
// if the assertion was successful, from this line on `received` is of type `Right<unknown>`

[leilapearson]

Hi @giogonzo, thanks for the info.

I am indeed using TS with strict: true, and for my toBeRight and toBeLeft jest matchers I'm letting the compiler check for an Either by only accepting an Either as an argument to those functions. For toBeEither though it seemed to make sense to accept an unknown, since -- if I'm checking -- there must be some reason to believe I might not have an Either.

This is the code where I ran into problems:

function isEither(received: unknown): received is Either<unknown, unknown> {
  return (
    received !== undefined &&
    received !== null &&
     isLeft(received as Either<unknown, unknown>) === true || isRight(received as Either<unknown, unknown>) === true
  );
}

My tests were failing because isRight was telling me I had an Either in cases where I clearly did not. isLeft on the other hand at least wouldn't return true in these cases.

fp-ts/lib/Either doesn't have an isEither function, or I would have used that instead of building my own.

I hadn't thought to use io-ts-types to check the received value, but of course it does make sense as it is specifically meant to validate un-validated input. I was hoping to limit my dependencies to just fp-ts proper for these matchers in the event that I package them up at some point and distribute them. I do make lots of use of io-ts and io-ts-types in my application though.

It also makes sense that you would expect isRight and isLeft to only be called in cases where you know you have an Either (and indeed that's what their type signatures say) and just want to know what flavour of Either you have.

All this said, it seems to me that the behavior of both isRight and isLeft ought to be consistent instead of inconsistent - even under circumstances like these. This small change accomplishes that without making the operation any more expensive - and indeed by making it slightly less expensive by using default instead of a specific check.

As for checking for null and undefined inputs, it does seem fair to let the strict compiler option take care of these, so that answers that question at least.

Bottom line, I agree with your points that this is not so much a bug but more of an enhancement. I do think the change itself still has value, but perhaps the tests I added don't make sense to add...

[giogonzo]

This is the code where I ran into problems:

Yes, you'll never be able to use any function like isRight from fp-ts/lib/Either without providing known Either values.
In order to get TS to refine an unknown value into a value of the shape that Either has ({ _tag: 'Right' , right } | { _tag: 'Left', left }) you could either use io-ts(types) or roll your own, as long as you are never required to cast.

I was hoping to limit my dependencies to just fp-ts

fp-ts itself is not concerned with parsing in the core lib. But I believe importing the required bits from io-ts-types wouldn't have many drawbacks.

All this said, it seems to me that the behavior of both isRight and isLeft ought to be consistent instead of inconsistent - even under circumstances like these

I don't think so, because the inconsistency here only derives from the fact that the functions are called providing parameters that are outside of the domain specified by their signatures. Their behavior in such cases is not specified.
If we start thinking in these terms, we'd end up adding runtime checks for every function and every argument, something definitely out of scope for this lib which is intended for TS (not JS) use

[leilapearson]

I agree, but this change doesn't add any runtime checks?

The change only updates the internal function implementation in a way that that is fully compliant with the type signatures and the expected behavior when called with proper Either values. It doesn't check for non-Either values and it doesn't do anything special to handle non-Either values.

I've updated the pull request to remove the tests that I had added since they were invalid and unnecessary per this discussion.

If you don't want the change, just let me know and I can just close the pull request - no hard feelings at all. As you point out, the change isn't really necessary. It's slightly cleaner this way I think but that's personal preference.

Thanks again for the time you spent explaining your position and the fp-ts philosophy, regardless of whether or not you decide to accept the PR.

[giogonzo]

I agree, but this change doesn't add any runtime checks?

right, I got confused by:

Should I be adding checks for null and undefined too?

but indeed the PR as of now is not adding runtime checks.

What it does though is adding the default switch case. As I see it, this has two effects:

1. more cases are handled "correctly". But again, this definition of correctness is not clear since it's not specified by the signature. And as such, from my understanding, fp-ts is not interested in it. Note that however, even after the change, isRight still fails e.g. on null or undefined (runtime error), or with { _tag: 'Right', foo: 'bar' } (returns true even if the value is not even of the correct shape), etc.

2. (unrelated to the main discussion here) it opts out of TS exhaustivity checking, obtained in this code base by annotating a return type on the function and avoiding the default case

Summing up, I don't think the original issue is a bug, nor that the PR should be merged.

I hope I was able to make my point, I'm not really good at words 😅Thank you @leilapearson for the report and discussion

[leilapearson]

Thanks. The exhaustive check info is useful too. I'd always considered that adding a default case was a best practice - but of course it can lead to subtle bugs where you inappropriately handle some new value in the default case.

Closing the PR :-)

P.S. Hope things are well with you, your family and your friends. Things here are far from normal due to COVID-19. I can't begin to imagine what it's like for you in Italy.

[giogonzo]

it can lead to subtle bugs where you inappropriately handle some new value in the default case.

Exactly 👍

Hope things are well with you

All good for now, thanks for asking :) Very strange times here and soon I believe in the rest of the world. Wish you all the best!