Community
Participate
Working Groups
Follow up from bug 575593 comment 2 ... "In the vicinity of bug 575051 I learned that after 'default' the selector is known to be non-null. It would be nice to let our implementation use that fact, but unfortunately we have no distinct flowInfo that models just the default branch. Rather all cases are mingled into the cumulative 'caseInits'. I believe this is owed to legacy switches, but in '->' cases separate flowInfos per branch would make a lot of sense, no? "
New Gerrit change created: https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/184428
After some more debugging I think I have a somewhat better grip on the issue now. TL;DR: most of my changes were unnecessary, I reduced them to a state that may or may not help code comprehension. Goal: I wanted to set some null info into the flowInfo of exactly one arm ("default ->") of a new switch, without affecting other arms at all. Perceived obstacle: I only saw one 'caseInits' being incrementally updated across the entire switch block. Thus I was afraid, that injected null info might spill into undesired places. Legacy switch: from my today's understanding, switch flow analysis consists of three things: (1) caseInits indeed collects info linearly from any statements seen, which is how we account for fall through cases. (2) break and such sever the linear connection by unconditionally re-setting caseInits to DEAD_END. (3) the flowInfo after the entire construct is assembled mainly by merging info from all possible exits into SwitchFlowContext.initsOnBreak. (3.1) if the last block lacks a break, the info from that block (=terminal caseInits) still needs to be merged with initsOnBreak. New switches: although those don't have fall through cases, they are coerced into the above schema (2.new): more elements can set caseInits to DEAD_END: - a yield statement (explicit or implicit) - an implicit break terminating the block of a switch rule. (3.new): after analysing all arms, caseInits is guaranteed to be DEAD_END, so merging with initsOnBreak is a no-op, equivalent to directly using initsOnBreak. Question 1: is my above interpretation correct? In the gerrit, patch set #4 I have reduced my changes (outside the immediate null issues of 'default') to making short-cuts from (1-3) for legacy switches implicit: * at the start of a case we explicit re-start from the incoming flowInfo * at the end of the switch statement we directly fetch initsOnBreak (no need to merge) Question 2: are these 'short cuts' legitimate? Question 3: do those changes help comprehensibility or are they rather seen as cluttering the code? More doubt: From recent reading I was confused about different options to concatenate cases, labels, patterns. Supporting the doubt: I openly admit that I haven't the slightest what is the implication of this line of code: fallThroughState = this.containsPatterns ? FALLTHROUGH : CASE; do pattern switches need to handle some kind of fallthrough? How so? Until this doubt is cleared up, I should better not make any changes beyond the basic updating of null info :)
(In reply to Stephan Herrmann from comment #2) > Supporting the doubt: I openly admit that I haven't the slightest what is > the implication of this line of code: > fallThroughState = this.containsPatterns ? FALLTHROUGH : CASE; > > do pattern switches need to handle some kind of fallthrough? How so? Good to have you looking at this, Stephan! As the person who wrote that (and also the person who happens to know very little about the flow analysis), let me explain the intention here. This is basically done to mimic the Javac behavior that reports an illegal fall-through when we have code like this: case Integer s : System.out.println(); case String s1: System.out.println("String"); So, it's like an override of the fall-through state the moment we see a pattern variable in a case label. The other place you might want to look at (the one I am less convinced about) is the the change I made to CaseStatement.analyseCode(BlockScope, FlowContext, FlowInfo), which covers the following code: case 1, Integer i -> System.out.println(o); I assume that we may not yet have flow analysis handling such cases. Basically, the error reporting of IllegalFallThroughToPattern() here hardly uses any flow analysis.
(In reply to Jay Arthanareeswaran from comment #3) > (In reply to Stephan Herrmann from comment #2) > > Supporting the doubt: I openly admit that I haven't the slightest what is > > the implication of this line of code: > > fallThroughState = this.containsPatterns ? FALLTHROUGH : CASE; > > > > do pattern switches need to handle some kind of fallthrough? How so? > > Good to have you looking at this, Stephan! > > As the person who wrote that (and also the person who happens to know very > little about the flow analysis), let me explain the intention here. > > This is basically done to mimic the Javac behavior that reports an illegal > fall-through when we have code like this: > > case Integer s : System.out.println(); > case String s1: System.out.println("String"); > > So, it's like an override of the fall-through state the moment we see a > pattern variable in a case label. Thanks, so this applies to "legacy" switches (not using '->'). But isn't that the same fall-through situation as for case 1: System.out.println(); case 2: System.out.println("2"); ? Our should your example look more like this: case Integer s: case String s1: System.out.println(s); I can see why this is illegal, and this would not be covered by the old implementation. OK. > The other place you might want to look at (the one I am less convinced > about) is the the change I made to CaseStatement.analyseCode(BlockScope, > FlowContext, FlowInfo), which covers the following code: > > case 1, Integer i -> System.out.println(o); > > I assume that we may not yet have flow analysis handling such cases. > Basically, the error reporting of IllegalFallThroughToPattern() here hardly > uses any flow analysis. Right, that's one of the combinations that I'm still unfamiliar with :) But if the rule is: only the first "constantExpression" can legally be a pattern, then your simply check looks totally OK to me. "Real flow analysis" would only be needed if there is some branching logic *within* a case statement or such. But since this is a simple list, I see no fault in your implementation. It actually indicates that new style switch is in fact simpler to process than old style, right? I was surprised by the outcome of this: void m(Object o) { switch (o) { case Number n, null -> System.out.print("n:"+n); case Object o1 -> System.out.println("o:"+o1); } } When invoked with 'null' it prints "n:null". Please verify my interpretation: the type pattern "Number n" does not match null (it is not an any pattern), but the subsequent 'null' pattern matches, which causes n to be bound to null nevertheless ("retroactively"). So even if evaluation of a list of patterns is or-ed, the pattern variable is always assigned. Is that it? After all, this discussion helped me to discover & file bug 575686 and bug 575687 :)
Anyway, the combination of old & new switches is more intricate than I thought on first look. Hence I'll leave the flowInfo handling as it is and add only the special null-treatment for the default pattern. Re-dedicating the bug accordingly.
Gerrit change https://git.eclipse.org/r/c/jdt/eclipse.jdt.core/+/184428 was merged to [BETA_JAVA17]. Commit: http://git.eclipse.org/c/jdt/eclipse.jdt.core.git/commit/?id=7040ea28be4a4071fc9134dca9eac6726b44cd3e
(In reply to Stephan Herrmann from comment #4) > Our should your example look more like this: > case Integer s: > case String s1: System.out.println(s); > > I can see why this is illegal, and this would not be covered by the old > implementation. OK. Yes, this explains better than the one I provided. > But if the rule is: only the first "constantExpression" can legally be a > pattern, then your simply check looks totally OK to me. Yes, I can confirm that. > When invoked with 'null' it prints "n:null". > Please verify my interpretation: the type pattern "Number n" does not match > null (it is not an any pattern), but the subsequent 'null' pattern matches, > which causes n to be bound to null nevertheless ("retroactively"). So even > if evaluation of a list of patterns is or-ed, the pattern variable is always > assigned. Is that it? Yes and here's the relevant spec: "If the value is the null reference, then we determine the first (if any) switch label in the switch block that applies to the value as follows: A switch label that has a null case label element applies. In addition, any pattern variables declared by patterns appearing in the switch label are initialized to the null reference." > > After all, this discussion helped me to discover & file bug 575686 and bug > 575687 :) Thanks! Will look into those soon.
