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

[Jjva] SwitchStmtsShouldHaveDefault should be aware of enum types #651

Closed
blerner opened this Issue Oct 4, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@blerner

blerner commented Oct 4, 2017

Rule Set: design

Description:
https://sourceforge.net/p/pmd/feature-requests/491/
and further discussion at
https://sourceforge.net/p/pmd/discussion/188194/thread/468076db/

Students of mine got bit by this bug recently, and I noticed this old feature request that seems not to have migrated to this git repo. Having a default case prevents intellij or eclipse from providing warnings about incomplete switch cases, when the enum definition changes. This seems to be a case of two useful tools fighting each other :-) Also, supporting this exemption helps pmd align with the Google style guide for switch statements more precisely.

(Not sure if this issue was discussed and abandoned, lost in the sf->git migration, or just ignored, so apologies if this is a duplicate...)

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Oct 4, 2017

@blerner thanks for migrating this. We didn't mass migrate issues, since several of the old ones had already become obsolete. We rely con the community to bring to our attentions those they still care about, just like you did.

This seems like a reasonable change. We will have to iterate this a bit, and will probably not make it for 6.0.0, still good to have it around.

@blerner

This comment has been minimized.

blerner commented Mar 29, 2018

Bump? Is it possible somewhere in the 6.* timeframe, or only in 7.0 and beyond?

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Mar 29, 2018

@blerner I can't foresee any breaking API changes being introduced by this, so there should be no blockers to get it into a 6.X release. It's just a matter of getting it prioritized / a PR, and merged.

Given the current status of type resolution, checking if the variable used for the switch has a non-null type, and then just checking if that Class<?> corresponds to an enum Enum.isAssignableFrom() to whitelist a switch statement should be plausible (it wasn't back in 2012 when the original issue was submitted).

@oowekyala

This comment has been minimized.

Member

oowekyala commented Jun 4, 2018

It would be nice to only whitelist the switch if every enum constant has its label. Similarly, we could have a rule detect unnecessary default cases if the switch is exhaustive.

The best implementation I can think of would be to add a method isExhaustive to ASTSwitchStatement, so that the code can be reused between the two rules, and they can be written in XPath.

@oowekyala oowekyala added the has:pr label Jun 5, 2018

@jsotuyod jsotuyod added this to the 6.5.0 milestone Jun 5, 2018

@jsotuyod jsotuyod changed the title from [Java] SwitchStmtsShouldHaveDefault should be aware of enum types to [Jjva] SwitchStmtsShouldHaveDefault should be aware of enum types Jun 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment