Comment 1 Olivier Thomann 2011-03-22 22:09:50 EDT

The synchronized leads to an empty enum type that looks like this:
synchronized enum $missing$ {
  $missing$() {
    super();
  }
  <clinit>() {
  }
}
So it blows up. I am investigating.

Comment 2 Olivier Thomann 2011-03-23 09:57:27 EDT

At this stage, the best we can do is to prevent the exception. The recovery is doing a poor job at getting a tree for the method body in this case.
So we can tag some nodes as MALFORMED.
The resulting tree is not really usable for refactoring as the only statement recovered in this method body is the synchronized keyword. Everything else is ignored after the syntax error.

What AST would you expect in this case ?

Comment 3 Markus Keller 2011-03-23 15:49:29 EDT

Simplified example:

public class Try {
    public static void main(String[] args) {
        synchronized
        new Object();
    }
}


(In reply to comment #2)
> What AST would you expect in this case ?

Ideally one where just the bad keyword is removed or recovered (similar to when I use 'void' or 'synchronizedXXX' instead). If that's too much work, an empty body (MALFORMED Block with no statements) is also fine.

Note that synchronized is not the only keyword causing trouble here.
public protected private static final strictfp etc. have the same problem.

Comment 4 Olivier Thomann 2011-04-05 09:33:13 EDT

Ayushman, please try to see if there is a better way to recover from the synchronized modifier alone.
If it possible to prevent the CCE, but we should see if there is a better way to recover first.

Comment 5 Ayushman Jain 2011-04-12 15:37:46 EDT

Created attachment 193087 [details]
proposed fix

This happens in the recovery because scope trial succeeds for the rule EnumHeaderName ::= Modifiersopt 'enum' Identifier (since 'synchronized', 'private', 'strictfp' etc are all modifiers), which is more permissive than it should be. Yet, there's no other option as well. 

The attached patch just makes sure that if the recovery, specifically the statements recovery finds a recovered enum declaration, it does not add it as a child to the recovered elements. The normal recovery will still find enums when they're declared in the usual correct manner.

Olivier, what do you think about this approach? I'll add test cases if you approve.

Comment 6 Olivier Thomann 2011-04-12 15:46:23 EDT

I like the approach. I think it would even be better if this rule is not considered at all for the recovery as the suggestion for the syntax error is misleading. I am not sure if this is really doable.

Comment 7 Ayushman Jain 2011-04-12 15:51:52 EDT

(In reply to comment #6)
> I like the approach. I think it would even be better if this rule is not
> considered at all for the recovery as the suggestion for the syntax error is
> misleading. I am not sure if this is really doable.

I did try to think about that, but its not really doable. Recovery has 1:1 mapping to the grammar, and unless we can rewrite the rule EnumHeaderName ::= Modifiersopt 'enum' Identifier to make it more strict and allow only the valid modifiers, we can't achieve any change in the recovery. That, ofcourse, won't be a very clean way. I have not been able to find any explanation though, as to why EnumHeaderName is chosen over ClassHeaderName. Looks like a random choice to me.

Anyway, with the above patch all tests.

Comment 8 Ayushman Jain 2011-04-13 05:54:00 EDT

Created attachment 193140 [details]
proposed fix + tests

Same patch with tests added.

Comment 9 Olivier Thomann 2011-04-13 10:03:44 EDT

+1

Comment 10 Ayushman Jain 2011-04-13 12:11:58 EDT

Released in HEAD for 3.7M7

Comment 11 Srikanth Sankaran 2011-04-25 04:45:53 EDT

Verified for 3.7 M7 using build I20110421-1800