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

Allow use of non-null assertion on Eithers #1343

Open
mmkal opened this issue Oct 28, 2020 · 14 comments
Open

Allow use of non-null assertion on Eithers #1343

mmkal opened this issue Oct 28, 2020 · 14 comments

Comments

@mmkal
Copy link

mmkal commented Oct 28, 2020

🚀 Feature request

Current Behavior

const myEither = E.right(123)

const num1 = myEither.right // Error: Property 'right' does not exist on type 'Left<never>'.ts(2339)
const num2 = myEither.right! // Error: Property 'right' does not exist on type 'Left<never>'.ts(2339)

Desired Behavior

const myEither = E.right(123)

const num1 = myEither.right // Error: Property 'right' does not exist on type 'Left<never>'.ts(2339)
const num2 = myEither.right! // OK: num2 has type 'number'

Suggested Solution

Update the Left and Right interfaces to:

export interface Left<E> {
  readonly _tag: 'Left'
  readonly left: E
  readonly right?: undefined
}
export interface Right<A> {
  readonly _tag: 'Right'
  readonly right: A
  readonly left?: undefined
}

By adding the optional properties with type undefined, we've kept the type system accurate - and in fact more informative. We know that an Either must not have the "other" property. But we've allowed non-null assertions in code where we're sure that the the value is a right (or left, as the case may be).

Who does this impact? Who is this for?

Specifically, I'm thinking about test code, or temporary debugging to reveal types in IDEs. In most production cases, it's better to use pipe, or isLeft/isRight, or check that _tag property. But that's an unnecessary overhead in lots of non-production cases, and this would be very convenient without introducing any danger.

To protect against abuse, there's the no-non-null-assertion eslint rule - which is nice because it can be applied selectively. For example, only to production code, and the rule can be relaxed in *.test.ts files, or similar.

Describe alternatives you've considered

Checking ._tag works, but it's awkward sometimes.

Your environment

Software Version(s)
fp-ts 2.8.5
TypeScript 4.0.3
@SRachamim
Copy link
Contributor

SRachamim commented Oct 28, 2020

I support your desired behaviour.

Anyway, I think your suggested solution (adding an optional value of left/right) doesn't fit with the philosophy of "invalid state should be unrepresentable". By adding an optional right value to the Left interface, you allow others to create a Left with a right value - which is absurd.

While the model should not be changed (it describes the nature of Either correctly), the type signature of the constructors may be improved:

// return-type of `Right<A>` instead of a generic `Either<never, A>`
export const right = <A>(a: A): Right<A> => ({ _tag: 'Right', right: a })

// return-type of `Left<E>` instead of a generic `Either<E, never>`
export const left = <E>(e: E): Left<E> => ({ _tag: 'Left', left: e })

I don't know what could be the reasons it wasn't implemented this way (it may be because of historic TypeScript limitations).

@mmkal
Copy link
Author

mmkal commented Oct 28, 2020

invalid state should be unrepresentable

Since typescript uses structural typing, this is already the case, and in fact it's worse right now than it would be with this change:

const doSomethingWithEither = <L, R>(e: Either<L, R>) => ...

const myEither = {
  _tag: 'Right',
  right: 123,
  left: 456,
} as const;

doSomethingWithEither(myEither)

Right now, this compiles without errors. With this change, the compiler would catch the mistake.

And an optional property with type undefined is technically accurate. The ? means the key is allowed to be missing. Which is true - it just so happens to always be missing.

@SRachamim
Copy link
Contributor

SRachamim commented Oct 28, 2020

That's not how ADTs work and could encourage bad habits. The model of Left should not declare a possible right value (it's never true) just because it's (sometimes) awkward to match the tag value.

Think about the 99.99% (I would even say 100%) of use cases where developers will look at this right? on the Left: In which circumstances will the Left hold a right? Should they worry about it? Should they test it or verify it? No, they shouldn't. Good model is a model which is honest about its constraints and possible shapes. Left will never ever hold a right value so no need to declare it as a possibility.

Your suggested solution is a kind of hack. I believe there are better ways to achieve the desired behaviour, and if not, then it's ok to be satisfied with the awkwardness of testing the _tag value (which I don't find awkward at all, especially when it's just a possible fallback to the guards and destructors the library offers).

@mmkal
Copy link
Author

mmkal commented Oct 28, 2020

That's not how ADTs work and could encourage bad habits

I'm curious what bad habits you envisage from this. I'd argue that the added inconvenience of having to use type-guarded if checks or similar is more likely to lead to bad habits like casting to any, especially for newcomers. I'm thinking of scenarios where the Either has come from an io-ts validation, as the return value from some other function, and import ... from 'io-ts' or import ... from 'fp-ts/...' doesn't appear anywhere in the file. Having to import helper functions seems like unnecessary overhead, which will inevitably lead to developers taking dangerous shortcuts.

I'm ready to accept the point that it's unusual, and therefore potentially confusing. I'm just struggling to picture a case where the answer to "Should they worry about it?" Or "Should they test or verify it?" would lead to a bad outcome. Checking against undefined is all the compiler would allow them to do, and that would be perfectly ok. the .right value of any Left will yield undefined. No dishonesty is introduced.

Good model is a model which is honest about its constraints and possible shapes

I agree. Right now, the model tells "nothing but the truth". I want something closer to "the whole truth". In typescript, all conforming to a shape { _tag: 'Left', left: E } means is that a value has those properties, with those types. But it may have all kinds of other properties too. We're in a position to make a stronger statement - right must never be defined.

In fact, TypeScript has the never type for situations like this, which might be clearer:

export interface Left<E> {
  readonly _tag: 'Left'
  readonly left: E
  readonly right?: never
}
export interface Right<A> {
  readonly _tag: 'Right'
  readonly right: A
  readonly left?: never
}

@mlegenhausen
Copy link
Collaborator

I want something closer to "the whole truth". In typescript, all conforming to a shape { _tag: 'Left', left: E } means is that a value has those properties, with those types.

For "the whole truth" we would need to define all possible attributes that are not allowed to be on the Left or Right value which in the end seems to be an infinite set of properties. So by setting one attribute that is not allowed you get no where nearer to the "truth".

Besides that you make the DX worse cause now right? and left? are prompting up when you try to define your types manually.

Having to import helper functions seems like unnecessary overhead, which will inevitably lead to developers taking dangerous shortcuts.

Taking shortcuts in programming leads always to bad code quality but it does not harm the correctness of your program even when the developer adds attributes that do not belong to this type.

@mmkal
Copy link
Author

mmkal commented Oct 29, 2020

For "the whole truth" we would need to define all possible attributes

That's why I used the word "closer". Most attributes are completely irrelevant and provide no value. Not sure why you're bringing them up.

Besides that you make the DX worse cause now right? and left? are prompting up when you try to define your types manually.

I don't follow. I think the DX would be a lot better. Users are usually dealing with Eithers, not Rights or Lefts. Right now, the only property that pops up is _tag, which is at best not so helpful and at worst misleading.

Taking shortcuts in programming leads always to bad code quality but it does not harm the correctness of your program

  1. It can harm correctness. See my example above where a currently-valid Either has both a left and a right property. Similarly, casting to any can cause all manner of real problems.
  2. Even if correctness couldn't be harmed, wouldn't it be better to avoid the need for the shortcuts and improve code quality?

@mmkal
Copy link
Author

mmkal commented Oct 29, 2020

Anyway, it seems I'm in the minority here. I'll leave open in case @gcanti happens to agree with me!

@steida
Copy link
Contributor

steida commented Nov 11, 2020

@mmkal I think you should read the answers again, slowly and carefully, because what you want is simply based on aesthetic feelings. Remember, the design is not how things look but how things work. It's only a barrier in your mind.

@mmkal
Copy link
Author

mmkal commented Nov 11, 2020

Thanks for the sage advice, @steida. I read every comment carefully, and I believe I understand them. I also responded to them - if you feel I haven't, feel free to reply to what I actually wrote.

You are right, though, that this is a feature request based on aesthetics. Code is written for humans. Library design should consider both aesthetics and functionality. It shouldn't sacrifice correctness for aesthetics, but I believe in this case it doesn't. I maintain that my suggestion is an accurate model of the data structure, and the aesthetics are better. This request is based on using fp-ts in the real world, where there are negative side-effects of bad aesthetics, because of human imperfection. Human beings have feelings and care about aesthetics, so when the design forces them to write ugly code, the chances that they'll sidestep the ugliness in a dangerous way increase - especially in a language like typescript with any, ts-ignore etc. Maybe they shouldn't, but they do. This isn't fp-ts's fault. But it could help in a small way, if it chose to.

To repeat what I said two weeks ago:

Anyway, it seems I'm in the minority here. I'll leave open in case @gcanti happens to agree with me!

Assuming that's not the case, @gcanti feel free to close this issue - it's a pretty low-quality discussion at this point.

@xuanduc987
Copy link

I want to add that you could also cast E.Either to E.Right

const myEither = E.right(123) as E.Right<number>

const num1 = myEither.right // works

const dummy = <E, T>(x: E.Either<E, T>) => x
const v = dummy(myEither) // works, but turned into E.Either<unknown, number>

const dummy2 = <E=never, T=never>(x: E.Either<E, T>) => x
const v2 = dummy2(myEither) // works, v2 is still E.Either<never, number>

@mmkal
Copy link
Author

mmkal commented Nov 12, 2020

I'm not sure you meant it this way, but honestly that's exactly the sort of thing I'm worried about. as is pretty unsafe, and it's not great if the current design pushes users towards it:

const myEither = E.left(123) as E.Right<number>

const num1 = myEither.right.toString() // compiles without errors, but really shouldn't

Also, you're duplicating the type from the value in the cast. What if it's more complex than number? This technique gets messy:

const myEither = E.right({
  foo: {
    bar: {
      baz: [123]
    }
  }
}) as E.Right<{
  foo: {
    bar: {
      baz: number[]
    }
  }
}>

const num1 = myEither.right.foo.bar.baz[0]

To solve specifically for E.right and E.left use-cases, it might be worth turning @SRachamim's suggestion in #1343 (comment) into a separate issue. But this request is a bit more general.

@xuanduc987
Copy link

I tracked down the discussion for #823
Seems like the problem is when E.right and E.left is used in return position. If they return different type then typescript could unify them when used with something like O.fold

You are right that as is unsafe, and write out concrete type annotation is a chore, because typescript still doesn't support partial specify generic (microsoft/TypeScript#26242).

As an alternative non production environment, you could define and use assertion like bellow:

function assertRight<E, A>(e: E.Either<E, A>): asserts e is E.Right<A> {
  if (E.isLeft(e)) throw new Error('Value must be right')
}

const myEither = E.right(123)
assertRight(myEither)
const num1 = myEither.right

@mohaalak
Copy link
Contributor

I want to show you an example of ADT that make sense.

const absurd = (a: never) => {}

type Rectangle = {tag: "Rectangle", width: number, height: number }
type Circle = {tag: "Circle", radius: number }
type Square = {tag: "Square", side: number }

type Shape = Rectangle | Circle | Square

function area(shape: Shape) {
   switch(shape.type) {
     case "Rectangle":
       return shape.width * shape.height;
     case "Circle":
       return shape.raidus ** 2 * Math.PI;
     case "Square":
        return shape.side ** 2; 
   }

   absurd(shape);
}

in this example, typescript is helping you out in each case you know exactly what properties are available to you.
do you add all other properties as optional to each shape? cause you want to get radius for example from Rectangle?

let's say after 2 months you want to add Ellipse to your shape, do you add all it's properties to other shapes?

even in object oriented programming on inhtertance you don't add all properites of subclass to its parent.

@DenisFrezzato
Copy link
Collaborator

@mohaalak Offtopic, but I couldn't help myself: if you add the return type to area, you don't need that absurd hack.

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

No branches or pull requests

7 participants