Skip to content

Conversation

@pkoenig10
Copy link
Member

@pkoenig10 pkoenig10 commented Dec 18, 2020

Similar to #1442

Before this PR

MissingCasesInEnumSwitch results in false positives when using switch expressions.

After this PR

MissingCasesInEnumSwitch is disabled when using Java 14 or newer.

Upstream fix is in google/error-prone#2026.

@changelog-app
Copy link

changelog-app bot commented Dec 18, 2020

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Disable MissingCasesInEnumSwitch on Java 14 source to avoid false positives

Check the box to generate changelog(s)

  • Generate changelog entry


@Test
@DisabledForJreRange(max = JRE.JAVA_13)
public void testSwitchExpression() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Once the issue is fixed in error-prone this test will begin failing. Unfortunately, we use Java 11 in CI so this test isn't run.

I'll try to remember to come back and revert this after the upstream fix is released.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can add a test which uses the errorprone package implementation version which forces us to take action when the version changes -- that way we're forced to remember :-)

@iamdanfox
Copy link
Contributor

Thanks for landing a fix for this upstream! IMO this is one of the most valuable checks error-prone has, so I'd want to be a littl cautious about turning it off too soon (especially as we're pushing all repos internally to compile with Java15 JVMs, even if they only use sourceCompat=11 and run on Java 11 JVMs).

Do you think it would be tolerable to leave it on but just recommend folks duplicate the branches for now?? e.g.

enum Case { ONE, TWO }
void m(Case c) {
  switch (c) {
    case ONE, TWO -> {}
  }
}
enum Case { ONE, TWO }
void m(Case c) {
  switch (c) {
    case ONE -> {}
    case TWO -> {}
  }
}

@iamdanfox
Copy link
Contributor

Actually, seems like the java compiler itself actually requires exhaustiveness of patterns in switch expressions, so maybe this is fine to suppress??

> Task :container-vuln-scanner:compileJava FAILED
/Volumes/git/container-vuln-scanner/container-vuln-scanner/src/main/java/com/palantir/containervulnscanner/SideEffects.java:148: error: the switch expression does not cover all possible input values
        return switch (scanOutcome) {

@pkoenig10
Copy link
Member Author

IMO this is one of the most valuable checks error-prone has, so I'd want to be a little cautious about turning it off too soon

100% agree, I was also concerned about this.

Actually, seems like the java compiler itself actually requires exhaustiveness of patterns in switch expressions, so maybe this is fine to suppress??

Yes, but only for switch expressions. So if you're using a mixture of switch statements and switch expressions, then you will lose the protection for the switch statements.

Do you think it would be tolerable to leave it on but just recommend folks duplicate the branches for now?? e.g.

This works, but it might be pretty clunky if the branches not just a trivial yield and would have to be duplicated many times.

@pkoenig10
Copy link
Member Author

Thinking about this some more, this change feels like a net negative. It's relatively easy for repos to disable this check if they want to fully migrate to switch expressions to instead rely on the compiler for these checks.

Alternatively, repos could just continue using switch statements in places they would have multiple expressions on a case label.

@pkoenig10 pkoenig10 closed this Dec 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants