Comment 1 kiril mitov 2006-05-19 12:01:36 EDT

Created attachment 42031 [details]
my eclipse configuration (in UTF-8)

This is my eclipse configuration. The file is in UTF-8.

Comment 2 Olivier Thomann 2006-05-19 12:03:54 EDT

I'll investigate.

Comment 3 Olivier Thomann 2006-05-19 12:05:55 EDT

Could you please provide the compiler's preferences of the project where this is failing?
It would also help a lot if you could provide a test case. Ideally the compilation unit where the problem occurs.
Thanks.

Comment 4 Olivier Thomann 2006-05-19 12:08:57 EDT

This would mean that you have a foreach statement with null as its action.
I don't see how this is possible.
Could you please try with the JIT disabled?

Comment 5 kiril mitov 2006-05-19 12:35:25 EDT

Created attachment 42040 [details]
jdt.core.prefs

The compiler settings for the project.

Comment 6 kiril mitov 2006-05-19 12:38:51 EDT

I have start my eclipse with ./eclipse -Djava.compiler=.
I will work this way for a couple of days to see if this will help.

Comment 7 Olivier Thomann 2006-05-19 12:40:17 EDT

Would it be possible to get the project as well?
Is it 100% reproducable?

Since you said this doesn't occur on 32 bits machine, I wonder if this is not a VM bug.
You might want to try a different VM.

Comment 8 kiril mitov 2006-05-19 12:41:24 EDT

Providing a test case is difficult for me because I did not notice on which file does the error occured. But from now on I will be more careful to notice that and create an attachement with this file on the first opportunity.

Comment 9 David Audel 2006-05-19 12:44:29 EDT

I can reproduce the stacktrace with the following test case

public class X {

	void bar(Collection<String> c) {
		for(String s: c) {
			try {
				
			}
		}
	}
}

But when the NPE occurs my workbench isn't closed.

Comment 10 kiril mitov 2006-05-19 12:45:40 EDT

Trying a different VM was the first thing to do when I saw the problem does not occur on 32 bit machines. 
I have tried sun-jdk-1.5.0.06, sun-jdk-1.5.0.05, sun-jre-bin-1.5.0.06 (I have not installed them especially for this problem - I just have them installed).

Comment 11 Olivier Thomann 2006-05-19 12:51:11 EDT

I am not sure that the java.lang.OutOfMemoryError: PermGen space and the NPE are related.

Comment 12 kiril mitov 2006-05-19 12:53:02 EDT

Yes the test case in comment_9 from David Audel  really cause the exception.
But this time the workbench does not close??? Strange... 

But having this code in the file and scrolling throught this piece of code by hiding it, clicking on the file and then showing it makes the exception be thrown many times...
Just scrolling does not throw the exception. You have to hide the foreach, then click on the file and then scroll to the foreach...

Comment 13 kiril mitov 2006-05-19 13:02:11 EDT

With the test case of comment_9 the exception is thrown also on 32 bits machine. I just tried it...

Comment 14 Olivier Thomann 2006-05-19 13:15:27 EDT

Martin, Philippe,

This is a case where the statement recovery failed and the action of the foreach statement is null.
We could try to create an fake empty statement, but in this case we would need to find a "good" position.
We could use the end position of the parent node since we need to ensure that the positions of this node is inside the position of its parent.
What do you think?

Comment 15 David Audel 2006-05-19 13:45:27 EDT

I confirm the explanation of Olivier.

The fake empty statement with the end of the 'foreach' as position would be good.
With a test case like
public class X {
  void bar(Collection<String> c) {
    for(String s: c) {
      try {
        bar();
      }
    }
  }
}
the resulting ast would be
public class X {
  void bar(Collection<String> c) {
    for(String s: c);
    {
       bar();
    }
  }
}

This ast isn't correct but as the statement recovery has failed, the ast cannot be correct.

For the test case of comment 9, the statement recovery can probably be fixed, but the statement recovery can failed for another reason. So the ASTConverter should add the protection suggested by Olivier even if we fix the test case scenario.

Note: the same problem cannot occurs for 'for', 'do' or 'while'.

Comment 16 Olivier Thomann 2006-05-19 13:52:09 EDT

I'll prepare a patch that will implement the proposal in comment 14.

Comment 17 Olivier Thomann 2006-05-19 14:22:01 EDT

Created attachment 42057 [details]
Proposed fix

Comment 18 Olivier Thomann 2006-05-19 14:22:46 EDT

Created attachment 42059 [details]
Regression test

Comment 19 Olivier Thomann 2006-05-19 15:43:03 EDT

With David's patch (he will post it after more testing), I get a better recovered tree. The for has a recovered block that contains a recovered try.
I believe that both patches are useful. David's one covers this specific case and my patch makes the ast conversion more resilient to error or missing recovery in the error recovery.
I would update the regression test if the patch from David is being released.

If David is confident with his patch, I would recommend it for 3.2 since this impact the reconciling (editing inside a CU and not just incorrect code).

Comment 20 Philipe Mulet 2006-05-20 07:16:04 EDT

Candidate for RC6

Comment 21 Philipe Mulet 2006-05-22 11:17:05 EDT

After discussing with David, I would suggest we address the NPE for RC6, and defer changing the statement recovery, since other similar cases need to be investigated, and recovered AST may be discussed (ie. is "try {...}" to be recovered into "try{...}finally{}" or rather as a block with no try ?).
Also, the recovery issue is orthogonal, David pls enter a separate PR for it.

+1 for 3.2RC6. The NPE fix looks good.

Martin/Dani - pls cast your vote.

Comment 22 Philipe Mulet 2006-05-22 11:19:04 EDT

Mike - pls cast your vote

Overall, the current behavior is a regression over 3.1. No client may notice the DOM AST difference compared to 3.1, as in 3.1, there was no statement recovery, and thus offending method body would have been empty.

Comment 23 David Audel 2006-05-22 11:25:47 EDT

I opened a new bug report for the recovery problem with try statement (bug 143001).

Comment 24 Martin Aeschlimann 2006-05-23 12:20:41 EDT

I'm confused about the statement
  int endPosition = parent.getStartPosition() + parent.getLength() - 1;
That would point to the last character (inside) the parent statement
Can the node make any assumptions about the parent and it's length?
What if the parent is a block and has more than one child?
Important for AST clients is that all child positions are in order and never overlap.

Comment 25 Olivier Thomann 2006-05-23 12:23:40 EDT

I don't see how a statement with a length 0 can overlap with any other statement.

Comment 26 Olivier Thomann 2006-05-23 12:24:07 EDT

parent.getStartPosition() + parent.getLength() - 1 is the last inclusive position of the parent.

Comment 27 Dani Megert 2006-05-23 12:26:23 EDT

If a client requests and AST without recovery will it still go through ASTConverter.createFakeEmptyStatement(...) (if not, where can I see this) and if so, will still go through the ASTRecoveryPropagator code? Or in simpler words: how is it guaranteed that the two changes are always called/used together?

Second question: is it 100% impossible that in  ASTRecoveryPropagator.visit(EmptyStatement) endPosition gets < 0?

Comment 28 Martin Aeschlimann 2006-05-23 12:33:34 EDT

The fake node marked with *

a.) parent [0, 5]
  child 1 [0, 5]
  child 2* [4, 0]   -> child 2 is overlapping with child 1

b.) parent [0, 5]
  child 1 [0, 2]
  child 2* [4, 0]
  child 3 [3, 2]
                    -> child 2 is in wrong order:
                       it's range must be between 2 and 3

Comment 29 Olivier Thomann 2006-05-23 12:36:56 EDT

(In reply to comment #27)
> If a client requests and AST without recovery will it still go through
> ASTConverter.createFakeEmptyStatement(...) (if not, where can I see this) and
> if so, will still go through the ASTRecoveryPropagator code? Or in simpler
> words: how is it guaranteed that the two changes are always called/used
> together?
If there is no statement recovery, the createFakeEmptyStatement() cannot be called with null.

Comment 30 Olivier Thomann 2006-05-23 12:47:29 EDT

What positions would you suggest for this fake node?

Comment 31 Martin Aeschlimann 2006-05-23 13:09:21 EDT

I think you have to look at your left sibling at take its end position (and length 0)

a.) parent [0, 5]
  child 1 [0, 5]
  child 2* [5, 0]

b.) parent [0, 5]
  child 1 [0, 2]
  child 2* [2, 0]
  child 3 [3, 2]

b.) parent [0, 5]
  child 1* [0, 0]
  child 2 [0, 3]
  child 3 [3, 2]

Comment 32 Olivier Thomann 2006-05-23 13:13:36 EDT

We could also don't set any position for the fake empty statement, but this would break the assumption that children are always included in the range of their parent except if we have a way to tag this node to say that it might not follow this rule.
How do we do it with the ast rewrite? What position do you give to inserted nodes ?

I don't see how:
a.) parent [0, 5]
  child 1 [0, 5]
  child 2* [5, 0]

are nodes with valid positions. 5 is not within the range of the parent.
the length is exclusive. So the parent contains positions from 0 to 4 inclusive. 5 cannot be a starting position of one of the children.

Comment 33 Martin Aeschlimann 2006-05-23 13:22:39 EDT

The inserted nodes in the AST have -1, 0. It doesn't matter there, but so far for AST's createdby the ASTParser we could assume that all node have a position.

Case a.) is of course dicussable but at least I think it wouldn't break our (and the ASTRewriter's) assumptions.

Comment 34 Olivier Thomann 2006-05-23 13:30:09 EDT

(In reply to comment #27)
> Second question: is it 100% impossible that in 
> ASTRecoveryPropagator.visit(EmptyStatement) endPosition gets < 0?
To get endPosition < 0, this means that the parent is empty at the position 0 (start = 0 and length = 0). I can test it, but I don't believe this is a valid case.

Comment 35 Olivier Thomann 2006-05-23 13:42:33 EDT

The only other solution I can see is to propagate the null back up and completely remove the "wrong" statement.
What do you prefer?

Comment 36 Mike Wilson 2006-05-23 13:56:02 EDT

will this discussion lead to an updated version of the patch?

Comment 37 Olivier Thomann 2006-05-23 14:02:28 EDT

If necessary, yes.
We have two approaches here:
1) Try to get the "right" sibling and update the positions of the fake empty statement with these positions.
2) Don't create a fake empty statement for the null case. This needs to propagate the null back up to be removed from the right block. Then no positions need to be updated since the statement doesn't exist anymore after the conversion.

If the statement is not null, the existing cases where we create fake empty statements are perfect fine as is. The null case can only happen during the recovery. I will attach a new patch that reflects this other approach.

Comment 38 Olivier Thomann 2006-05-23 14:09:13 EDT

Created attachment 42294 [details]
New patch

This patch is removing all statements that are converted to null (handling of the createFakeEmptyStatement returning a null value).
Let me know which one you prefer. Of course the original test case is also fixed with this patch. The method body ends up being empty in the following case:

public class X {

        void bar(Collection<String> c) {
                for(String s: c) {
                        try {

                        }
                }
        }
}

Martin and Markus, let me know if this affects JDT/UI. All DOM test cases from JDT/Core are passing with this patch.

Comment 39 Olivier Thomann 2006-05-23 14:26:38 EDT

I believe that the second patch is safer. Removing the culprit statement leaves an AST tree well formed.
Should the block be marked as MALFORMED? The ast recovery propagator is already tagging it as RECOVERED.

Comment 40 Olivier Thomann 2006-05-23 15:10:40 EDT

Marin, Dani, Philippe and Mike,

Let me know if you approve the second patch.

Comment 41 Mike Wilson 2006-05-23 15:25:58 EDT

Does the patch include code that provides the final answer your question "Should the block be marked as MALFORMED?"

Comment 42 Olivier Thomann 2006-05-23 16:14:41 EDT

The block is marked as RECOVERED. Do we also want it tagged as MALFORMED?
My patch doesn't tag it as MALFORMED. It depends if this is required or not. I believe RECOVERED is good enough.
Martin and Daniel, do you want it tagged as MALFORMED?

Comment 43 Martin Aeschlimann 2006-05-24 04:22:03 EDT

I think the last approach is the way to go. RECOVERED is enough.
The only small fix I suggest is to be nicer if the elso block has a problem: No need to skip the whole if statement.
that means, change:
	final Statement elseStatement = convert(statement2);
	if (elseStatement == null) return null;
	ifStatement.setElseStatement(elseStatement);
to
	final Statement elseStatement = convert(statement2);
	if (elseStatement != null) 
	     ifStatement.setElseStatement(elseStatement);

Comment 44 Dani Megert 2006-05-24 04:25:44 EDT

+1 for 3.2 RC6.

Comment 45 Mike Wilson 2006-05-24 09:38:47 EDT

+1

Comment 46 Olivier Thomann 2006-05-24 11:20:36 EDT

(In reply to comment #43)
Ok, I'll preserve the if statement if the else statement conversion returns null.
Do I have your +1 then for RC6?

Comment 47 Philipe Mulet 2006-05-24 11:44:34 EDT

+1 for 3.2RC6

Comment 48 Martin Aeschlimann 2006-05-24 11:47:59 EDT

+1

Comment 49 Olivier Thomann 2006-05-24 20:52:34 EDT

Fixed and released in HEAD.
Regression test added in org.eclipse.jdt.core.tests.dom.ASTConverter15Test.test0220

Comment 50 Olivier Thomann 2006-05-26 11:36:11 EDT

Verified in I20060526-0010 for 3.2RC6

Comment 51 kiril mitov 2006-05-30 05:40:51 EDT

Not sure if the bug is the same but I download the eclipse RC6 
from Fri, 26 May 2006 -- 00:10 (-0400), and without changing any preferences and configurations I started working and after some time I noticed the following exception has occured:

!ENTRY org.eclipse.jdt.ui 4 2 2006-05-30 12:03:50.786
!MESSAGE Problems occurred when invoking code from plug-in: "org.eclipse.jdt.ui".
!STACK 0
java.lang.IllegalArgumentException
        at org.eclipse.jdt.core.dom.ASTNode.setSourceRange(ASTNode.java:2599)
        at org.eclipse.jdt.core.dom.ASTConverter.convert(ASTConverter.java:483)
        at org.eclipse.jdt.core.dom.ASTConverter.buildBodyDeclarations(ASTConverter.java:179)
        at org.eclipse.jdt.core.dom.ASTConverter.convert(ASTConverter.java:2659)
        at org.eclipse.jdt.core.dom.ASTConverter.convert(ASTConverter.java:2420)
        at org.eclipse.jdt.core.dom.ASTConverter.convert(ASTConverter.java:511)
        at org.eclipse.jdt.core.dom.ASTConverter.buildBodyDeclarations(ASTConverter.java:179)
        at org.eclipse.jdt.core.dom.ASTConverter.convert(ASTConverter.java:2659)
        at org.eclipse.jdt.core.dom.ASTConverter.convert(ASTConverter.java:1242)
        at org.eclipse.jdt.core.dom.AST.convertCompilationUnit(AST.java:270)
        at org.eclipse.jdt.internal.core.CompilationUnit.buildStructure(CompilationUnit.java:187)
        at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:229)
        at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:505)
        at org.eclipse.jdt.internal.core.CompilationUnit.makeConsistent(CompilationUnit.java:993)
        at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.makeConsistent(ReconcileWorkingCopyOperation.java:131)
        at org.eclipse.jdt.internal.core.ReconcileWorkingCopyOperation.executeOperation(ReconcileWorkingCopyOperation.java:71)
        at org.eclipse.jdt.internal.core.JavaModelOperation.run(JavaModelOperation.java:720)
        at org.eclipse.jdt.internal.core.JavaModelOperation.runOperation(JavaModelOperation.java:779)
        at org.eclipse.jdt.internal.core.CompilationUnit.reconcile(CompilationUnit.java:1123)
        at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy$1.run(JavaReconcilingStrategy.java:97)
        at org.eclipse.core.runtime.SafeRunner.run(SafeRunner.java:37)
        at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:82)
        at org.eclipse.jdt.internal.ui.text.java.JavaReconcilingStrategy.reconcile(JavaReconcilingStrategy.java:145)
        at org.eclipse.jdt.internal.ui.text.CompositeReconcilingStrategy.reconcile(CompositeReconcilingStrategy.java:86)
        at org.eclipse.jdt.internal.ui.text.JavaCompositeReconcilingStrategy.reconcile(JavaCompositeReconcilingStrategy.java:94)
        at org.eclipse.jface.text.reconciler.MonoReconciler.process(MonoReconciler.java:75)
        at org.eclipse.jdt.internal.ui.text.JavaReconciler.process(JavaReconciler.java:342)
        at org.eclipse.jface.text.reconciler.AbstractReconciler$BackgroundThread.run(AbstractReconciler.java:204)

I tried to reproduce the error but again I had no such luck.

Comment 52 Jerome Lanneluc 2006-05-30 07:38:16 EDT

The stack traces are different. Please open a separate bug report.

Comment 53 Jerome Lanneluc 2006-05-30 07:38:40 EDT

Was verified.