Bug 328361 - [1.4][compiler] variable initialized within an assert expression are no longer reported as potential non initialized
Summary: [1.4][compiler] variable initialized within an assert expression are no longe...
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.7   Edit
Hardware: PC Windows 7
: P3 critical (vote)
Target Milestone: 3.7 M3   Edit
Assignee: Ayushman Jain CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-21 10:44 EDT by Olivier Thomann CLA
Modified: 2010-10-26 01:49 EDT (History)
1 user (show)

See Also:
Olivier_Thomann: review+

Attachments
proposed fix v1.0 + regression tests (5.82 KB, patch)
2010-10-21 15:29 EDT, Ayushman Jain CLA 		no flags Details | Diff
Updated patch (6.59 KB, patch)
2010-10-21 15:39 EDT, Olivier Thomann CLA 		no flags Details | Diff
updated patch v2 (5.46 KB, patch)
2010-10-21 15:58 EDT, Ayushman Jain CLA 		no flags Details | Diff
Show Obsolete (2) 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 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