-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Comments
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();
}
https://psalm.dev/r/e3257c26e9<?php
/**
* @template T
* @template E of Throwable = Throwable
*/
interface ResultInterface {
/**
* @return T
*
* @throws E
*/
public function getResult();
}
|
ResultInterface<T>
to ResultInterface<T, E>`ResultInterface<T>
to ResultInterface<T, E>
@veewee do you think this can make it to 3.0? |
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 |
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: /**
* @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 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 Conclusion @devnix Feel free to add to the list here - I might have forgotten a few details. |
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;
}
|
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:
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:Psalm would only throw a
MissingTemplateParalm
. At the same time, PHPStan would report two errors. Still, the return would be implicitly inferred fromResultInterface<int> to ResultInterface<Throwable>
in thenot_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:UndefinedDocblockClass
throws
annotation vimeo/psalm#6098E
at@throws
The text was updated successfully, but these errors were encountered: