Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

@psalm-mutation-free only works *once* inside if #8133

Closed
ThomasLandauer opened this issue Jun 21, 2022 · 7 comments
Closed

@psalm-mutation-free only works *once* inside if #8133

ThomasLandauer opened this issue Jun 21, 2022 · 7 comments

Comments

@ThomasLandauer
Copy link
Contributor

https://psalm.dev/r/fb594bf0e4

This line is not raising an error, due to @psalm-mutation-free and if (is_string()):

$cat->setBar($foo->getBar());

However, the next line does give an error - so it looks like @psalm-mutation-free isn't "working" here anymore:

echo $foo->getBar() . $foo->getBar();

If I delete the first line, the second line does not raise the error. This makes me think that @psalm-mutation-free is only working at the first call.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/fb594bf0e4
<?php

class Foo
{
    private ?string $bar = null;
    
    /**
     * @psalm-mutation-free
     */
    public function getBar(): ?string
    {
        return $this->bar;
    }
}

class Cat
{
    private ?string $bar = null;

    public function setBar(string $bar): void
    {
        $this->bar = $bar;
    }

}

$foo = new Foo();
$cat = new Cat();
if (is_string($foo->getBar())) {
    $cat->setBar($foo->getBar());
    echo $foo->getBar() . $foo->getBar();
}
Psalm output (using commit 10ea05a):

INFO: PossiblyNullOperand - 31:10 - Cannot concatenate with a possibly null null|string

INFO: PossiblyNullOperand - 31:27 - Cannot concatenate with a possibly null null|string

@someniatko
Copy link
Contributor

Psalm cannot know what happens inside the setBar() when it evaluates usages of this method. You could for example change $this->bar to null inside of it.

@ThomasLandauer
Copy link
Contributor Author

Hm. But it's $cat->setBar(). How could this modify the value of $foo->getBar()?

@AndrolGenhald
Copy link
Collaborator

Psalm isn't smart enough to tell that $cat can't contain a reference to $foo.

@orklah
Copy link
Collaborator

orklah commented Jun 21, 2022

and most importantly, Psalm doesn't check method name like setXXX or getXXX, so the method name could as well be called $cat->changeTheValueOfEveryFooInstanceInMemory($foo->getBar()); 😄

@ThomasLandauer
Copy link
Contributor Author

ThomasLandauer commented Jun 21, 2022

I see, indeed, it could even be $cat->setBar('123') (i.e. no need for $foo->getBar() as argument).

So the exact rule is that there must not be a call to any function (more precisely: to any function without @psalm-mutation-free) between the if (is_string()) and the actual call to $foo->getBar()?

This is somewhat surprising (at least to me) - what about adding more explanation to https://psalm.dev/docs/running_psalm/issues/PossiblyNullArgument/ ?
Are those error docs pages so minimalist on purpose, or just cause nobody took the time yet to add some hints about how the issue could be resolved?

@orklah
Copy link
Collaborator

orklah commented Jun 21, 2022

Mainly the latter, contributions are very welcome :)

ThomasLandauer added a commit to ThomasLandauer/psalm that referenced this issue Jun 21, 2022
See vimeo#8133 (comment)

Don't know if this is the best way to explain this, but it's a start :-)

Is there a better way to add the link to https://psalm.dev/docs/annotating_code/supported_annotations/#psalm-mutation-free ?

I also removed the `<?php` tag from the code block.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants