Groovy-Eclipse breaks java refactoring (extract to local var)

[aclement]

According to this spring-ide issue: spring-attic/spring-ide#89

Installing groovy-eclipse breaks the extract to local variable refactoring. Eric are you aware of anything changed recently that might have done that?

[eric-milles]

There was a change in ASTConverter related to end angle bracket position. Here is the edit. I'll see if the linked issue is fixed by reverting.

// GROOVY edit - avoid IllegalArgumentException in setSourceRange below
//end = type2 != null ? type2.getStartPosition() + type2.getLength() - 1 : end;
if (type2 != null) {
	if (type2.getLength() > 0) {
		end = type2.getStartPosition() + type2.getLength() - 1;
	} else if (type2.isSimpleType()) {
		Name name = ((SimpleType) type2).getName();
		// TODO: If there are previous type args, use their positional info or lengths...
		end += name.toString().length() + 1; // ballpark it
		System.err.println("SimpleType with no position info"); //$NON-NLS-1$
	} else {
		System.err.println("Type with no position info"); //$NON-NLS-1$
	}
}
// GROOVY end

[aclement]

Thanks. That could be it. All the position fiddling is very sensitive to change. Strictly speaking we should run all the JDT tests against patched JDT and it would catch these things but never got around to that.

[eric-milles]

When I got started with this I tried to fix the JDT tests that are in the compiler.tests and builder.tests, but there were so many failures and it was very time consuming. Would be nice to be able to just import the JDT test plug-ins and run those tests.

[eric-milles]

I am able to recreate the issue locally, so I should be able to fix here shortly.

[eric-milles]

Sorry, it appears to be this edit in same file:

// GROOVY edit - end can be -1
end = retrieveClosingAngleBracketPosition(/*end + 1*/ sourceStart + simpleName.getLength());
type.setSourceRange(sourceStart, end - sourceStart + 1);

[eric-milles]

Could you have the original submitter try again? This should be fixed now.

[aclement]

They confirmed it is fixed, thanks so much for the fast turnaround Eric. Do we have enough test coverage around these changes? I didn't see any new tests in your fix commit. I'm just aware, as I say, that the positions stuff is so sensitive, very easy to break. Thanks again!