Bug 162918 - [compiler] Illegal usage of a local inside a switch statement is not rejected
Summary: [compiler] Illegal usage of a local inside a switch statement is not rejected
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.3   Edit
Hardware: PC All
: P3 normal (vote)
Target Milestone: 3.3 M4   Edit
Assignee: Maxime Daniel CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-10-31 10:50 EST by Olivier Thomann CLA
Modified: 2006-12-11 15:05 EST (History)
1 user (show)

See Also:


Attachments
Fix plus test cases - option 1 (6.08 KB, patch)
2006-11-10 10:08 EST, Maxime Daniel CLA
no flags Details | Diff
Fix plus test cases - option 2 (4.03 KB, patch)
2006-11-10 10:13 EST, Maxime Daniel CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Thomann CLA 2006-10-31 10:50:47 EST
The following test case compiles fine in Eclipse where I would expect an error to be reported:
public class X {
 	public static void main(String[] args) {
 		switch(1) {
 			case 0: final int i = 1;
 			case i: System.out.println("SUCCESS");
 		}
 	}
}

In 1.6 mode with -preserveAllLocals, we get a VerifyError for inconsistent stack maps because the stack map that corresponds to the case i contains a local that is not defined before the switch statement.
All jumping positions of the switch statements (each case + default) must have the stack map. This is not the case with our code generation.

javac 1.4.2_12, 1.5.0_09, and 1.6b103 reject it with:
i may not have been initialized. javac 1.3.1 accepts the code.
Comment 1 Philipe Mulet CLA 2006-11-08 05:18:00 EST
Need to tune the flow analysis to reset data when entering the next catch block.
Only toplevel final variables need to be unset (since subscope vars are not even visible). Could be achieved by a single pass over the case statements, when we know we met a final local definition (others have zero influence).

We only fail the #foo1() scenario below.

public class X {
	void foo1() {
        switch(1) {
                case 0: 
                	final int i = 1;
                case i: // should complain: i not initialized
                	System.out.println(i); // should complain: i not initialized
        }
    }
	void foo2() {
        switch(1) {
                case 0: 
                	int j = 0;
                case 1:
                	System.out.println(j); // should complain: j not initialized
        }
    }
	void foo3() {
        switch(1) {
                case 0: 
                	class Local {};
                case 1: 
                	new Local(); // should complain: Local undefined
        }
    }
}
Comment 2 Maxime Daniel CLA 2006-11-09 06:02:04 EST
Added FlowAnalysisTest#34 to 36, of which only #35 is enabled.
The third scenario fails in 1.3 mode, but passes for higher java levels.
Comment 3 Maxime Daniel CLA 2006-11-09 08:22:25 EST
UnconditionalFlowInfo#isDefinitelyAssigned(LocalVariableBinding) merely assumes that any given constant variable is definitely assigned at any point in the code, regardless of the flow info upon which it is called. This bug shows a test case for which the assumption does not hold true, because there is a mean to drop in the middle of a statements list, in effect bypassing the initialization.
I suspect that this behavior is a mere optimization. Running full tests to confirm this. If nothing breaks, I will consider that the short circuit optimization must be removed.
Comment 4 Maxime Daniel CLA 2006-11-09 09:05:32 EST
Seems that javac changed its mind about the subject indeed. It was previously (up to 1.3) considered that constants would be inlined 'first', regardless of the flow of control. In other words, final int i = 1 would introduce a constant named i that would be visible wherever i was visible, and since it was a constant, it was initialized.
JLS 3 is not explicitly giving constants that privilege though, and javac 1.4+ rejects the case.
Hence seems that we should diffentiate our behavior depending on the source level here.
Philippe, what do you think?
Comment 5 Olivier Thomann CLA 2006-11-09 09:12:29 EST
I would consider this as a bug in 1.3 and fix it in a consistent manner for all compliances. In 1.6, this has to be rejected as it is not possible to end up with consistent stack maps.
Comment 6 Philipe Mulet CLA 2006-11-10 07:26:20 EST
I would make it source level dependent (or target level?).
I believe we do the same for the local type scenario to ease backward compatibility.
Comment 7 Philipe Mulet CLA 2006-11-10 07:27:31 EST
Now if too problematic for performance, we could consider addressing it for all versions (i.e. no source level check). I suspect we would penalize everything for checking options in a situation which never occurs.
Comment 8 Maxime Daniel CLA 2006-11-10 10:08:28 EST
Created attachment 53625 [details]
Fix plus test cases - option 1

This patch provides an effective implementation for a first option, in which we keep different behaviors between compliance 1.3 and compliance > 1.3. The only performance penalty we have is in case of erroneous code, which is acceptable.
Comment 9 Maxime Daniel CLA 2006-11-10 10:13:00 EST
Created attachment 53626 [details]
Fix plus test cases - option 2

I have observed that jdk1.3.1_16 only partially follows the 'constant is initialized' rule. More specifically, it accepts the i of 'case i', but reports an error against the i of 'System.out.println(i);'. This patch hence implements option 2 in which all compliance levels are aligned.
Patch currently under test, will be released if everything is ok.
Comment 10 Maxime Daniel CLA 2006-11-13 00:42:13 EST
Released for 3.3 M4.
Comment 11 Olivier Thomann CLA 2006-12-11 15:05:17 EST
Verified for 3.3M4 with I20061211-1119