Skip to content

Implement identical/equal comparisons on EnumCaseObjectType#2095

Merged
ondrejmirtes merged 2 commits into
phpstan:1.9.xfrom
staabm:identical-enum
Dec 13, 2022
Merged

Implement identical/equal comparisons on EnumCaseObjectType#2095
ondrejmirtes merged 2 commits into
phpstan:1.9.xfrom
staabm:identical-enum

Conversation

@staabm

@staabm staabm commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this new error is a bit misleading. not sure we want to keep it like that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in contrast, I like this error way more then the one we had before this PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use EnumCaseObjectType::equals() instead.

@staabm staabm Dec 13, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch - fixed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@staabm
staabm marked this pull request as ready for review December 13, 2022 08:18
@phpstan-bot

Copy link
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes
ondrejmirtes merged commit 5fc2bfd into phpstan:1.9.x Dec 13, 2022
@ondrejmirtes

Copy link
Copy Markdown
Member

Thank you!

@staabm
staabm deleted the identical-enum branch December 13, 2022 08:35
@ondrejmirtes

Copy link
Copy Markdown
Member

Sorry, I had to revert this, it's pretty annoying in practice. We first need to do a different feature in order for this one to be less annoying.

@staabm

staabm commented Dec 13, 2022

Copy link
Copy Markdown
Contributor Author

no problem. in case there is something I can do, please open a issue about the thing we need, so I can investigate.

@ondrejmirtes

Copy link
Copy Markdown
Member

The problem is that this now reports "always true" for the last elseif in an if-elseif-else blocks. In real-world projects this now reports dozens of new errors.

Also arguably last match arm shouldn't report this. Because people might prefer to write Foo::Bar => doFoo() instead of default => doFoo() to be notified when there's a new enum case to handle.

So, I have an idea: We need a new custom node visitor to mark the condition expression in last match arm and last elseif with an attribute. And ConstantConditionRuleHelper can use this node attribute to decide that "always true" shouldn't be reported there.

There's already an issue for this: phpstan/phpstan#8042

And after this is implemented, we can afford to report all "always true" conditions for everyone in bleedingEdge/2.0, not just phpstan-strict-rules users. Because the most annoying ones will not be reported anymore.

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

Successfully merging this pull request may close these issues.

Same enums comparison is not reported

3 participants