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

[RFC] Change ResultInterface<T> to ResultInterface<T, E> #440

Open
devnix opened this issue Jan 22, 2024 · 5 comments
Open

[RFC] Change ResultInterface<T> to ResultInterface<T, E> #440

devnix opened this issue Jan 22, 2024 · 5 comments
Assignees
Labels
Priority: Low This issue can probably be picked up by anyone looking to contribute to the project, as an entry fix Type: Enhancement Most issues will probably ask for additions or changes.

Comments

@devnix
Copy link
Sponsor Contributor

devnix commented Jan 22, 2024

I've been chatting with @veewee on Discord about adding a template to ResultInterface to return what kind of error it could contain.

I'm not sure if this could be considered a breaking change, as it only affects static analysis and can be easily ignored, baselined, or fixed in a moment by going to each error and changing every @return ResultInterface<Foo> to @return ResultInterface<Foo, Throwable> if you don't feel like studying the specific throwables you want to return from there.

Another option apart from waiting for a new major version or just letting static analysis break is to wait for some default generic type syntax, see:

This could allow us to have a syntax that would behave the same way but without having any new errors:

/**
 * @template T
 * @template E of Throwable = Throwable
 */
interface ResultInterface {}

If we are OK with baselining errors (regardless of whether the change is in a minor or major version), the following syntax would infer the template with Throwable as the default value:

/**
 * @template T
 * @template E of Throwable
 */
interface ResultInterface {}

Psalm would only throw a MissingTemplateParalm. At the same time, PHPStan would report two errors. Still, the return would be implicitly inferred from ResultInterface<int> to ResultInterface<Throwable> in the not_documented() case on both major static analysis tools.


Apart from this, there is another feature that would be nice to have in the static analysis, and that is to be able to infer the type of @throws with a generic, so we could type this:

/**
 * @template T
 * @template E of Throwable = Throwable
 */
interface ResultInterface {
    /**
     * @return T
     * 
     * @throws E
     */
    public function getResult();
}
@devnix devnix added the Type: Enhancement Most issues will probably ask for additions or changes. label Jan 22, 2024
Copy link

I found these snippets:

https://psalm.dev/r/891520eca6
<?php

/**
 * @template T
 * @template E of Throwable
 */
interface ResultInterface 
{
    /**
     * @return E
     */
    public function getThrowable();
}

/** 
 * @template E of Throwable
 * @implements ResultInterface<never, E>
 */
class Failure implements ResultInterface
{
    /**
     * @param E $throwable
     */
    public function __construct(
        private readonly Throwable $throwable
    ) {
    }
    
    public function getThrowable()
    {
        return $this->throwable;
    }
}

/**
 * @return ResultInterface<int, RuntimeException>
 */
function documented()
{
    return new Failure(new RuntimeException());
}

/**
 * @return ResultInterface<int>
 */
function not_documented()
{
    return new Failure(new RuntimeException());
}

function test_documented(): RuntimeException
{
    return documented()->getThrowable();
}

function test_not_documented(): Throwable
{
    return not_documented()->getThrowable();
}
Psalm output (using commit 3c90054):

ERROR: MissingTemplateParam - 44:12 - ResultInterface has missing template params, expecting 2
https://psalm.dev/r/e3257c26e9
<?php

/**
 * @template T
 * @template E of Throwable = Throwable
 */
interface ResultInterface {
    /**
     * @return T
     * 
     * @throws E
     */
    public function getResult();
}
Psalm output (using commit 3c90054):

ERROR: UndefinedDocblockClass - 11:16 - Docblock-defined class, interface or enum named E does not exist

@devnix devnix changed the title [RFC] Change ResultInterface<T> to ResultInterface<T, E>` [RFC] Change ResultInterface<T> to ResultInterface<T, E> Jan 22, 2024
@veewee veewee added the Priority: Low This issue can probably be picked up by anyone looking to contribute to the project, as an entry fix label Jan 22, 2024
@azjezz
Copy link
Owner

azjezz commented Mar 24, 2024

@veewee do you think this can make it to 3.0?

@veewee
Copy link
Collaborator

veewee commented Mar 24, 2024

I've had an offline discussion about this and there were some good arguments in favour and against this. Will add the main discussion points to this ticket later so that we can conclude here

@veewee
Copy link
Collaborator

veewee commented Mar 25, 2024

These were the main reasons that were holding back on implementing it:

There is no way in psalm (nor phpstan) to throw a generic exception:
https://psalm.dev/r/b3988674a5

/**
 * @template T of \Throwable
 * @param T $e
 * @throws T
 */
function throwIt(\Throwable $e): never {
    throw $e;
}

There is no way in psalm to set a default generic value

In a lot of situations, you are using something like Result\wrap() or similar in which you don't know what type of exception will be thrown. This will therefor be defaulted to \Throwable, making people have to add the type \Throwable everywhere where they use Result. It doesn't add any value for the user to add this type - just more work for no extra gain. This could be solved if we had generics with default types.

But there is no such thing like:

/**
 * @template E of \Throwable = \Throwable
 */

That allows you to overwrite defaults.

Either

Most of the problems that the additional <E> would solve, could be better dealt with when introducing an Eiter<L, R> type as described in #355.
There the types would be required and there won't be a throw. So you could use one of them as the specific exception.

Conclusion
It is possible to add an additional <E> param. But it wouldn't add a lot of value, if any at current moment.

@devnix Feel free to add to the list here - I might have forgotten a few details.

Copy link

I found these snippets:

https://psalm.dev/r/b3988674a5
<?php

/**
 * @template T of \Throwable
 * @param T $e
 * @throws T
 */
function throwIt(\Throwable $e): never {
    throw $e;
}
Psalm output (using commit ef3b018):

ERROR: UndefinedDocblockClass - 6:12 - Docblock-defined class, interface or enum named T does not exist

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low This issue can probably be picked up by anyone looking to contribute to the project, as an entry fix Type: Enhancement Most issues will probably ask for additions or changes.
Projects
None yet
Development

No branches or pull requests

3 participants