Skip to content

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

Merged
rnveach merged 1 commit into
checkstyle:masterfrom
nrmancuso:issue-11220
Jan 28, 2022
Merged

Issue #11220: False positive in MissingSwitchDefault with pattern in switch label#11221
rnveach merged 1 commit into
checkstyle:masterfrom
nrmancuso:issue-11220

Conversation

@nrmancuso

@nrmancuso nrmancuso commented Jan 20, 2022

Copy link
Copy Markdown
Contributor

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

@nrmancuso nrmancuso left a comment

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.

Comments:

* @param caseGroupAst first case group to check.
* @return true if 'default' switch found.
*/
private static boolean containsDefaultSwitch(DetailAST caseGroupAst) {

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.

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
Copy Markdown
Contributor 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
Copy Markdown
Contributor 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
@nrmancuso

Copy link
Copy Markdown
Contributor Author

GitHub, rebase

@romani romani left a comment

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.

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

romani commented Jan 24, 2022

Copy link
Copy Markdown
Member

Github, generate site

@github-actions

Copy link
Copy Markdown
Contributor

@nrmancuso

Copy link
Copy Markdown
Contributor Author

Github, generate site

@github-actions

github-actions Bot commented Jan 24, 2022

Copy link
Copy Markdown
Contributor

@romani romani left a comment

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.

item:

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

romani commented Jan 26, 2022

Copy link
Copy Markdown
Member

GitHub, generate web site

@github-actions

github-actions Bot commented Jan 26, 2022

Copy link
Copy Markdown
Contributor

@nrmancuso

Copy link
Copy Markdown
Contributor Author

Github, rebase

@romani romani left a comment

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.

Ok to merge

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor 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
Copy Markdown
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

romani commented Jan 27, 2022

Copy link
Copy Markdown
Member

GitHub, generate web site

@github-actions

github-actions Bot commented Jan 27, 2022

Copy link
Copy Markdown
Contributor

@nrmancuso nrmancuso requested a review from rnveach January 28, 2022 03:54
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