Bug 134848 - [compiler][null] false positive after nested loop with break to explicit label
Summary: [compiler][null] false positive after nested loop with break to explicit label
Status: VERIFIED FIXED
Alias: None
Product: JDT
Classification: Eclipse Project
Component: Core (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 major (vote)
Target Milestone: 3.3 M3   Edit
Assignee: Maxime Daniel CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 139431 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-04-04 15:36 EDT by Eugene Kuleshov CLA
Modified: 2006-10-30 13:57 EST (History)
1 user (show)

See Also:


Attachments
Fix + test cases (3.18 KB, patch)
2006-10-04 11:34 EDT, Maxime Daniel CLA
no flags Details | Diff
Fix + test cases (3.32 KB, patch)
2006-10-05 02:24 EDT, 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 Eugene Kuleshov CLA 2006-04-04 15:36:32 EDT
I have very long method where null reference analyzer raise "variable may be null" warning.

Source code is at http://cvs.forge.objectweb.org/cgi-bin/viewcvs.cgi/asm/asm/src/org/objectweb/asm/ClassReader.java?annotate=1.50.2.17&only_with_tag=MUSTANG_SUPPORT

Warnings shown for variable mv on lines 1067 and 1073. Even so line 753 is checking that mv is not null.
Comment 1 Maxime Daniel CLA 2006-04-05 12:24:00 EDT
Reproduced with build N20060405-0010.
Comment 2 Maxime Daniel CLA 2006-04-06 04:03:52 EDT
Reduced the test case to:
public class X {
  void foo(Object o) {
    while (true) {   
      if (o != null) {
	o.toString(); // does not complain here
	loop: while (true) {
	  break loop;
	}
	o.toString(); // should not complain here either
      }
    }
  }
}

Added NullReferenceTest #456 (skipped for now).
Comment 3 Maxime Daniel CLA 2006-04-25 00:49:08 EDT
Post 3.2.
Comment 4 Maxime Daniel CLA 2006-05-09 05:02:45 EDT
*** Bug 139431 has been marked as a duplicate of this bug. ***
Comment 5 Maxime Daniel CLA 2006-09-26 13:17:31 EDT
This has to do with the way we stack contexts up to cope with labelled and unlabelled target contexts. Somehow, the following code snippets should get the same target context for the break statement, and this is currently not the case:
// a
while (true) {
  break;
}
// b
loop: while (true) {
  break;
}
// c
loop: while (true) {
  break loop;
}

Need to investigate further to decide how to reunify these or else put in place delegation mechanisms that would present them as unified to the algoritms involved.
Comment 6 Maxime Daniel CLA 2006-09-27 09:43:15 EDT
Sharing flow info instances between contexts is a no go because we very often replace one instance with another when manipulating flow infos (one reason is that we share a single DEAD_END instance for performance's sake, the other reason is that merge operations etc. do return a different instance). However, we can still climb the chain of contexts up and mark each element of the proper equivalent class (the labeled context, its parents that form a chain of labeled contexts, the looping context that is on the stack and has the labeled context as its parent).
This is not enough, though, because the current implementation of LabeledStatement goes far less than WhileStatement (and other loops) when it comes to reinjecting upstream info bits that are not negated by included loops. Would need loops to signal their best guess to enclosing contexts (including looping ones and labeled ones) so as to avoid to erase protections.
Will investigate doing more in the labeled statement than currently to reinject upstream info.
Comment 7 Maxime Daniel CLA 2006-10-04 11:34:03 EDT
Created attachment 51410 [details]
Fix + test cases

Pursued with success the path suggested at the end of comment 6.
The key is that when a loop only exits via a break to an explicit label, its flow info is DEAD_END, which fails to convey the null info that misses from the flow info at the break point. In such case, LabeledStatement now reinjects its upstream flow info before returning.
Patch currently under test.
Comment 8 Maxime Daniel CLA 2006-10-04 11:42:21 EDT
Comment on attachment 51410 [details]
Fix + test cases

A bit too simple, need to do a finer job at only impacting null info (in case of non reachable code).
Comment 9 Maxime Daniel CLA 2006-10-05 02:24:01 EDT
Created attachment 51454 [details]
Fix + test cases

Improved patch: now manages properly the case where the break flow info is unreachable.
Comment 10 Maxime Daniel CLA 2006-10-05 02:32:33 EDT
Released for 3.3 M3
Comment 11 Olivier Thomann CLA 2006-10-30 13:57:28 EST
Verified for 3.3 M3 using warm-up build I20061030-0010