vimeo / psalm

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

Support @mutable-in-scope or similar #4348

Open jsiefer opened 4 years ago

jsiefer commented 4 years ago

Hi,

I was wondering if would possible to allow a tag for creating an object within in a current function scope and allow changing properties, but once it leaves that scope it becomes immutable even to itself. Unless you clone it, then you can edit it again in your scope.

Here is an example: https://psalm.dev/r/e51136a00b

<?php
declare(strict_types=1);

/**
 * @mutable-in-scope
 */
class Person {
    /** @var string */
    public $name = '';

    public function rename(string $name): self
    {
        $self = clone $this;
        $self->name = $name;

        return $self;
    }
}

/**
 * @param list<Person> $persons
 * @param string $name
 *
 * @return list<Person>
 */
function renameAll(array $persons, string $name): array
{
    $result = [];
    foreach ($persons as $person) {
        $person = $result[] = clone $person;
        $person->name = $name;
    }

    return $result;
}

function createJohn(): Person
{
    $a = new Person();
    $a->name = 'John';

    return $a;
}

$john = createJohn();
$john->name = 'bad'; // outside of scope

$bob = $john->rename('bob');
psalm-github-bot[bot] commented 4 years ago

I found these snippets:

https://psalm.dev/r/e51136a00b ```php name = $name; return $self; } } /** * @param list $persons * @param string $name * * @return list */ function renameAll(array $persons, string $name): array { $result = []; foreach ($persons as $person) { $person = $result[] = clone $person; $person->name = $name; } return $result; } function createJohn(): Person { $a = new Person(); $a->name = 'John'; return $a; } $john = createJohn(); $john->name = 'bad'; // outside of scope $bob = $john->rename('bob'); ``` ``` Psalm output (using commit 083cc29): INFO: UnusedVariable - 50:1 - Variable $bob is never referenced ```
muglug commented 4 years ago

@immutable should work fine? https://psalm.dev/r/4b8b30e7f7

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

I found these snippets:

https://psalm.dev/r/4b8b30e7f7 ```php name = $name; return $self; } } /** * @param list $persons * @param string $name * * @return list */ function renameAll(array $persons, string $name): array { $result = []; foreach ($persons as $person) { $person = $result[] = clone $person; $person->name = $name; } return $result; } function createJohn(): Person { $a = new Person(); $a->name = 'John'; return $a; } $john = createJohn(); $john->name = 'bad'; // outside of scope $name = $john->rename('bob'); ``` ``` Psalm output (using commit 234eae6): No issues! ```
jsiefer commented 4 years ago

It should throw an error at line 48 $john->name = 'bad'; // outside of scope, or?

andrew-demb commented 4 years ago

Annotation should be prefixed - psalm-immutable. https://psalm.dev/r/dee28ab0da

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

I found these snippets:

https://psalm.dev/r/dee28ab0da ```php name = $name; return $self; } } /** * @param list $persons * @param string $name * * @return list */ function renameAll(array $persons, string $name): array { $result = []; foreach ($persons as $person) { $person = $result[] = clone $person; $person->name = $name; } return $result; } function createJohn(): Person { $a = new Person(); $a->name = 'John'; return $a; } $john = createJohn(); $john->name = 'bad'; // outside of scope $name = $john->rename('bob'); ``` ``` Psalm output (using commit aee4311): ERROR: InaccessibleProperty - 31:9 - Person::$name is marked readonly ERROR: InaccessibleProperty - 42:5 - Person::$name is marked readonly ERROR: InaccessibleProperty - 48:1 - Person::$name is marked readonly ```
jsiefer commented 4 years ago

Right, but @psalm-immutable does not allow changes of objects/structs created in the same scope where it has been created.

ERROR: InaccessibleProperty - 31:9 - Person::$name is marked readonly

ERROR: InaccessibleProperty - 42:5 - Person::$name is marked readonly

Those lines should be valid for "@mutable-in-scope"

muglug commented 4 years ago

Yes, sorry, I used the wrong annotation. Is there a reason https://psalm.dev/r/5a888fc54a is not ok?

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

I found these snippets:

https://psalm.dev/r/5a888fc54a ```php name = $name; return $self; } } /** * @param list $persons * @param string $name * * @return list */ function renameAll(array $persons, string $name): array { $result = []; foreach ($persons as $person) { $person = clone $person; $person->rename($name); $result[] = $person; } return $result; } function createJohn(): Person { $a = new Person(); $a->rename('John'); return $a; } $john = createJohn(); $john->name = 'bad'; // outside of scope $name = $john->rename('bob'); ``` ``` Psalm output (using commit 862a956): ERROR: InaccessibleProperty - 49:1 - Person::$name is marked readonly ```
jsiefer commented 4 years ago

When you have a lot of data objects and manipulated them, then by using the @immutable approach above, a function has to be called for any change on a data object, this is tiny overhead for small projects but can be come quite big on more complex systems.

PHP can be super quick and can easily handle a large number of objects and assignments, but by using immutable setters, millions of functions calls would cause PHP to become quite slow.

So the idea is, you can manipulate an object in your scope, set multiple attributes and compute on them etc, but as soon as it leaves your scope, the object becomes immutable. This design works great, but has to be enforced manually at the moment. I thought psalm might be able to cover that scenario.

orklah commented 3 years ago

Can you post some kind of before/after? I'm having troubles understanding what's not ok

jsiefer commented 2 years ago

Hi @orklah

Here is an example:

<?php

/**
 * @mutable-in-scope
 */
class Point {
    /** @var float */
    public $x = .0;

    /** @var float */
    public $y = .0;
}

/**
 * @param list<Point> $points
 * @param float $x
 * @param float $y
 *
 * @return list<Point>
 */
function translate(array $points, float $x, float $y): array
{
    foreach ($points as &$p) {
        $p = clone $p;
        $p->x += $x; // valid
        $p->y += $y; // valid
    }

    return $points;
}

/**
 * @param list<Point> $points
 * @param float $x
 * @param float $y
 *
 * @return list<Point>
 */
function translate2(array $points, float $x, float $y): array
{
    foreach ($points as &$p) {
        $p->x += $x; // invalid
        $p->y += $y; // invalid
    }

    return $points;
}

$a = new Point();
$a->x = 5.0; // valid
[$a] = translate([$a], 10.0, 5.0);
$a->x = 5.0; // invalid
[$a] = translate2([$a], 10.0, 5.0); // invalid

Now this is a super simple example and one could question this approach. However, consider having millions of points and multiple properties. It would be nice if an object is immutable by default, unless it is freshly instantiated or cloned so that the original object stays intact.

orklah commented 2 years ago

Well, I wouldn't be opposed to merge a PR that would do that personally. I won't work on it myself though...