vimeo / psalm

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

ImpureVariable - cannot reference $this in a pure context #7424

Open ptomulik opened 2 years ago

ptomulik commented 2 years ago

The example defines two methods, which are IMHO identical. Surprisingly, one method generates error ImpureFunctionCall which is related to $this being used by callback. Marking the callback as @psalm-pure changes the error type from ImpureFunctionCall to ImpureVariable.

https://psalm.dev/r/cec6529dc8

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/cec6529dc8 ```php $array */ public function f(array $array): array { return array_map(function (C $c): C { return $c->s($this); }, $array); } /** * @psalm-param array $array */ public function g(array $array): array { $me = $this; return array_map(function (C $c) use ($me): C { return $c->s($me); }, $array); } } ``` ``` Psalm output (using commit cb976f8): ERROR: ImpureFunctionCall - 14:16 - Cannot call an impure function from a mutation-free context ```
orklah commented 2 years ago

yeah, this should probably be flagged as ImpureVariable indeed

ptomulik commented 2 years ago

yeah, this should probably be flagged as ImpureVariable indeed

I wonder why decission was made define $this being an impure variable? Shouldn't we treat $this as an additional argument passed (implicitly) to non-static methods? I believe $this in PHP is not a part of global state, is not global variable. Rather we should think of a method as of a function which always takes $this as one of its argument, and this function should be analyzed to decide whether it's pure or not.

orklah commented 2 years ago

You may be right, I thought "pure" instead of "immutable"

ptomulik commented 2 years ago

What I'm trying to say, is that this should pass without the impureVariable error:

https://psalm.dev/docs/running_psalm/issues/ImpureVariable/

orklah commented 2 years ago

Well, $this can change between calls, so it makes sense to refuse any behaviour using $this, unless we can be sure $this is immutable itself

ptomulik commented 2 years ago

Well, $this can change between calls, so it makes sense to refuse any behaviour using $this, unless we can be sure $this is immutable itself

Any other argument can change as well, so why $this is so special? How $this is different from $that in the following snippet?

https://psalm.dev/r/563751c4eb

psalm-github-bot[bot] commented 2 years ago

I found these snippets:

https://psalm.dev/r/563751c4eb ```php f()->n . "\n"); echo($a->g($b)->n . "\n"); // changed between calls $b->n = 21; echo($a->g($b)->n . "\n"); ``` ``` Psalm output (using commit cb976f8): No issues! ```
ptomulik commented 2 years ago

What I want to say, it that I think that any non static method:

class C {
  public function foo(...$args) {
  }
}

should be analyzed as it was

class C {
  public static function foo(self $this, ...$args) {
  }
}

The notation $this->foo(...) is only a syntactic sugar, and its semantics is like foo($this, ...).

AndrolGenhald commented 2 years ago

@ptomulik That's the difference between @psalm-pure and @psalm-mutation-free. It's a bit confusing right now, partially because the error messages don't distinguish between the two.

It looks like the real issue here is that for methods we have @psalm-pure which means the output is solely dependent on the arguments and not $this, and @psalm-mutation-free which means the output is dependent on arguments and $this, but psalm can't figure out that array_map is mutation-free. We don't have a way to distinguish between mutation-free-callable and pure-callable.

I tentatively agree that @psalm-mutation-free seems redundant with @psalm-pure, if you want a method to not depend on $this just make it static... but we still have the problem that psalm has to somehow figure out that the array_map callback isn't pure (it uses a variable from the method scope), but the method is still pure despite that because the callback doesn't use any variables from a scope higher than the method scope.

AndrolGenhald commented 2 years ago

@psalm-github-bot looks like you break if I edit a comment to remove snippets.