Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 545518 - [12] Flow analysis is skipped for multi-constant case statement
Summary: [12] Flow analysis is skipped for multi-constant case statement
Status: RESOLVED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 4.10   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: BETA J12   Edit
Assignee: Jay Arthanareeswaran CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 545516 545525 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-03-19 02:04 EDT by Jay Arthanareeswaran CLA
Modified: 2019-03-25 23:36 EDT (History)
2 users (show)

See Also:
manoj.palat: review+

Attachments
test case (1.42 KB, patch)
2019-03-19 06:31 EDT, Manoj N Palat CLA 		no flags Details | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Jay Arthanareeswaran CLA 2019-03-19 02:04:02 EDT 
Consider the following case in compliance 12 with preview feature usage warning ignored:

public class X {
    public void main(String [] args, final String argument) {
	switch(args[0]) { 
      case "ABC", (false ? (String) "c" : (String) "d") : break;
	}
    }
}

There should be a dead code warning, but there isn't. If you shuffle the constants, this is reported. The reason is we don't call analyseCode() on all the constant expressions inside CaseStatement.analyseCode(...). I have a fix ready.
Comment 1 Eclipse Genie CLA 2019-03-19 02:56:38 EDT 
New Gerrit change created: https://git.eclipse.org/r/138986
Comment 2 Jay Arthanareeswaran CLA 2019-03-19 02:57:53 EDT 
*** Bug 545516 has been marked as a duplicate of this bug. ***
Comment 3 Manoj N Palat CLA 2019-03-19 04:22:18 EDT 
+1 for this issue..
However got another bug 545525 while testing  related with multi-constants - However, this patch can be committed since that error is happening independent of this patch.
Comment 5 Manoj N Palat CLA 2019-03-19 05:55:44 EDT 
A few issues - realized a little later:

the check needs to be:
	if (!this.parsingJava12Plus) {
//		problemReporter().caseStatementWithArrowNotBelow12(caseStatement);
		problemReporter().previewFeatureNotSupported(); //$NON-NLS-1$
	} else if (!this.options.enablePreviewFeatures){
		problemReporter().previewFeatureNotEnabled(); //$NON-NLS-1$
	} else {
		if (this.options.isAnyEnabled(IrritantSet.PREVIEW)) {
			problemReporter().previewFeatureUsed(
		}
	}
and the case.constantExpressions should be assigned only after all the checks. This is the root causeof bug 545387 as well.

The <= 12 flag in the current patch does n't cover all these scenarios
Comment 6 Manoj N Palat CLA 2019-03-19 05:58:48 EDT 
*** Bug 545525 has been marked as a duplicate of this bug. ***
Comment 7 Manoj N Palat CLA 2019-03-19 06:01:09 EDT 
(In reply to Manoj Palat from comment #5)

> checks. This is the root causeof bug 545387 as well.
> 
Bug 545525 is the correct bug number of the duplicate and not the one above. (blame it on release pressure :()
Comment 8 Manoj N Palat CLA 2019-03-19 06:31:28 EDT 
Created attachment 277929 [details]
test case

@Jay: Please find a test case patch. Feel free to add this if it helps.
Comment 9 Eclipse Genie CLA 2019-03-19 06:42:53 EDT 
New Gerrit change created: https://git.eclipse.org/r/139017
Comment 10 Jay Arthanareeswaran CLA 2019-03-19 10:35:55 EDT 
(In reply to Manoj Palat from comment #8)
> Created attachment 277929 [details] [diff]
> test case
> 
> @Jay: Please find a test case patch. Feel free to add this if it helps.

Thanks Manoj! I had a similar test case, but it did point out that I had some unnecessary code in my version of test. I have fixed it now.
Comment 11 Jay Arthanareeswaran CLA 2019-03-19 13:03:22 EDT 
A word about the failures in FormatterRegressionTests: Formatter/Rewrite now expect the expressions() to return a non empty list and if we don't find, we fal back to the legacy expression. And looks like there's a difference in how the formatter treats the code in presence of expressions() and otherwise. This can be fixed, but I couldn't figure where in formatter this is done and decided to simply set the expressions (in compiler ast) regardless of the compliance level. The test case mentioned in bug 545525 is taken care of by rejecting the code at < 12.

I am OK to revert the last piece of change in Parser#consumeCaseLable() and fix the formatter, but I don't see any harm in going with the latest patch.

Manoj, please take a look. TIA!
Comment 12 Manoj N Palat CLA 2019-03-19 20:02:15 EDT 
(In reply to Jay Arthanareeswaran from comment #11)
> 
> Manoj, please take a look. TIA!

Thanks Jay. Code looks ok and my sanity tests also did not reveal any issue. 
In the latest build run neither do I see a failure in the rewriting test nor do I see any change in rewriting tests in the patch, but I see you have mentioned rewrite failures. can you please explain?
If there are no rewrite failures, but only formatter issues, we can go ahead and commit the patch with a follow up bug on the formatter issue.
+1 if that is the case.
Comment 13 Jay Arthanareeswaran CLA 2019-03-19 22:06:00 EDT 
(In reply to Manoj Palat from comment #12)
> (In reply to Jay Arthanareeswaran from comment #11)
> > 
> > Manoj, please take a look. TIA!
> 
> Thanks Jay. Code looks ok and my sanity tests also did not reveal any issue. 
> In the latest build run neither do I see a failure in the rewriting test nor
> do I see any change in rewriting tests in the patch, but I see you have
> mentioned rewrite failures. can you please explain?

I meant to say the change might affect rewrite as well if they are expecting expressions() always in AST > 12 (regardless of how many case constants). But then I don't see any failures. Probably they handle it better than the formatter, I suppose. Either way, the latest patch should make sure that those clients are not affected now.