vimeo / psalm

A static analysis tool for finding errors in PHP applications
https://psalm.dev
MIT License
5.56k stars 660 forks source link

PHP native decorators are not recognized #3228

Closed ktomk closed 4 years ago

ktomk commented 4 years ago

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[bot] commented 4 years ago

I found these snippets:

https://psalm.dev/r/a6afb77607 ```php 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 commented 4 years ago

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[bot] commented 4 years ago

I found these snippets:

https://psalm.dev/r/4146a0fcf0 ```php blahBlahBlah(); ``` ``` Psalm output (using commit e1c6fcc): ERROR: UndefinedMethod - 14:13 - Method IteratorIterator::blahblahblah does not exist ```
ktomk commented 4 years ago

@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 commented 4 years ago

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

ktomk commented 4 years ago

@muglug: I updated my case against the \@dev version (currently at a701163f0aecdc1db24cd3a334c85d4b918c4ff2) 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).