-
Notifications
You must be signed in to change notification settings - Fork 657
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
Regression in 5.18.0 throws RedundantCondition after comparing with ===
to another value of the same type
#10534
Comments
I found these snippets: https://psalm.dev/r/32b06be2e3<?php
class Room {
public const TYPE_UNKNOWN = -1;
public const TYPE_ONE_TO_ONE = 1;
public const TYPE_GROUP = 2;
public const TYPE_PUBLIC = 3;
public const TYPE_CHANGELOG = 4;
public const TYPE_ONE_TO_ONE_FORMER = 5;
public const TYPE_NOTE_TO_SELF = 6;
/**
* @psalm-param Room::TYPE_* $type
*/
public function __construct(
private int $type,
) {
}
/**
* @return int
* @psalm-return Room::TYPE_*
*/
public function getType(): int {
return $this->type;
}
/**
* @param int $type
* @psalm-param Room::TYPE_* $type
*/
public function setType(int $type): void {
$this->type = $type;
}
}
class RoomService {
/**
* @param Room $room
* @param int $newType Currently it is only allowed to change between `Room::TYPE_GROUP` and `Room::TYPE_PUBLIC`
* @param bool $allowSwitchingOneToOne Allows additionally to change the type from `Room::TYPE_ONE_TO_ONE` to `Room::TYPE_ONE_TO_ONE_FORMER`
* @return bool True when the change was valid, false otherwise
*/
public function setType(Room $room, int $newType, bool $allowSwitchingOneToOne = false): bool {
$oldType = $room->getType();
if ($newType === $oldType) {
return true;
}
if (!$allowSwitchingOneToOne && $oldType === Room::TYPE_ONE_TO_ONE) {
return false;
}
if ($oldType === Room::TYPE_ONE_TO_ONE_FORMER) {
return false;
}
if ($oldType === Room::TYPE_NOTE_TO_SELF) {
return false;
}
if (!in_array($newType, [Room::TYPE_GROUP, Room::TYPE_PUBLIC, Room::TYPE_ONE_TO_ONE_FORMER], true)) {
return false;
}
if ($newType === Room::TYPE_ONE_TO_ONE_FORMER && $oldType !== Room::TYPE_ONE_TO_ONE) {
return false;
}
$room->setType($newType);
if ($oldType === Room::TYPE_PUBLIC) {
// … something
}
return true;
}
}
|
nickvergessen
added a commit
to nextcloud/spreed
that referenced
this issue
Jan 9, 2024
Ref vimeo/psalm#10534 Signed-off-by: Joas Schilling <coding@schilljs.com>
I bisected it now and it seems to be: 108f626
I also reverted the changes in |
Ok, so the issue is that it works with literal strings, but literal int's have an issue. I won't have time to investigate this until beginning of next month, feel free to give it a try and PR a fix |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The "problematic" code is:
https://github.com/nextcloud/spreed/blob/bb89b3f371cacd542d7aaa1961e5d65e2333e48e/lib/Service/RoomService.php#L490
The problem seems to be due to:
https://github.com/nextcloud/spreed/blob/bb89b3f371cacd542d7aaa1961e5d65e2333e48e/lib/Service/RoomService.php#L474
As removing those lines fixes it.
So I changed it to use a variable:
Still results in the same
RedundantCondition
.However changing the order of the comparison now makes it work:
The regression should be within this gap, as a75d26a (used by playground) works and b113f3e which is the 5.18.0 release reports the issue.
a75d26a...b113f3e
The text was updated successfully, but these errors were encountered: