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

PHP native decorators are not recognized #3228

Closed
ktomk opened this issue Apr 25, 2020 · 6 comments
Closed

PHP native decorators are not recognized #3228

ktomk opened this issue Apr 25, 2020 · 6 comments
Labels

Comments

@ktomk
Copy link
Contributor

ktomk commented Apr 25, 2020

PHP IteratorIterator (and a couple of sub-types based on it, e.g. FilterIreator) are decorating the Traversable (the one constructor parameter).

However psalm detects an UndefinedMethod (see https://psalm.dev/022) albeit it is defined in the Traversable:

<?php

class Subject implements Iterator {
    /**
     * the index method exists
     *
     * @param int $index
     * @return bool
     */
    public function index($index) {
        return true;
    }

    public function current() {
        return 2;
    }

    public function next() {}

    public function key() {
        return 1;
    }

    public function valid() {
        return false;
    }

    public function rewind() {}
}

$iter = new IteratorIterator(new Subject());
var_dump($iter->index(0)); # bool(true)

What I would like to ask for (decorating support is maybe not so wrong as there might be something coming up with PHP 8), is that I can use @method to denote index() existing without relying on the psalm configuration it should be acceptable without __call but instead that the psalm utility is aware of the fact that the Traversable Subject is decorated by IteratorIterator.

I can work-around this by setting <psalm usePhpDocMethodsWithoutMagicCall="true"> as the concrete code already has the @method annotation (not part of the example above) but no __call implementation.

However if the @method annotation would not be in my (extend to the example given) case, the work-around would not work with psalm leaving the existing method detected as nonexistent (false-positive).

First of all I think it's ok to report the issue.

Additionally I'd like to ask if there is some (psalm) specific annotation I could make use of to alleviate next to @method and changing psalms' configuration.

(this is also something Phpstorm can't manage [yet], it just takes @method for granted but can somewhat deduce the concrete implementation as well. Scrutinizer needs an additional @scrutinizer ignore-call annotation at invocation-point)

@psalm-github-bot
Copy link

I found these snippets:

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

class Subject implements Iterator {
    /**
     * the index method exists
     *
     * @param int $index
     * @return bool
     */
    public function index($index) {
        return true;
    }

    public function current() {
        return 2;
    }

    public function next() {}

    public function key() {
        return 1;
    }

    public function valid() {
        return false;
    }

    public function rewind() {}
}

$iter = new IteratorIterator(new Subject());
var_dump($iter->index(0)); # bool(true)
Psalm output (using commit e1c6fcc):

ERROR: UndefinedMethod - 32:17 - Method IteratorIterator::index does not exist

ERROR: ForbiddenCode - 32:1 - Unsafe var_dump

@weirdan
Copy link
Collaborator

weirdan commented Apr 25, 2020

To sum up, IteratorIterator forwards unknown methods to the object it wraps, and Psalm should know it (but doesn't). Reproduced more succinctly and prominently: https://3v4l.org/vCAfm & https://psalm.dev/r/4146a0fcf0

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/4146a0fcf0
<?php

class Subject implements Iterator {
    public function current() { return 2; }
    public function next() {}
    public function key() { return 1; }
    public function valid() { return false; }
    public function rewind() {}
    /** @return string */
    public function blahBlahBlah() { return "blah"; }
}

$iter = new IteratorIterator(new Subject());
echo $iter->blahBlahBlah();
Psalm output (using commit e1c6fcc):

ERROR: UndefinedMethod - 14:13 - Method IteratorIterator::blahblahblah does not exist

@weirdan weirdan added the bug label Apr 25, 2020
@ktomk
Copy link
Contributor Author

ktomk commented Apr 25, 2020

@weirdan Yes, this is exactly it. And good correction on the examples index() and true were perhaps not making it immediately visible, sorry.

From what I know this is a special property of IteratorIterator and it makes sense (at least to humble me) having an iterator as a decorator (but I know it can inherit surprises [next to Traverable being a PHP internal interface], referencing Anthony Ferrara in Nov 2011 about IteratorIterator, IMHO wrongfully he was coining it as inconsistent, but I can understand the moment of realization as a WTF moment).

Probably decorators are easy to handle for psalm and the case of IteratorIterator was never brought up, as these are annotated in code most often (e.g. Subject|Decorator), for a native decorator, this most certainly is not the case (there is no PHP code to add annotations to that psalm could infer from).

@muglug
Copy link
Collaborator

muglug commented Apr 26, 2020

This (I think) calls for templated @mixin. I'll try to work on that today.

@muglug muglug closed this as completed in ebcb0b8 Apr 26, 2020
@ktomk
Copy link
Contributor Author

ktomk commented Apr 27, 2020

@muglug: I updated my case against the @dev version (currently at a701163) and get the same result. Do I need to make use of @mixin as well in my own codebase?

/E: Found it, @mixin works, counter-test with previous revision does also work. I guess b/c the FilterIterator in my concrete case passes the real subject to parent::__construct() within the FilterIterator::__construct() which has as first parameter not the subject (in terms of decoration).

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

No branches or pull requests

3 participants