[TBD] Typing improvements

[someniatko]

• added PHP return types to the methods (min supported PHP version is 7.0, which supports them already);
• improved Psalm templates: added covariance support (mostly due to need of using never type for Success and Error cases), fixed errors (flatMap() returned Option<NewSuccess,NewError> but should return Option<NewSuccess,OldError|NewError>), made template names more human-friendly instead of single letters like T, F etc;
• removed redundant comments in the subtypes;
• added running Psalm to the GitHub Actions pipeline;
• added psalm/phar as a dev dependency.

It would not pass the Psalm check yet, because due to covariance and dependency on the Option class, the latter must be marked immutable and @template-covariant as well. So, the Option package must be updated too. However, it will be tricky, or even not possible, because of the LazyOption addition. I think, the LazyOption support should be dropped, because it is not immutable by any means, which breaks "functional style" of the library a bit, and also prevents typing it correctly in Psalm. Or it should wrap the existing Option somehow, without actually being an Option, dunno.

I also plan to propose adding get() method if this is merged. The get() method would return whatever value the Result holds, whether it is Success or Error. This is useful if mapping different cases (i.e. Success or Error) to values of the same type, e.g. HTTP Responses.

If merged, I guess this should go into a brand new 2.x version branch, due to a BC break.

[GrahamCampbell]

Thanks for the PR. If you don't add the return types to the abstract class, then there are no BC breaks here?

[someniatko]

If specifying the result type: For the Result class' callers it's not a BC, because it only shrinks the set of possible return values (from "mixed" to Result). For the class' extenders it is, they now have to specify the return types in their methods, but does anyone extend the Result class anyway? So for majority if not all library users it's not a BC break.

If not specifying it and keeping only docblocks: From PHP POV it's not a BC break, however Psalm annotations changed and static analysis may fail in different ways than it did before (however, assuming annotations were sorta broken anyway, it's not a big deal).

[GrahamCampbell]

however Psalm annotations changed and static analysis may fail in different ways than it did before (however, assuming annotations were sorta broken anyway, it's not a big deal).

Yeh. I'd be inclined to say that we're treating this as a bug fix. :)

[someniatko]

@GrahamCampbell What do we decide next? Personally, while understanding that this is a technical BC break and stuff, firstly no one would have any rational reason to extend from those classes, and secondly — not having return type declarations seems odd in 2021. WDYT?

[someniatko]

@GrahamCampbell ping?

[simPod]

@GrahamCampbell is there anything that needs to be done?