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

Result is too restrictive, introduce Either type? #355

Open
someniatko opened this issue Jun 28, 2022 · 22 comments
Open

Result is too restrictive, introduce Either type? #355

someniatko opened this issue Jun 28, 2022 · 22 comments
Assignees
Labels
Priority: Medium This issue may be useful, and needs some attention. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Type: Enhancement Most issues will probably ask for additions or changes.

Comments

@someniatko
Copy link

someniatko commented Jun 28, 2022

I want to type a function/method representing action which may fail, but not from pure technical perspective, rather from the business logic one. For instance, "register new user" action may succeed, or may fail for reasons like "username is already taken" or "password is too short" etc. I don't want to split this action into two steps, a validation method and then the actual action method, because I don't like creating an intermediate state which is invalid (however others may want to do it in two steps — it's just style preference, I made this choice).

This lib has a Result type which looks like it's exactly what I need, but it's unfortunately not: its Failure branch is limited only to exceptions (more correctly, throwables). However this is not how I view the business logic: for me exceptions are, well, exceptional situations: e.g. "database connection failed" or "file could not be read", "remote API returned HTTP 500" etc. Purely technical ones. Or could be business-logic related, but unexpected, e.g. some entity's invariant was violated due to some logic error in the code, or maybe someone edited its state directly in the database etc. This is the other thing though, i don't want any exception-related fluff here: traces, codes etc. Such action "failures" are no more than just normal operation of my system.

Describe the solution you'd like
Ideally I'd like to extend Result failure type to accept not only Exceptions. This is not possible due to possible backwards-compatibility break. Instead, a new type can be introduced: Either, with Left and Right variants as inspired by Haskell.

Describe alternatives you've considered
I have my own library for this, https://github.com/someniatko/php-result-type, which was inspired by this one: https://github.com/GrahamCampbell/Result-Type, but it has more robust Psalm typing.

@someniatko someniatko added the Type: Enhancement Most issues will probably ask for additions or changes. label Jun 28, 2022
@azjezz
Copy link
Owner

azjezz commented Jun 28, 2022

I'm okay with adding this feature to PSL, however, i don't think i have the time to implement this myself, PRs are welcome :)

@azjezz azjezz added Priority: Medium This issue may be useful, and needs some attention. Status: Available No one has claimed responsibility for resolving this issue. labels Jun 28, 2022
@veewee
Copy link
Collaborator

veewee commented Jun 29, 2022

You might find some inspiration in this version as well
https://github.com/marcosh/lamphpda/blob/master/src/Either.php

@someniatko
Copy link
Author

@veewee Thank you! This lib truly is a gem, however it has built some sort of "functional infrastructure" with Functor, Monad typeclasses etc. I am not sure we can and should recreate something like that in PSL.

@azjezz
Copy link
Owner

azjezz commented Jun 30, 2022

personally, I imagine the Either API in PSL to look like this:

final class Either<Tr, Tl> {
   // static factories 
  public static function right<T>(T $val): Either<T, _>;
  public static function left<T>(T $val): Either<_, T>;

  // Throw MissingValueException if not right 
  public function getRight(): Tr;
  // Same
  public function getLeft(): Tl;

  public function isRight(): bool;
  public function isLeft(): bool;

  // could be useful
  public function wrapRight(): Result<Tr, MissingValueException>;
  public function wrapLeft(): Result<Tl, MissingValueException>;
}

@someniatko
Copy link
Author

someniatko commented Jun 30, 2022

IMO to complete the API it would be great to have at least those:

/** @var callable(Tr):TrNew $map */
public function mapRight(callable $map): Either<Tr|TrNew, Tl>;

/** @var callable(Tl):TlNew $map */
public function mapLeft(callable $map): Either<Tr, Tl|TlNew>;

public function getEither(): Tr|Tl;

BTW, by having getEither() IDK if we need separate getRight() and getLeft(), which may throw exception.

@someniatko
Copy link
Author

someniatko commented Jun 30, 2022

/** @var callable(Tr):Either<TrNew, TlNew> $map */
public function flatMapRight(callable $map): Either<Tr|TrNew, Tl|TlNew>;

/** @var callable(Tl):Either<TrNew, TlNew> $map */
public function flatMapLeft(callable $map): Either<Tr|TrNew, Tl|TlNew>;

may also be desirable

@veewee
Copy link
Collaborator

veewee commented Jun 30, 2022

Maybe also a proceed($onLeft, $onRight): T similar to the result class?

@azjezz
Copy link
Owner

azjezz commented Jul 1, 2022

BTW, by having getEither() IDK if we need separate getRight() and getLeft(), which may throw exception.

I think we should, getEither would return Tr|Tl, when isRight returns true, i know that getEither would return Tr, but psalm/phpstan would assume it's Tr|Tl, having getRight() allows me to use it, knowing it won't throw since isRight() returned true.

@someniatko
Copy link
Author

someniatko commented Jul 1, 2022

The whole point of Either is that you don't know which of the two outcomes will you get beforehand. You can still process its individual "branches" using mapLeft() / mapRight()
or proceed() as @veewee suggests

@azjezz
Copy link
Owner

azjezz commented Jul 1, 2022

I'm in favor of proceed($fr, $fl) to keep consistency, however, getEither IMO, shouldn't not be a thing, Either<Tr, Tl> represents either one of those two types in question ( Tr and Tl ), if you want to work on both, you would use proceed<Tr2, Tl2>((Closure(Tr): Tr2) $fr, (Closure(Tl): Tl2) $fl): Either<Tr2, Tl2> is what you want to use, which would return another Either instance, if you want to map one of the branches, you would use mapRight<Tr2>((Closure(Tr): Tr2) $fr): Either<Tr2, Tl> ( same for mapLeft<Tl2>((Closure(Tl): Tl2) $fl): Either<Tr, Tl2> ), if you want to unwrap the value, in most cases, you would want to know which value you are getting, so what i would suggest instead of getRight/getLeft/wrapRight/wrapLeft is:

public function unwrap(): Tr|Tl;

public function unwrapRight(): Tr; // throws otherwise
public function unwrapLeft(): Tr; // throws otherwise

public function unwrapRightOr<T>(T $value): Tr|T; // returns `$value` otherwise
public function unwrapLeftOr<T>(T $value): Tr|T; // returns `$value` otherwise

@azjezz
Copy link
Owner

azjezz commented Jul 1, 2022

rust either type has many other methods that we could also implement ( see: https://docs.rs/either/latest/either/enum.Either.html#method.left_or_else )

public function getRight(): Tr; // throws otherwise
public function getLeft(): Tr; // throws otherwise

public function getRightOr<T>(T $value): Tr|T; // returns `$value` otherwise
public function getLeftOr<T>(T $value): Tl|T; // returns `$value` otherwise

public function getRightOrThen<T>((Closure(): T) $f): Tr|T; // returns `$f` results otherwise
public function getLeftOrThen<T>((Closure(): T) $f): Tl|T; // returns `$f` results otherwise

public function getRightOrElse((Closure(Tl): Tr) $f): Tr; // returns `$f` results otherwise
public function getLeftOrElse((Closure(Tr): Tl) $f): Tl; // returns `$f` results otherwise

public function map<Tm>((Closure(Tr|Tl): Tm) $f): Either<Tm, Tm>;
public function mapRight<Tr2>((Closure(Tr): Tr2) $f): Either<Tr2, Tl>;
public function mapLeft<Tl2>((Closure(Tl): Tl2) $f): Either<Tr, Tl2>;

( note: all methods prefixed with unwrap in rust either type return Option<T> which we don't have here, returning ?T is not an option as null could be a valid T value. )

@azjezz
Copy link
Owner

azjezz commented Jul 1, 2022

example ( stupid, and you shouldn't use Either in this case, but it gives you the idea 😛 ) :

function get_organization_owner(Either<Organization, Project> $either): Owner
{
  return $either->getLeftOrElse(
    fn(Project $project): Organization => $project->getOrganization()
  )->getOwner();
}

$owner = get_organization_owner(Either::left($organization));
$owner = get_organization_owner(Either::right($project));

@azjezz
Copy link
Owner

azjezz commented Jul 1, 2022

@someniatko wdyt?

@someniatko
Copy link
Author

@azjezz it totally makes sense to me with those three unwrap functions you've suggested. Basically, unwrapRight(), unwrapLeft() and unwrap() are just renamed getLeft(), getRight() and getEither() from the previous comments. And in Haskell there is either() function which is basically in our terms proceed() + unwrap().

@azjezz
Copy link
Owner

azjezz commented Jul 1, 2022

what do you think about the other ones ( *Or, *OrElse, *OrThen )?

@someniatko
Copy link
Author

Hmm, I looked a bit more closely, the proceed() method of the Result actually also unwraps it as well. So if we want to do it consistently, proceed() must also unwrap. I see it as a convenience method over mapLeft() + mapRight() + unwrap(), however maybe there is some deeper functional sense in it, IDK.

@someniatko
Copy link
Author

someniatko commented Jul 1, 2022

As to unwrapLeft() / unwrapRight() functions which in Rust return an Option, why then not implementing Option first?

@someniatko
Copy link
Author

someniatko commented Jul 1, 2022

If we implement an Option and return it for the unwrapX() methods, then we don't actually have to use those or / orThen methods on Either itself, we could just have them on the Option:

$either
    ->unwrapRight()
    ->orThen(fn() => computeSomething());

I am not sure about orElse though, thinking.
It's basically some sugar for proceed($fr, $fl) without providing one of the callables, right?

@someniatko
Copy link
Author

someniatko commented Jul 1, 2022

By the way, if proceed will also extract the value as either() in Rust and Haskell, then, I guess, separate unwrap() (getEither() as I suggested earlier) method is indeed not needed.

Just summarizing the discussion, the full API for could look like this:

// static factories 
public static function right<T>(T $val): Either<T, _>;
public static function left<T>(T $val): Either<_, T>;

public function isRight(): bool;
public function isLeft(): bool;

public function map<Tm>((Closure(Tr|Tl): Tm) $f): Either<Tm, Tm>;
public function mapRight<Tr2>((Closure(Tr): Tr2) $f): Either<Tr2, Tl>;
public function mapLeft<Tl2>((Closure(Tl): Tl2) $f): Either<Tr, Tl2>;

public function flatMap<Tmr,Tml>((Closure(Tr|Tl): Either<Tmr,Tml>) $f): Either<Tmr, Tml>;
public function flatMapRight<Tmr,Tml>((Closure(Tr): Either<Tmr,Tml>) $f): Either<Tr|Tmr, Tl|Tml>;
public function flatMapLeft<Tmr,Tml>((Closure(Tl): Either<Tmr,Tml>) $f): Either<Tr|Tmr, Tl|Tml>;

public function proceed<Tm>((Closure(Tr): Tm) $fr, (Closure(Tl): Tm) $fl): Tm;

public function unwrapRight(): Option<Tr>;
public function unwrapLeft(): Option<Tl>;

// could be useful
public function wrapRight(): Result<Tr, MissingValueException>;
public function wrapLeft(): Result<Tl, MissingValueException>;

@azjezz
Copy link
Owner

azjezz commented Jul 2, 2022

@someniatko Option implemented in #356, would appreciate your review :)

@azjezz azjezz added this to the 3.0.0 milestone Dec 19, 2022
@azjezz azjezz added Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. and removed Status: Available No one has claimed responsibility for resolving this issue. labels Dec 19, 2022
@veewee veewee removed this from the 2.4.0 milestone May 17, 2023
@simPod
Copy link
Contributor

simPod commented Apr 29, 2024

Is someone planning to work on this and has the final interface been decided?

@veewee
Copy link
Collaborator

veewee commented Apr 29, 2024

I dont think anyone is planning on working on this at the moment, so feel free to pick it up.
About the interface, you could start with the one described in #355 (comment) maybe? It can always be extended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium This issue may be useful, and needs some attention. Status: Accepted It's clear what the subject of the issue is about, and what the resolution should be. Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

No branches or pull requests

4 participants