Skip to content
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

Issue #11220: False positive in MissingSwitchDefault with pattern in switch label #11221

Merged
merged 1 commit into from
Jan 28, 2022

Conversation

nrmancuso
Copy link
Member

@nrmancuso nrmancuso commented Jan 20, 2022

Closes #11220

Report label: all projects

Diff Regression config: https://gist.githubusercontent.com/nmancus1/3db3e23a5063857160d7d92cbdfa1bfe/raw/b8fc9b284c6604bf0862f237f030794bfb0a7800/config.xml

Diff Regression projects: https://raw.githubusercontent.com/checkstyle/contribution/e34312d61da0b073f31339cfbf385f289b721fed/checkstyle-tester/projects-to-test-on-for-github-action.properties

Final report: https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/f5590ec_2022232703/reports/diff/index.html

Only diffs are in openjdk17, in noncompilable files. I am not sure if we should add files filters for these files, though, since they are how I confirmed logic of check update. I think that they might be valuable in the future for similar reasons.

Blocked until #11100

Only last commit is under review.

Example of compile error on new enum value:

$ cat Test.java 
public class Test {
enum Color { RED, GREEN, BLUE, MY_COLOR }

/**
 * In the case of switch expressions, the compiler requires
 * that they are exhaustive, so this check does not enforce
 * a default label. This code would still compile with a
 * default label, but it is unnecessary so this check does
 * not enforce it.
 */
static int showSwitchExpressionExhaustiveness(Color color) {
    switch (color) {
        case RED: System.out.println("RED"); break;
        case BLUE: System.out.println("BLUE"); break;
        case GREEN: System.out.println("GREEN"); break;
        // Check will require default label below, compiler
        // does not enforce a switch statement with constants
        // to be complete.
        //default: System.out.println("Something else");
    }

    // Check will not require default label in switch
    // expression below, because code will not compile
    // if all possible values are not handled. If the
    // 'Color' enum is extended, code will fail to compile.
    return switch (color) {
        case RED:
            yield 1;
        case GREEN:
            yield 2;
        case BLUE:
            yield 3;
    };
}

  }

$ javac --enable-preview --release 17 Test.java 
Test.java:26: error: the switch expression does not cover all possible input values
    return switch (color) {
           ^
1 error

@nrmancuso nrmancuso self-assigned this Jan 20, 2022
@nrmancuso nrmancuso force-pushed the issue-11220 branch 2 times, most recently from 7bbd54b to e4b6731 Compare January 20, 2022 01:21
@nrmancuso nrmancuso marked this pull request as draft January 20, 2022 01:24
@nrmancuso nrmancuso removed their assignment Jan 20, 2022
@nrmancuso nrmancuso marked this pull request as ready for review January 20, 2022 05:30
@nrmancuso nrmancuso force-pushed the issue-11220 branch 3 times, most recently from 8e3e43b to 6702d33 Compare January 20, 2022 17:10
@nrmancuso nrmancuso self-assigned this Jan 20, 2022
@nrmancuso nrmancuso force-pushed the issue-11220 branch 4 times, most recently from 06f1f01 to 21925fb Compare January 20, 2022 19:11
Copy link
Member Author

@nrmancuso nrmancuso left a comment

Choose a reason for hiding this comment

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

Comments:

@@ -127,22 +139,71 @@ public void visitToken(DetailAST ast) {
* @param caseGroupAst first case group to check.
* @return true if 'default' switch found.
*/
private static boolean containsDefaultSwitch(DetailAST caseGroupAst) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed the name of this method to be consistent with language from the JLS: https://docs.oracle.com/javase/specs/jls/se17/preview/specs/patterns-switch-jls.html

This language is also used throughout the check for consistency.

void m2(String s) {
switch (s) { // ok
case "a": throw new AssertionError("Wrong branch.");
case default: break;
Copy link
Member Author

Choose a reason for hiding this comment

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

default case label

@nrmancuso
Copy link
Member Author

Github, generate report

@checkstyle checkstyle deleted a comment from github-actions bot Jan 20, 2022
@checkstyle checkstyle deleted a comment from github-actions bot Jan 20, 2022
@checkstyle checkstyle deleted a comment from github-actions bot Jan 20, 2022
@checkstyle checkstyle deleted a comment from github-actions bot Jan 20, 2022
@checkstyle checkstyle deleted a comment from github-actions bot Jan 20, 2022
@checkstyle checkstyle deleted a comment from github-actions bot Jan 20, 2022
@checkstyle checkstyle deleted a comment from github-actions bot Jan 20, 2022
@checkstyle checkstyle deleted a comment from github-actions bot Jan 20, 2022
@checkstyle checkstyle deleted a comment from github-actions bot Jan 20, 2022
@checkstyle checkstyle deleted a comment from github-actions bot Jan 20, 2022
@github-actions
Copy link
Contributor

@nrmancuso
Copy link
Member Author

GitHub, rebase

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Item:

@nrmancuso nrmancuso force-pushed the issue-11220 branch 2 times, most recently from 81fdc35 to 314603a Compare January 23, 2022 20:47
@nrmancuso nrmancuso removed the blocked label Jan 23, 2022
@romani
Copy link
Member

romani commented Jan 24, 2022

Github, generate site

@github-actions
Copy link
Contributor

@nrmancuso
Copy link
Member Author

Github, generate site

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

item:

@nrmancuso nrmancuso force-pushed the issue-11220 branch 2 times, most recently from 39983b1 to d50c431 Compare January 26, 2022 03:59
@romani
Copy link
Member

romani commented Jan 26, 2022

GitHub, generate web site

@nrmancuso
Copy link
Member Author

Github, rebase

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Ok to merge

@@ -17,7 +17,9 @@
introduction of new types in an enumeration type. Note that
the compiler requires switch expressions to be exhaustive,
so this check does not enforce default branches on
such expressions.
such expressions. Also, switch statements that use pattern or null
Copy link
Member

Choose a reason for hiding this comment

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

@romani Am I missing something that this file looks manually changed and isn't using the description in the javadoc and xdoc?

Copy link
Member

Choose a reason for hiding this comment

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

@nmancus1 Are you manually modifying this file?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you manually modifying this file?

No.

Copy link
Member

Choose a reason for hiding this comment

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

Such files are updated by test that generate meta data

@romani
Copy link
Member

romani commented Jan 27, 2022

GitHub, generate web site

@rnveach rnveach merged commit 7cd807c into checkstyle:master Jan 28, 2022
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.

False positive in MissingSwitchDefault with pattern in switch label
4 participants