Bug 328361

Summary: [1.4][compiler] variable initialized within an assert expression are no longer reported as potential non initialized
Product: [Eclipse Project] JDT Reporter: Olivier Thomann <Olivier_Thomann>
Component: CoreAssignee: Ayushman Jain <amj87.iitr>
Status: VERIFIED FIXED QA Contact:
Severity: critical    
Priority: P3 CC: srikanth_sankaran
Version: 3.7Flags: Olivier_Thomann: review+
Target Milestone: 3.7 M3   
Hardware: PC   
OS: Windows 7   
Whiteboard:
Attachments:
Description Flags
proposed fix v1.0 + regression tests
none
Updated patch
none
updated patch v2 none

Description Olivier Thomann CLA 2010-10-21 10:44:04 EDT
The following test case should fail to compile:
public class X {
    static final int i;
    static {
        assert (i = 0) == 0;
        System.out.println(i);
    }
}

This is a regression over 3.6.2. This is a must fix for 3.7M3.
Comment 1 Ayushman Jain CLA 2010-10-21 15:29:23 EDT
Created attachment 181440 [details]
proposed fix v1.0 + regression tests

This bug shows that using addInitializationsFrom(..) to add the initialization info from assert into the main stream wasn't the correct approach, since initializations taking place inside assert may/may not be honoured depending on whether asserts are enabled.

This patch uses mergedWith(..) instead, which instead of adding initializations blindly from the assert, combines those from the main stream and assert stream. This helps us in filtering variables which may not have been initialized in the main stream.
Comment 2 Ayushman Jain CLA 2010-10-21 15:31:42 EDT
This patch passes all compiler tests.
Olivier, requesting a quick review. Thanks!
Comment 3 Olivier Thomann CLA 2010-10-21 15:34:05 EDT
I think the test should check for this.complianceLevel >= ClassFileConstants.JDK1_4 instead of this.complianceLevel >= ClassFileConstants.JDK1_5 since assert are defined in 1.4.

Beside this, it looks ok.

Could you please move the tests to a public test suite ? I would recommend the tests to be moved to the class org.eclipse.jdt.core.tests.compiler.regression.AssertionTest. Then you don't actually need to check the compliance level.
Comment 4 Olivier Thomann CLA 2010-10-21 15:39:46 EDT
Created attachment 181442 [details]
Updated patch

Same patch with the tests moved to the AssertionTest suite.
Comment 5 Ayushman Jain CLA 2010-10-21 15:58:15 EDT
Created attachment 181445 [details]
updated patch v2

Just noticed that there were two duplicate tests in the above patch - test018 and 020. So just removed one.
Comment 6 Ayushman Jain CLA 2010-10-22 00:15:35 EDT
Released in HEAD for 3.7M3
Comment 7 Srikanth Sankaran CLA 2010-10-26 01:49:47 EDT
Verified for 3.7 M3 using Build id: I20101025-1800