vimeo / psalm

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

TaintAnalysis: Tags of interface take precedence over class declaration #9836

Open ohader opened 1 year ago

ohader commented 1 year ago

https://psalm.dev/r/818815f379

This seemed to be working with vimeo/psalm:~5.8.0, but change somehow on the way to ~5.9.0. https://github.com/vimeo/psalm/compare/5.8.0...5.9.0git bisect points to commit dfd7ffc4598e03db1e15bac82908b36ddbe51455

Does not work (~5.9.0, tested with 5.x-dev 2bbfca6)

class HandlerImplementation implements HandlerInterface
{
    /**
     * @psalm-taint-specialize
     * @psalm-taint-sink sql $value
     */
    public function execute(string $value, array $params = []): static {}
}

Removing implements HandlerInterface works in ~5.9.0

class HandlerImplementation
{
    /**
     * @psalm-taint-specialize
     * @psalm-taint-sink sql $value
     */
    public function execute(string $value, array $params = []): static {}
}
psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/818815f379 ```php execute($_GET['inject']); ``` ``` Psalm output (using commit 2bbfca6): No issues! ```
orklah commented 1 year ago

Oh, I see what happened.

Before my change in https://github.com/vimeo/psalm/pull/9562, the type returned by resolveHandler() was HandlerInterface|HandlerImplementation

HandlerImplementation being a subtype of HandlerInterface, it's now naturally combined as HandlerInterface

So there's two way to see the issue here:

I think the second option is the best IMHO so I'll flag this as enhancement.