vimeo / psalm

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

@psalm-pure for classes? #9834

Open kkmuffme opened 1 year ago

kkmuffme commented 1 year ago

https://psalm.dev/r/b3282d3d68

Sometimes (e.g. utility classes) you want to enforce that it only contains pure methods, allowing @psalm-pure on the class (like @psalm-immutable) would be nice. Additionally this would declaring @psalm-pure on each method obsolete then (like @psalm-mutation-free isn't necessary on methods in @psalm-immutable)

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/b3282d3d68 ```php
orklah commented 1 year ago

That's something I never completely understood in Psalm.

Which symbols are allowed to be called pure/immutable/mutation-free/external-mutation-free, which of these denominations can be used in a symbol that has another denomination etc...

We miss a big documentation on that and it's a pain to work with.

I think if we wanted to add what you want, a @psalm-all-method-pure would maybe be more clear. I'm afraid adding @psalm-pure to a class would add to the confusion and make people think a class can actually be pure (which by the way, I have no idea if that could be a thing and what should be checked in order to enforce that...)

klimick commented 1 year ago

For me @psalm-immutable is pretty weird. When I met him as first time, I thought that he makes all the properties of the class as readonly. My surprise was indescribable when I realized that this annotation also applies to methods: https://psalm.dev/r/987df51c8b

Why is another annotation needed? We're going to fix @psalm-immutable?

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/987df51c8b ```php
kkmuffme commented 1 year ago

My surprise was indescribable

Same for me, when I realized this a few days ago. So basically @psalm-immutable is kind of like @psalm-pure for classes already anyway? Except that the methods aren't marked as pure leading to complete inconsistency?

klimick commented 1 year ago

Except that the methods aren't marked as pure leading to complete inconsistency?

I don't like it. This leads to strange edge cases: https://psalm.dev/r/933c984ea0

In my opinion, @psalm-immutable should be about only immutability. i.e. @psalm-immutable = @psalm-readonly for all properties.

/**
 * @psalm-immutable
 */
final class Foo
{
    public function __construct(
        public int $a,
        public string $b,
    ) {}
}
final class Foo
{
    public function __construct(
        /** @psalm-readonly */
        public int $a,
        /** @psalm-readonly */
        public string $b,
    ) {}
}
psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/933c984ea0 ```php $values */ public function __construct( private array $values, ) { } /** * @return ArrayCollection */ public function forEach(callable $callback): ArrayCollection { foreach ($this->values as $value) { // ImpureFunctionCall !?? $callback($value); } return $this; } } // UnusedMethodCall ??? (new ArrayCollection([1, 2, 3]))->forEach(fn(int $i) => print_r($i)); ``` ``` Psalm output (using commit a82e7fc): ERROR: UnusedMethodCall - 31:35 - The call to ArrayCollection::forEach is not used ERROR: ImpureFunctionCall - 23:13 - Cannot call an impure function from a mutation-free context ```
kkmuffme commented 1 year ago

In my opinion, @psalm-immutable should be about only immutability.

Exactly.

ygottschalk commented 1 year ago

I would recommend giving this blog post a read: https://psalm.dev/articles/immutability-and-beyond

In my opinion, @psalm-immutable should be about only immutability. i.e. @psalm-immutable = @psalm-readonly for all properties.

Let me try to change your minds by giving this example: https://psalm.dev/r/9612ecfbc7 Adding the @psalm-immutable annotation will catch the 'error'.

It is really hard to define what immutability and purity mean in an object oriented context.

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/9612ecfbc7 ```php neverChange->foo = 1; } } class DoOtherStuff { public function doStuff(Immutable $immutable): void { $immutable->mutate(); } } $data = new stdClass; $data->foo = 0; $immutable = new Immutable($data); /* ... */ if ($immutable->neverChange->foo === 0) { /* ... */ (new DoOtherStuff())->doStuff($immutable); /* Here I would assume, that my immutable object still holds the not change value stdClass::foo = 0 but it got mutated to stdClass::foo = 1 */ } ``` ``` Psalm output (using commit e15e03d): No issues! ```
klimick commented 1 year ago

Adding the @psalm-immutable annotation will catch the 'error'.

What 'error'? I don't quite understand. You have put a reference to the mutable data structure in to the immutable data structure. What are you expecting? It data structure can change at any time. But reference inside immutable data structure will ever point to the same object.

https://psalm.dev/r/c4e223238d

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/c4e223238d ```php foo = 123; $immutable->neverChange->bar = 321; ``` ``` Psalm output (using commit e15e03d): No issues! ```
ygottschalk commented 1 year ago

You have put a reference to the mutable data structure in to the immutable data structure. What are you expecting?

I expect an object which is called immutable to not change its state. For me, this includes all of the states of all inner objects (recursively), not just the data (in this case a reference / pointer to some other object) directly inside the immutable object.

So IMO the example you gave should throw some error related to the change of the inner state (...->bar = 321;) similar to how psalm handles this with arrays: https://psalm.dev/r/912c1c1cdf

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/912c1c1cdf ```php [0], 'bar' => [1]]); $immutable->neverChange['foo'] = [321]; ``` ``` Psalm output (using commit 4ebe4c1): ERROR: InaccessibleProperty - 15:1 - Immutable::$neverChange is marked readonly ```
klimick commented 1 year ago

I expect an object which is called immutable to not change its state. For me, this includes all of the states of all inner objects (recursively), not just the data (in this case a reference / pointer to some other object) directly inside the immutable object.

How? https://psalm.dev/r/6b6a329212 You can have many many other referenses to stdClass comes from anywhere. Where is guarantie that owner does not mutate stdClass that was passed to the immutable class?

The most reasonable thing we can do is prohibit to pass mutable type to the immutable type.

<?php

final class Mutable
{
    public function __construct(
        public string $value,
    ) {
    }
}

/**
 * @psalm-immutable
 */
final class Immutable
{
    public function __construct(
        // This should be an error!
        public Mutable $value,
    ) {
    }
}
psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/6b6a329212 ```php nesting = new Nesting3($obj); } public function mutate(): void { $this->obj->bar = 321; } } final class Nesting1 { public readonly Nesting2 $nesting; public function __construct(private readonly stdClass $obj) { $this->nesting = new Nesting2($obj); } public function mutate(): void { $this->obj->foo = 123; } } $nesting1 = new Nesting1(new stdClass()); $nesting1->mutate(); $nesting1->nesting->mutate(); ``` ``` Psalm output (using commit 4ebe4c1): No issues! ```
klimick commented 1 year ago

See #5441 I even forgot that it used to be. But for some reason this issue was ignored (

See #6881 similar issue

kkmuffme commented 1 year ago

@ygottschalk your example is wrong (I saw you do the same thing before), but this is not a valid syntax. When you do the example correctly: https://psalm.dev/r/a1981ffdc7

It is reported even without @psalm-immutable.

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/a1981ffdc7 ```php neverChange->foo = 1; } } class DoOtherStuff { public function doStuff(Immutable $immutable): void { $immutable->mutate(); } } $data = new stdClass; $data->foo = 0; $immutable = new Immutable($data); /* ... */ if ($immutable->neverChange->foo === 0) { /* ... */ (new DoOtherStuff())->doStuff($immutable); /* Here I would assume, that my immutable object still holds the not change value stdClass::foo = 0 but it got mutated to stdClass::foo = 1 */ } ``` ``` Psalm output (using commit 4ebe4c1): ERROR: TooManyArguments - 17:14 - Too many arguments for Immutable::__construct - expecting 0 but saw 1 ERROR: PropertyNotSetInConstructor - 5:21 - Property Immutable::$neverChange is not defined in constructor of Immutable or in any methods called in the constructor ```
ygottschalk commented 1 year ago

@kkmuffme My first given example copy-pasted to some sandbox: https://3v4l.org/IfpNO#v8.2.7

When you do the example correctly: [...]

Let me correct your "correction": https://psalm.dev/r/29a298f5a3 - No errors

It is reported even without @psalm-immutable.

I was talking about an ImpurePropertyAssignment error, not some TooManyArguments or PropertyNotSetInConstructor

psalm-github-bot[bot] commented 1 year ago

I found these snippets:

https://psalm.dev/r/29a298f5a3 ```php neverChange = $neverChange; } public function mutate(): void { $this->neverChange->foo = 1; } } class DoOtherStuff { public function doStuff(Immutable $immutable): void { $immutable->mutate(); } } $data = new stdClass; $data->foo = 0; $immutable = new Immutable($data); /* ... */ if ($immutable->neverChange->foo === 0) { /* ... */ (new DoOtherStuff())->doStuff($immutable); /* Here I would assume, that my immutable object still holds the not change value stdClass::foo = 0 but it got mutated to stdClass::foo = 1 */ } ``` ``` Psalm output (using commit 99a54fb): No issues! ```
ygottschalk commented 1 year ago

@klimick Reading your post I was like "Wasn't that an issue I worked around someday..?" but reading on I saw you actually found that.

How?

I was just trying to explain what my general understanding / interpretation of an 'immutable object' is. I am aware that it is (near) impossible to enforce that. Even if we would require a deep clone which would be guaranteed to not have any references elsewhere, you could easily use the Reflection API to access and change that.