-
Notifications
You must be signed in to change notification settings - Fork 546
Improve EnumSanityRule #4543
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
Improve EnumSanityRule #4543
Conversation
src/Rules/Classes/EnumSanityRule.php
Outdated
| } | ||
| if ($enumNode->scalarType === null && $stmt->expr !== null) { | ||
| $errors[] = RuleErrorBuilder::message(sprintf( | ||
| 'Enum %s is not backed, but case %s has value.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message changed from
Enum %s is not backed, but case %s has value %s.
to
Enum %s is not backed, but case %s has value.
I'm not sure you're okay with this or if
- it should be behind bleedingEdge
- it should be behind some extra condition (and have two differents messages ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would the message change? If it has value, we can still include it in the error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check was
if ($stmt->expr instanceof Node\Scalar\Int_ || $stmt->expr instanceof Node\Scalar\String_) {
and is changed to
if ($enumNode->scalarType === null && $stmt->expr !== null) {
so I cannot safely access to $stmt->value
I made 8e72e2b to revert this change for Int/String/Float, but the test show the issue with other things like
case E = false;
case F = Foo::A;
where no value is displayed. Is there some helper method to get a value/name from an Expr ? Or is it ok like this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can either use ExprPrinter on $stmt->expr, or you can get type of $stmt->expr with InitializerExprTypeResolver and do ->describe(VerbosityLevel::value()) on the type. I like the 2nd option more.
src/Rules/Classes/EnumSanityRule.php
Outdated
|
|
||
| $exprType = $scope->getType($stmt->expr); | ||
| $constantValues = $exprType->getConstantScalarValues(); | ||
| if (count($constantValues) === 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By setting enum cases value here, instead of in the condition
if ($stmt->expr instanceof Node\Scalar\Int_ || $stmt->expr instanceof Node\Scalar\String_)
I also catch other constant values like the one set with a class Constant.
Cf test
enum Backed: int {
case One = Foo::A;
case Two = Foo::A;
}
22ab2d1 to
37b0e41
Compare
2c62a53 to
24ef1dc
Compare
|
CS build fail |
Closes phpstan/phpstan#13768