Fix isValid static analysis#166
Conversation
| * | ||
| * @param $value | ||
| * @psalm-param mixed $value | ||
| * @psalm-param T $value |
There was a problem hiding this comment.
imo this doesn't make sense, if i declare value as T i would already knew it is valid, no?
Maybe you can rather prepare a fail case which proofs that there is a problem with the current implementation?
There was a problem hiding this comment.
Let's assume this class:
/**
* @extends Enum<string>
*/
class Status extends Enum {
const SUCCESS = 'SUCCESS';
const FAILURE = 'FAILURE';
}The behavior of isValid is as follows:
var_dump(Status::isValid('SUCCESS')); // true
var_dump(Status::isValid('UNKNOWN')); // false
var_dump(Status::isValid(1)); // falseWith your implementation static analysis fails for all 3:
// Call to static method MyCLabs\Enum\Enum<string>::isValid() with 'SUCCESS' will always evaluate to true.
var_dump(Status::isValid('SUCCESS')); // true
// Call to static method MyCLabs\Enum\Enum<string>::isValid() with 'UNKNOWN' will always evaluate to true.
var_dump(Status::isValid('UNKNOWN')); // false + the static analysis is incorrect
// Call to static method MyCLabs\Enum\Enum<string>::isValid() with 1 will always evaluate to false.
var_dump(Status::isValid(1)); // falseWith my suggestion:
var_dump(Status::isValid('SUCCESS')); // true, static analysis passes
var_dump(Status::isValid('UNKNOWN')); // false, static analysis passes
var_dump(Status::isValid(1)); // false, static analysis fails saying that isValid expects a string, int givenThere was a problem hiding this comment.
imo the problem is your Enum<string> annotation, with it you declare T as any string, but it is only SUCCESS and FAILURE.
Can your try this:
/**
* @extends Enum<Status::*>
*/
class Status extends Enum {
const SUCCESS = 'SUCCESS';
const FAILURE = 'FAILURE';
}There was a problem hiding this comment.
Ooooh! Okay, interesting. I was completely unaware of that syntax. With that it works fine of course. It should be mentioned in the readme somewhere. Thanks!
There was a problem hiding this comment.
Agree with the readme, maybe you can just open a PR for it?
In addition, worth to mention you can also use only a specific scope of constants
/**
* @extends Enum<Status::VALUE_*>
*/
class Status extends Enum {
const VALUE_SUCCESS = 'SUCCESS';
const VALUE_FAILURE = 'FAILURE';
const FOO = 'BAR';
}or as literal values
/**
* @extends Enum<'SUCCESS'|'FAILURE'>
*/
class Status extends Enum {
const SUCCESS = 'SUCCESS';
const FAILURE = 'FAILURE';
}
I don't think the
@psalm-assert-if-trueis correct here. It causes false-positives like this with PHPStan 1.9:Which is of course wrong. Even if value is a string it doesn't necessarily mean it's one of the valid values for the enum.
Instead I think we should type $value with T since it doesn't really make sense to call isValid with a different type anyway - that would always return false.
cc @michaelpetri