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

Support (partial) inheritance of @throws annotations #4757

Closed
Daimona opened this issue Jan 22, 2023 · 1 comment · Fixed by #4773
Closed

Support (partial) inheritance of @throws annotations #4757

Daimona opened this issue Jan 22, 2023 · 1 comment · Fixed by #4773

Comments

@Daimona
Copy link
Member

Daimona commented Jan 22, 2023

If a method's doc comment has a @throws annotation and the method is overridden in a subclass, phan doesn't inherit the annotation from the parent class/interface/trait. This can be observed by setting warn_about_undocumented_throw_statements to true in the config, and running phan on the following snippet:

<?php

class BaseClass {
    /**
     * @throws RuntimeException
     */
    public function foo() {
        throw new RuntimeException( 'A' );
    }
}

class ChildClass extends BaseClass {
    public function foo() {
        throw new RuntimeException( 'B' );
    }
}

for which phan will output:

PhanThrowTypeAbsent \ChildClass::foo() can throw new RuntimeException('B') of type \RuntimeException here, but has no '@throws' declarations for that class

Instead, I think if phan sees a @throws annotation for a method, any override of that method could also be expected to throw the same exceptions. This would be similar to how inherit_phpdoc_types, and I assume the same setting could control whether @throws annotations are inherited.


However, I think that phan should NOT assume that the method will possibly throw the exception. I can imagine there might be cases when a method can throw an exception, but an override of that method guarantees not to throw any exceptions. As far as I can see, phan does not have an issue type for @throws annotations on methods that don't actually throw those exceptions (because I guess it might be hard to prove statically). However, the warn_about_undocumented_exceptions_thrown_by_invoked_functions setting can be used to enforce @throws or catch in methods calling a method that can throw. In this case, the following code should not raise any warnings:

<?php

class BaseClass {
    /**
     * @throws RuntimeException
     */
    public function foo() {
        throw new RuntimeException( 'A' );
    }
}

class ChildClass extends BaseClass {
    public function foo() {
        echo "this method definitely does not throw any exceptions";// Phan should NOT complain about this method not throwing
    }
}

function doTest() {
    $c = new ChildClass();
    $c->foo();// Phan should NOT emit PhanThrowTypeAbsentForCall here
}

I think this might be achieved by keeping two separate lists for @throws that were found in the method's own docblock, and for the ones inherited from an ancestor.

@TysonAndre
Copy link
Member

As far as I can see, phan does not have an issue type for @throws annotations on methods that don't actually throw those exceptions (because I guess it might be hard to prove statically).

Yes, that and the fact that user @throws may be API documentation for subclasses, or user code being incomplete.

I think this might be achieved by keeping two separate lists for @throws that were found in the method's own docblock, and for the ones inherited from an ancestor.

A non-standard @throws never might work, though I think a class name is usually expected.

Two separate lists makes sense for analyzing the implementers and callers differently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants