New Check YieldCount

[mahfouz72]

child of #14961

Restricts the number of yield statements in switch expression case block.

---

Config

$ cat config.xml 
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="TreeWalker">
        <module name="YieldCount"/>
    </module>
</module>

---

Example

 record ColoredPoint(int p, int x, int c) { }
record Rectangle(ColoredPoint upperLeft, ColoredPoint lowerRight) { }

public class Test {
    int test(Object obj) {
        return switch (obj) {
            case ColoredPoint(int p, int x, int c)-> {  // violation, Yield count is 4 (max allowed is 2)
                if (p > x) yield 0;
                if (c ==9) yield 9;
                if (c == 11) yield 11;
                yield x;
            }
            case Rectangle(var _, var _) -> {  // ok, Yield count is 1
                yield 2;
            }
            default -> throw new IllegalStateException("Unexpected value: " + obj);
        };
    }
    int test2(Object obj) {
        int y =  switch (obj) {
            case ColoredPoint(int p, int x, int c)-> {  // violation, Yield count is 4 (max allowed is 2)
                if (p > x) yield 0;
                if (c ==9) yield 9;
                if (c == 10) yield 10;
                yield x;
            }
            case Rectangle(var _, var _) -> {  // ok, Yield count is 1
                yield 2;
            }
            default -> throw new IllegalStateException("Unexpected value: " + obj);
        };
        return y + 5;
    }
}

---

Expected Output

$ java -jar checkstyle-10.19.0-all.jar -c config.xml Test.java 
Starting audit...
[ERROR] D:\Test.java:7:12: Yield count is 4 (max allowed is 2) [YieldCount]
[ERROR] D:\Test.java:21:13: Yield count is 4 (max allowed is 2) [YieldCount]
Audit done.
Checkstyle ends with 1 errors.

[romani]

can https://checkstyle.sourceforge.io/checks/misc/descendanttoken.html be used ?

[mahfouz72]

It can be used.

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
        "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">
<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <module name="TreeWalker">
        <module name="DescendantToken">
            <property name="tokens" value="SWITCH_RULE, CASE_GROUP"/>
             <property name="limitedTokens" value="LITERAL_YIELD"/>
            <property name="maximumNumber" value="2"/>
        </module>
    </module>
</module>

PS D:\CS\test> java  -jar checkstyle-10.17.0-all.jar -c config.xml src/Test.java
Starting audit...
[ERROR] D:\CS\test\src\Test.java:9:13: Count of 4 for 'SWITCH_RULE' descendant 'LITERAL_YIELD' exceeds maximum count 2. [DescendantToken]
Audit done.
Checkstyle ends with 1 errors.

does this mean we don't need this check? note that we can also use descendantToken instead of ReturnCount

[romani]

@nrmancuso will do decision

[romani]

At least we should extend our examples to let users to have ready to use config for such use case .

Additionally I would update summary of this Check to have word "nested" somewhere and maybe some other words to let all find this Check easier at https://checkstyle.org/checks.html

It is not first time even I can not find this Check, even I know it exists.

[nrmancuso]

can https://checkstyle.sourceforge.io/checks/misc/descendanttoken.html be used ?

Awesome idea, @mahfouz72 let's update this issue to reflect a documentation update in DescendantToken for the yield example and to jazz up the description a bit as @romani suggested above.

[rnveach]

It looks like DescendantToken doesn't have an easy way to configure depth. It will only stop nesting at a prefix count (default is basically infinity), so either we will be scanning into a inner switch rule and combining it's count with the outer count, or not scanning deep enough in nested if statements.

Here is a rough example (may not be a completely accurate code example, but it should provide the point):

$ cat TestClass.java
record ColoredPoint(int p, int x, int c) { }
record Rectangle(ColoredPoint upperLeft, ColoredPoint lowerRight) { }

public class Test {
    int test(Object obj) {
        return switch (obj) {
            case ColoredPoint(int p, int x, int c)-> { // count 2

                int nx = switch (obj) {
                    case ColoredPoint(int p, int x, int c)-> {  // count 1
                        yield x;
                    }
                    default -> throw new IllegalStateException("Unexpected value: " + obj);
                };

                if (p > nx) yield 0;
                yield x;
            }
            default -> throw new IllegalStateException("Unexpected value: " + obj);
        };
    }
}

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>
    <property name="severity" value="warning"/>
    <property name="haltOnException" value="false"/>

    <module name="TreeWalker">
        <module name="DescendantToken">
            <property name="tokens" value="SWITCH_RULE, CASE_GROUP"/>
             <property name="limitedTokens" value="LITERAL_YIELD"/>
            <property name="maximumNumber" value="2"/>
        </module>
    </module>
</module>

$ java -jar checkstyle-10.17.0-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[WARN] TestClass.java:7:13: Count of 3 for 'SWITCH_RULE' descendant 'LITERAL_YIELD' exceeds maximum count 2. [DescendantToken]
Audit done.

Technically the first switch has a count of 2, and the inner switch has a count of 1. The violation is combining them and saying it is 3.

To me this seems like it still needs to be a new check.

@nrmancuso Please decide.

[nrmancuso]

It looks like DescendantToken doesn't have an easy way to configure depth. It will only stop nesting at a prefix count (default is basically infinity), so either we will be scanning into a inner switch rule and combining it's count with the outer count, or not scanning deep enough in nested if statements.

Excellent counterexample. Looks like we may not be able to avoid a new check for this, DescendantToken will not support various scenarios like @rnveach illustrated above. Also - this should be a simple check to implement.

We could also consider MatchXpathCheck. I am going to assign this to myself and see if I can come up with some reasonable expression before we do any work here.

[nrmancuso]

I was not able to come up with an XPath for this, but I have been thinking more about this check and believe that we should wait to see some interest from the community before we begin implementation. I could see a few different ways to approach this check (counting yields per switch, or yields for an individual case), plus how we should consider nested switches with yields, etc, and we should get some feedback from actual users on our ideas before we start any work.