Bug 318020

Comment 1 Karim Fatehi 2010-06-25 13:21:27 EDT

Created attachment 172790 [details]
Code that fails in 3.6 but works in 3.5.x and java.

If you use this class in an older version of eclipse, or try to compile using sun java compiler it works.

However it does not work with eclipse 3.6.

if you remove the line that has if(true) then it works. I have attached that file in another attachment.

Comment 2 Karim Fatehi 2010-06-25 13:23:39 EDT

Created attachment 172792 [details]
Code modified to succeed in 3.6

Now that I remove the if(true) the it works in eclipse 3.6

However eclipse shouldn't deviate from the way java compiler does this. Regression and therefore a bug.

Comment 3 Olivier Thomann 2010-06-25 13:43:57 EDT

Ayushman, please investigate.
This is a regression over 3.5.x so it must be fixed for 3.6.1.

Comment 4 Ayushman Jain 2010-06-28 08:04:18 EDT

While trying to fix this issue, I found that fixing this would break an existing behaviour wrt initializers : 

Currently, for the following we dont report any error
public class A{
  {
      if(true)
     throw new NullPointerException();
  }
  public A() {
  }
}

but for :

public class A{
  {
     throw new NullPointerException();
  }
  public A() {
  }
}

we report an error: Initializer does not complete normally.

Javac 1.6 and 1.7 - does the same. This seems to be a javac bug to me, with which eclipse has been inadvertently complying since a while. If we fix the isse reported here, we'll also be 'fixing' the behaviour wrt initializers, thus making eclipse compiler incompatible with javac in that area. I've reported the bug at http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6964607. Depending on the response, we'll be in a better position to take a call on how to fix this.

Comment 5 Ayushman Jain 2010-06-28 08:09:34 EDT

Created attachment 172893 [details]
proposed fix under test

The above patch fixes this case and also improves diagnosis of more than 150 existing tests, but breaks the eclipse compiler compatibility with javac for initializers. :(

Comment 6 Olivier Thomann 2010-06-28 14:22:37 EDT

I think the problem comes from another place.
We compile the following code as expected:
public class X {
  public static void main(String[] args) throws Exception {
    String temp;
    if (true) {
      throw new Exception("Not a string");
    }
    temp.trim();
  }
}

we report a dead-code warning for the "temp.trim();" expression statement.

We don't compile the following code:
public class X {
  public static void main(String[] args) throws Exception {
    String temp;
    Object temp2 = "test";
    if (temp2 instanceof String) {
      temp = (String) temp2;
    }
    else {
      if (true)
        throw new Exception("Not a string");
    }
    temp.trim();
  }
}
which should end up being the same thing for the temp local. The local is initialized or the code on which we report the error is dead.
I wonder if the problem doesn't come from the fact that we might "merge" the initialization state from the then statement with the one from the else block (that contains the if (true)) even if the else block ends up returning (throw exception).

Srikanth, what do you think ?

Comment 7 Olivier Thomann 2010-06-28 14:46:38 EDT

I think the problem comes from line 343 in org.eclipse.jdt.internal.compiler.flow.FlowInfo.

I am updating the title to reflect more the real issue.

Comment 8 Karim Fatehi 2010-06-28 14:53:18 EDT

Thank you Olivier for making the title better. I agree with your analysis.

Comment 9 Ayushman Jain 2010-06-29 11:11:17 EDT

(In reply to comment #6)

> I wonder if the problem doesn't come from the fact that we might "merge" the
> initialization state from the then statement with the one from the else block
> (that contains the if (true)) even if the else block ends up returning (throw
> exception).
> 
> Srikanth, what do you think ?

Olivier, the problem does indeed is a manifestation of merging of the then branch with the else branch containing if(true). Actually, all the branches that return or throw an exception should be marked as "dead end". In the given case, the else branch should hence be marked as dead end, which is not happening. Because of this, the else stream's initializations are getting merged with that of the then stream, thereby casting a doubt on the initialization of temp. The above patch fixes this.

(In reply to comment #7)

>I think the problem comes from line 343 in
>org.eclipse.jdt.internal.compiler.flow.FlowInfo.

I would say no, because line 343 will never have executed if the else stream was marked as DEAD_END in the first place (which is the right thing to do). It is getting executed currently because we are not able to identify else stream as DEAD_END.

Hence the fix.

Comment 10 Olivier Thomann 2010-06-29 12:16:52 EDT

I think your patch is too restrictive.
http://java.sun.com/docs/books/jls/third_edition/html/statements.html#14.21 clearly states that the if statement has a specific semantic.
I think with the fix you have more the HYPOTHETICAL case described in the JLS and not the ACTUAL case.

Did I miss something ?

Comment 11 Ayushman Jain 2010-06-30 08:41:23 EDT

Created attachment 173083 [details]
proposed fix v2.0

This fix makes the if statement analysis more coherent with the JLS spec. Earlier, we used to mark the IfElseStatementUnreachable flag for if(true) type statements. WIth the patch, we use Statement.isKnowDeadCodePattern() and thus dont mark the else block as unreachable. For code gen optimization, the boolean constant will suffice.

Also the condition in the mergeIfElse....() method now depends on this flag being set or not. This eliminates the if(true) and if(false) type of cases.

Note that I also observed that the flowInfo was getting affected when any of its initsWhenTrue or initsWhenFalse were getting modified (i.e. getting set to unreachable). Further investigation showed that we return FlowInfo from Literal.analyseCode() without creating a copy. So this has also been fixed.

This patch passes all tests.

Comment 12 Olivier Thomann 2010-06-30 10:51:48 EDT

Ayushman, please provide regression tests for it.
You might want to create a new InitializationTest class inside the compiler regression test suite.

Comment 13 Srikanth Sankaran 2010-07-01 09:54:54 EDT

I agree with the comment about the special treatment needed
for if (true/false) constructs.

Ayush, please post a cumulative patch with tests for review,
Thanks.

Comment 14 Ayushman Jain 2010-07-07 09:04:32 EDT

Created attachment 173643 [details]
proposed fix v2.0 + regression tests

Here's a patch with added tests. It has a minor change from the previous one - In the previous one I'd modified Literal#analyseCode() to return a copy of the flowInfo, which isn't required. We usually create a copy when we're calculating another flowInfo from the given flowInfo, or when initializing a new one with the info from an old one. So in the Literal.analyseCode() method, there's no need to return a copy of the info. It is actually in IfStatement#analyseCode() that we calculate the initsWhenTrue and initsWhenFalse. We should use a copy of the flowInfo for initsWhenFalse, just like we do for initsWhenTrue by using safeInitsWhenTrue().

Comment 15 Ayushman Jain 2010-07-08 04:16:16 EDT

Created attachment 173745 [details]
proposed fix v2.0 + updated regression tests

Oops, I'd added the tests at the wrong place. 
Created a new InitializationTests suite per comment 12.

Comment 16 Olivier Thomann 2010-07-12 12:10:06 EDT

Ayushman, please release for 3.7M1.
The review is required for 3.6.1, but you can go ahead for 3.7M1.
Patch looks good.

Comment 17 Ayushman Jain 2010-07-17 14:29:46 EDT

*** Bug 320046 has been marked as a duplicate of this bug. ***

Comment 18 Srikanth Sankaran 2010-07-19 05:02:30 EDT

I have reviewed the changes and spent some time comparing
the behavior of javac and eclipse on the various tests you
have and didn't find anything wrong.

But I don't understand the use of the (misspelt) method 
isKnowDeadCodePattern - 

In all of the following cases:

(1) if (true) 
         throw new Exception("Not a string"); 

(2) if (false) 
         throw new Exception("Not a string"); 

(3) private static final boolean DEBUG = true;
    if (DEBUG)
         throw new Exception("Not a string"); 

(4)  private static final boolean DEBUG = false;
    if (DEBUG)
         throw new Exception("Not a string");

either the method doesn't get called at all (from the new site)
or it always returns false.


So you need to explain to me why this works and is the right fix.

I would like to know what exactly broke the original working behavior
and view that in connection with the new code change.

Comment 19 Ayushman Jain 2010-07-20 05:16:19 EDT

Created attachment 174719 [details]
proposed fix v2.0 updated

(In reply to comment #18)

> But I don't understand the use of the (misspelt) method 
> isKnowDeadCodePattern - 
> 
> In all of the following cases:
> 
> (1) if (true) 
>          throw new Exception("Not a string"); 
> 
> (2) if (false) 
>          throw new Exception("Not a string"); 
> 
> (3) private static final boolean DEBUG = true;
>     if (DEBUG)
>          throw new Exception("Not a string"); 
> 
> (4)  private static final boolean DEBUG = false;
>     if (DEBUG)
>          throw new Exception("Not a string");
> 
> either the method doesn't get called at all (from the new site)
> or it always returns false.

This was an experimental code and was supposed to have been removed. So removed this in the new patch. 
Here's an explanation of the fix in its final shape:

there are basically two ways of marking either a then construct or an else construct as unreachable:

1. Set the flag for the relevant construct as FlowInfo.UNREACHABLE - This is done only for our null analysis purposes and has no effect on the code generation.
2. Set the flag on the parent ifStatement - either ASTNode.IsElseStatementUnreachable or ASTNode.IsThenStatementUnreachable - see that these flags are checked in code generation.

Earlier in mergedOptimizedBranchesIfElse, we were using the UNREACHABLE flag to determine whether some construct is unreachable and whether its potentialy initialised variables should be merged into the main stream or not. But this approach brings in many false positive cases such as 

 Object temp2 = new String("test");
    if (temp2 instanceof String) {
      temp = (String) temp2;
    }
    else {
      if (true)
        throw new Exception("Not a string");
    }

Here after having analysed the else construct, the compiler can very well see that the else statement is going to end up at a dead end (throw clause). And we consider code which is going to end up at a dead end as being UNREACHABLE. So the else constructs UNREACHABLE flag is now set. Now the documentation at line335 in mergedOptimizedBranchesIfElse says

		// We don't do this if both if and else branches themselves are in an unreachable code
		// or if any of them is a DEAD_END (e.g. contains 'return' or 'throws') 

but here the else part is indeed a dead end. So the UNREACHABLE bit  is not a perfect indicator of a non - "dead end", yet unreachable branch. Even dead end branches have UNREACHABLE bit set and we want to omit those cases here. So IfStatement.ElseStatementIsUnreachable is a better indicator since it does not get set when a branch is marked dead end.

This is the main motive behind the patch.

Comment 20 Olivier Thomann 2010-07-20 16:28:32 EDT

The test suite InitializationTests needs to be added to the compiler test suite.

Comment 21 Ayushman Jain 2010-07-21 05:34:19 EDT

Created attachment 174828 [details]
final patch

(In reply to comment #20)
> The test suite InitializationTests needs to be added to the compiler test
> suite.
Done.

Released in HEAD for 3.7M1
Released in R3_6_maintenance for 3.6.1.

Added tests org.eclipse.jdt.core.tests.compiler.regression.InitializationTests.*

Comment 22 Ayushman Jain 2010-07-21 06:54:42 EDT

Fixed.

Comment 23 Karim Fatehi 2010-07-21 09:38:23 EDT

thanks, you guys are real heroes. where can i see the release schedule for 3.6.1? i really like the other features of 3.6 but i cannot release it for use at the company until this fix makes it through.

Comment 24 Ayushman Jain 2010-07-21 09:56:29 EDT

(In reply to comment #23)
> thanks, you guys are real heroes. where can i see the release schedule for
> 3.6.1? i really like the other features of 3.6 but i cannot release it for use
> at the company until this fix makes it through.

3.6.1 is planned for 24-09-2010. Here's the timeline: http://www.eclipse.org/projects/timeline/index.php?projectid=eclipse

Thanks! :)

Comment 25 Olivier Thomann 2010-07-21 09:59:26 EDT

(In reply to comment #23)
> thanks, you guys are real heroes. where can i see the release schedule for
> 3.6.1? i really like the other features of 3.6 but i cannot release it for use
> at the company until this fix makes it through.
If you don't want to wait for 3.6.0, you can always use today's M-build. It contains the fix.

Comment 26 Richard 2010-07-23 04:21:27 EDT

Also from my side  thanks a lot for taking the time to look at and fix this issue and sorry for filing the duplicate. I tried the M version from 21st of July and the problem has gone.

Comment 27 Karim Fatehi 2010-07-23 09:41:52 EDT

will this fix also be making it to the upcoming v4 release?

thanks.

Comment 28 Olivier Thomann 2010-07-23 10:29:29 EDT

(In reply to comment #27)
> will this fix also be making it to the upcoming v4 release?
No. It will be available in 3.6.1.

Comment 29 Karim Fatehi 2010-07-23 10:44:15 EDT

(In reply to comment #28)
> (In reply to comment #27)
> > will this fix also be making it to the upcoming v4 release?
> No. It will be available in 3.6.1.

so will we have to open a new bug report once v4 is out and if the bug is also there?

Comment 30 Olivier Thomann 2010-07-28 12:16:05 EDT

(In reply to comment #29)
> so will we have to open a new bug report once v4 is out and if the bug is also
> there?
No. The E4 sdk available later today is based on Eclipse 3.6.0. So this is the same version of JDT/Core as for 3.6.0. This being said, the fix for this issue will be available for 3.7M1 and 3.6.1. If there is a build of the e4 sdk to use 3.6.1 it will get the fix for free.

Comment 31 Srikanth Sankaran 2010-08-04 01:45:33 EDT

Verified for 3.7 M1 using build I20100802-1800

Comment 32 Srikanth Sankaran 2010-08-26 02:53:22 EDT

Verified for 3.6.1 RC2 using Build id: M20100825-0800