Only consider class-strings when resolving dependencies#239
Only consider class-strings when resolving dependencies#239rainbow-alex wants to merge 2 commits intophpstan:masterfrom
Conversation
| } elseif ($node instanceof Array_) { | ||
| } elseif ( | ||
| $node instanceof Array_ | ||
| && $this->considerArrayForCallableTest($node) |
There was a problem hiding this comment.
I'm not too sure about this check or the implementation. Feels hacky to start excluding specific syntax at this point, no?
There was a problem hiding this comment.
Maybe we should exclude String_ instead of just allowing ::class. This will also exclude examples like [$this, 'foo']. But I'm fine with this hack as long as it serves it purpose.
There was a problem hiding this comment.
@ondrejmirtes I was implementing your suggestion and I don't think it will fix the issue in our case, since there's the indirection via self::.
class Foobar
{
self::FOO = 'foo';
self::FOO = 'bar';
self::FOOBAR = [self::FOO, self::BAR];
}
The only way to fix it by looking at the AST that I can see is to only allow ::class, which I agree is too strict.
edit: tested it and it doesn't fix the issue for us :(
|
Just tried something: b537b97 Please test phpstan/phpstan dev-master in 10-15 minutes. Thank you. |
|
I tried it and I no longer get any errors on master. Strangely enough if I comment out your fix I am not getting errors either. I'll try to look into this in depth later today. |
|
@ondrejmirtes When I create a minimal case I can confirm that your change fixes the issue. I suspect I am not seeing errors on master without your change because something caused the order of files to change again. But as far as I can see the fix is good! Thank you! |
@ondrejmirtes here is the change you suggested in #236. Tests seemed to be ok when I ran them locally with #238 included.