Unused method calls when class is immutable #6967
Comments
|
I found these snippets: https://psalm.dev/r/50306152ed<?php
declare(strict_types=1);
/** @psalm-immutable */
final class UserList
{
public function validate(): void
{
// Some validation happens here
throw new \InvalidArgumentException();
}
}
$a = new UserList();
$a->validate();https://psalm.dev/r/df40430282<?php
declare(strict_types=1);
/** @psalm-immutable */
final class UserList
{
private array $userIds = [];
public function __construct(array $userIds)
{
$this->userIds = $userIds;
}
public function add(string $userId): self
{
$newUserIds = $this->userIds;
$newUserIds[] = $userId;
return new self($newUserIds);
}
public function mustContainUserId(string $userId): void
{
if (!in_array($userId, $this->userIds, true)) {
throw new \DomainException('Missing user');
}
}
public function mustNotBeEmpty(): void
{
if (count($this->userIds) === 0) {
throw new \DomainException('No user available');
}
}
}
$a = new UserList([]);
$a->mustNotBeEmpty();
$a->mustContainUserId('d90c8652-1dab-4bc1-9d51-a02c758408c5'); |
|
Yeah, this is an issue. It has to do with the definition of immutable and the assumption that void immutable function are useless. Psalm rightfully assume that a true immutable function who doesn't return is useless. However, throwing an exception is hardly true immutability. I guess this could be solved by removing the unused method call when |
|
@orklah Thanks for the quick reply.
What part of immutability goes against throwing an exception? I've just reread the Psalm article about immutability, but didn't find a reference to it. It's quite possible that I'm missing something, I just don't understand what part 🙂 We use immutable objects a lot with value objects. And put validation in the value objects themselves. This way we have readable code like:
Unfortunately using a |
|
I found these snippets: https://psalm.dev/r/e281137268<?php
declare(strict_types=1);
/** @psalm-immutable */
final class UserList
{
private array $userIds = [];
public function __construct(array $userIds)
{
$this->userIds = $userIds;
}
public function add(string $userId): self
{
$newUserIds = $this->userIds;
$newUserIds[] = $userId;
return new self($newUserIds);
}
/** @throws \DomainException */
public function mustContainUserId(string $userId): void
{
if (!in_array($userId, $this->userIds, true)) {
throw new \DomainException('Missing user');
}
}
/** @throws \DomainException */
public function mustNotBeEmpty(): void
{
if (count($this->userIds) === 0) {
throw new \DomainException('No user available');
}
}
}
$a = new UserList([]);
$a->mustNotBeEmpty();
$a->mustContainUserId('d90c8652-1dab-4bc1-9d51-a02c758408c5'); |
|
What I meant to say is:
Currently it does both and so it suggest removing function that have external side effects (namely, throwing an exception) My take is that immutable function shouldn't be able to throw, but it's not the current definition in Psalm. It could be debated that immutable stays with the current definition and we need a new term for when a function doesn't have side effects AND doesn't throw, but it will start getting confusing. In the meantime, a change could be made so that Psalm stop emitting UnusedMethodCall when the method has @throws (but it doesn't currently work) |
|
@orklah My understanding is that pure functions aren't allowed to throw exceptions. But immutable classes doesn't have to be pure. They can but pure would be the next step. I would be very much in favor of allowing exceptions to be thrown on functions within immutable classes 🙂 |
|
Psalm just don't consider exceptions at all for pure/immutable: https://psalm.dev/r/5e92a0f653 |
|
I found these snippets: https://psalm.dev/r/5e92a0f653<?php
/**
* @psalm-pure
*/
function takesAnInt(int $i): void {
throw new Exception((string)$i);
} |
See #2160
It doesn't have to be a single term. @Ocramius proposed |
Yeah, but requiring people to add both immutable and never-throw just to be able to flag the function as unused -and only if it returns void- seems a bit much... |
Throw can be pure - the only impure fact in it is the current stack trace, but there's nothing impure in a thrown exception otherwise. |
What I don't get is that somehow, those two aren't treated the same way: According to Psalm's doc, a
Either references are not considered as outputs and the first one should be fine or the reference is an output, and in this case, the exception should be too, no? |
|
I found these snippets: https://psalm.dev/r/d114adc5ee<?php
class A{
/** @psalm-pure */
public static function B(string &$error): void{
$error = 'this is an error';
}
}
class C{
/** @psalm-pure */
public static function D(): void{
throw new Exception('this is an error');
}
}
/** @psalm-pure */
function pure(): void{
$myError = '';
A::B($myError);
$_myOtherError = '';
try{
C::D();
}
catch(Exception $e){
$_myOtherError = $e->getMessage();
}
} |
Recently there was an issue which was fixed that mentioned unused method calls (#5528) which was marked as resolved and solved the issue when
@psalm-assertis used.Unfortunately there is still an issue when there is no
@psalm-assert. For some cases using@psalm-assertdoesn't make sense because we don't assert the type of parameter (we already handle that through typing) or the method doesn't have a parameter to begin with.I didn't add a reproducible case back then, my bad.
Here are two cases which reproduce the open issue. One which is the simplest way to reproduce it and another that is a real life example:
Small reproduction:
https://psalm.dev/r/50306152ed
Real life example:
https://psalm.dev/r/df40430282
The text was updated successfully, but these errors were encountered: