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

@require-extends not recognized for @throws #10475

Closed
RobertMe opened this issue Jan 24, 2024 · 13 comments
Closed

@require-extends not recognized for @throws #10475

RobertMe opened this issue Jan 24, 2024 · 13 comments
Labels
Milestone

Comments

@RobertMe
Copy link

Bug report

Personally I would expect that if I have an interface with @phpstan-require-extends \Exception that I can use @throws <this interface> and it will work fine. This as it is known that every implementation of the interface will extend \Exception and thus is a throwable. But this isn't the case, and an error is reported that " is not subtype of Throwable"

Code snippet that reproduces the problem

https://phpstan.org/r/fe5464f1-7d2e-4570-a61f-c86b0a82b669

Expected output

No error

Did PHPStan help you today? Did it make you happy in any way?

No response

@RobertMe
Copy link
Author

Hmm, actually the interface can (in addition) extend Throwable, in which case the error is gone as well.

But then I would still expect that the information added by the @require-extends annotation would be taken into account when analyzing the @throws.

@ondrejmirtes
Copy link
Member

Yes, this should be supported, but you should extends \Throwable in the interface anyway :)

@ondrejmirtes ondrejmirtes added this to the Easy fixes milestone Jan 24, 2024
@RobertMe
Copy link
Author

but you should extends \Throwable in the interface anyway :)

Yes, 100% agreed. When finding out I could just extend \Throwable afterwards I felt kinda stupid to create this issue in the first place. But it does hold some value, but at the same time would obviously make it low priority, as I think in most cases extending such interface from \Throwable would work, unless you really desire some very specific class to be extended.

@RobertMe
Copy link
Author

@ondrejmirtes I might wanna try to fix this, depending on how easy the "easy fix" is :).

Am I correct in that this is something which possibly PHPDocNodeResolver::resolveThrowsTags() should handle? By getting the (reflection?) class of the given type (in the @throws tag)? Getting the PHPDoc node on that type? And then using PHPDocNodeResolver::resolveRequireExtendsTags()? And adding the resulting types to the list of types (of the throws tag). So that in the example the @throws is "resolved" as ExceptionInterface|Exception, instead of just ExceptionInterface? Or just wondering, possibly not adding it as an additional entry to the $types array, but modifying the $type into an intersection, so ExceptionInterface&Exception, meaning that if you define multiple @throws tags, for example with an @throws DomainException included, it resolves to DomainException|(ExceptionInterface&Exception) (instead of DomainException|ExceptionInterface|Exception)

Or is this possibly even something which could / should be handled by a TypeNodeResolver implementation? So that TypeNodeResolver::resolve() creates such a union type?

@ondrejmirtes
Copy link
Member

No, this is just something the respective rule that reports this problem should handle.

@RobertMe
Copy link
Author

Which means iterating over the type(s) resolved by resolveThrowsTags and then still getting the reflection class => PHPDocNode => resolveRequireExtendsTags() (per type) and checking whether any of those is a subclass of \Throwable? Where currently there just is a simple "is the resolved type a subclass of \Throwable check.

@ondrejmirtes
Copy link
Member

ClassReflection has a method to get @phpstan-require-extends tags with already resolved type.

@RobertMe
Copy link
Author

Thanks. Then I'll try to fix this over the weekend 👍🏻

@RobertMe
Copy link
Author

So I tried to get this working. But an issue I'm running into is when the declared @throws is a "complex" type (union or intersection). This as in that case the contained types needs to be checked, whether it has @require-extends or @require-implements which is a throwable (and such a type can be a union or intersection by itself I guess, so would need some form of recursion).

So I presume this results in some specific handling where it needs to be checked if the @throws type is a union and run some custom code or the type is an intersection and run some custom code? Which would "mimic / reuse" the the isSubTypeOf() implementation of these types, but taking the @require-* annotations into account? So in case of a union all types in the union must one way or another be a Throwable (either by implementing, or by @require-* implementing / extending), and in case of an intersection any of the types in it must be a Throwable.

Or another solution might be to recreate a type by replacing any type with an intersection of the named type combined with the types set in the @require-* annotation. Such that in the "try" example the @throws ExceptionInterface is not tested by ExceptionInterface as named in the PHPDoc, but is tested by ExceptionInterface&Exception. And in case of @throws LogicException|ExceptionInterface it would thus be transformed into LogicException|(ExceptionInterface&Exception), and after such a "transformation" the existing ->isSuperTypeOf check will still work.

What do you think about this? Is either of these directions correct? And which do you prefer then? Or is there some other solution I should choose for this?

@ondrejmirtes
Copy link
Member

I don't know, just submit what you have as a PR and also add some tests that are failing.

@RobertMe
Copy link
Author

Done, as phpstan/phpstan-src#2890

@ondrejmirtes
Copy link
Member

Implemented phpstan/phpstan-src#2890

Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants