Groovy plugin may break JDT rename feature completely

Sorry, I meant the 2.0.0Release. Please change it for me, thanks.

Environment is Eclipse 3.5.1.

I am on leave right now and will not be able to get to this immediately, but I will try to look at this when I can in the next few days.

In reply to comment #3:
> I am on leave right now and will not be able to get to this immediately, but I
> will try to look at this when I can in the next few days.

Thanks, Andrew. It would be good to add yourself to Cc: on the Eclipse bug because any new findings on my side will appear over there:

302455: java.lang.ClassCastException in secondary types removal
https://bugs.eclipse.org/bugs/show_bug.cgi?id=302455

Enjoy your vacation!

Jörg

The initial stack overflow that you reported in https://bugs.eclipse.org/bugs/show_bug.cgi?id=299746 was fixed on January 26. Unfortunately, there is no jira attached to the fix. Updating to the latest snapshot release will fix the stack overflow problem.

I'll have a deeper look at the secondary types problem when I can.

A new dev build of groovy-eclipse is available with a slight adjustment in secondary type handling. Please try it if you get a chance. (available from the usual place: http://dist.codehaus.org/groovy/distributions/greclipse/snapshot/e3.5/ )

Hi Andy,

thanks for the new version. I will give it try.

In addition, I noticed that the current plugin is not compatible with Eclipse 3.6 Milestone builds.
Since the Eclipse guys currently fix a couple of related issues in the 3.6 head, it would be helpful
to have a 3.6 compatible development build of the Groovy plugin.

Cheers, Jörg

FYI: Comment on Eclipse bug 299746: StackOverflowError while renaming a method if the Groovy plugin is installed
https://bugs.eclipse.org/bugs/show_bug.cgi?id=299746

Set as not eclipse instead of duplicate. This initial issue reported at comment is a problem with the Groovy plugin. The search issue of bug 286379 was reported later while doing tests to reproduce the problem.

Hence, it's clearer to have on bug per issue:

• bug 299746: Groovy problem described at comment 0
• bug 286379: Search problem got at comment 4
• bug 302455: Secondary types cache problem got a cmment 6In reply to GRECLIPSE-642:

we will start work on a 3.6 based version shortly.

In reply to comment #6:
> A new dev build of groovy-eclipse is available with a slight adjustment in
> secondary type handling. Please try it if you get a chance. (available from the
> usual place:
> http://dist.codehaus.org/groovy/distributions/greclipse/snapshot/e3.5/ )

Just upgraded to version 2.0.1.20100212-1000-e35 from the given dev build link
and now it works a bit better:

I do not get any stack overflow exceptions, but it takes several seconds in the pre-conditions check.
Finally, it offers an Groovy ambigous rename dialog but the code places are not related to the rename I am doing:

I rename a public static final String in a Java class and the Groovy ambigous rename dialog presents me other places where an equally named String is used but is also defined in the same class.

E.g.

class1.java:

public class class1 {
	public final static String PROPERTY = "xxx";
	...
}

...
class2.groovy

public class class2 {

	public final static String PROPERTY = "yyy";
	...
	private final static String A = PROPERTY + "-A";
	...
}

Now doing a rename refactoring of class1.PROPERTY also presents to me the reference of PROPERTY in class2.A,
but this seems to me completely unrelated.

In addition, this check takes much time. If I delete the complete Groovy source tree, the rename takes about 1 second, not several ones.

Is this another bug?

Glad to hear the exception is gone. I'm sure the slowness you are seeing can be sped up, I don't believe we have (yet) put any effort into optimizing that code.

In reply to comment #12:
> Glad to hear the exception is gone. I'm sure the slowness you are seeing can be
> sped up, I don't believe we have (yet) put any effort into optimizing that code.

Sad to say, but the only workaround at the moment is to (temporarily) remove the complete Groovy code.

Would it help to put all Groovy code into a separate project?

Is there any way to disable the pre-condition checks in the Groovy code?

> Would it help to put all Groovy code into a separate project?

I'd suspect this wouldn't help as if there is dependency between the projects it will still be searched to see if a refactoring change affects it. But if setup in this way you could possibly break the dependency, make the rename, then reintroduce the dependency.

> Is there any way to disable the pre-condition checks in the Groovy code?

We haven't looked at that code at all yet, it is straight from the old (<2) version of Groovy-Eclipse. Given the optimizations we've made to other inherited code from the old plugin, I'm sure it can be improved.

We haven't looked at that code at all yet, it is straight from the old (<2) version of Groovy-Eclipse. Given the optimizations we've made to other inherited code from the old plugin, I'm sure it can be improved.

Andy, I would be happy to provide you with jstack trace to indicate the places where the most time is spent.
I have a little shell script which can do jstack traces every 0.5 or even 0.1 second. Using diff or vimdiff you
can then see where the hotspots are.

Just reply which kind of information you need to optimize this code. It is quite crucial for us.

Thanks, Jörg

Hi Andy,

can we expect some significant improvements here for the 2.0.1 release?
I am happy to provide you with the required feedback from our side.

Currently, we a big Java project and a smaller sub-tree of Groovy code inside. But the Groovy plugin prevents the rename refactoring (except local renames) for the complete Java project.

All our developers currently doing rename by hand which is a shame.

As I said: I am happy to provide you with any feedback you need, but the current situation is quite annoying.

Thanks, Jörg

I hope you will see some changes in the 2.0.1 timeframe, yes.

In reply to comment #11:
> Renaming a public method takes minutes (!!!) and during that time this operation
> cannot be cancelled.
>
> If this would help you, I can provide jstack traces every 0.5 seconds taking
> during the refactoring. In this way, you could see that the most time is spent
> in the "Modal Context" thread, method
>

org.codehaus.groovy.eclipse.refactoring.core.rename.renameMethod.RenameMethodProvider.prepareCandidateLists()
> 

>
> Is there any way to speed this up?

Groovy refactoring currently works by looking through all groovy source file in the associated projects to find potential matches (that is what happens in prepareCandidateLists()). I'm guessing that you have a project with a large number of groovy files and this is why rename is taking so long.

The refactoring code was contributed by an outside user and does not use Java search to prune the search space. Once this happens, I'd imagine that you would see a significant speedup.

In reply to comment #18:
> In reply to comment #11:
> > Renaming a public method takes minutes (!!!) and during that time this
> operation
> > cannot be cancelled.
> >
> > If this would help you, I can provide jstack traces every 0.5 seconds taking
> > during the refactoring. In this way, you could see that the most time is spent
> > in the "Modal Context" thread, method
> >
>

org.codehaus.groovy.eclipse.refactoring.core.rename.renameMethod.RenameMethodProvider.prepareCandidateLists()
> > 

> >
> > Is there any way to speed this up?
>
> Groovy refactoring currently works by looking through all groovy source file in
> the associated projects to find potential matches (that is what happens in
> prepareCandidateLists()). I'm guessing that you have a project with a large
> number of groovy files and this is why rename is taking so long.

Our project contains 214 groovy files and 4738 java files. Do you think this is big?

> The refactoring code was contributed by an outside user and does not use Java
> search to prune the search space. Once this happens, I'd imagine that you would
> see a significant speedup.

And when will this happen? We are expecting an improvement here.

Alternatively, I would like to see a configuration option to disable Groovy refactoring completely
since it breaks the more important Java refactoring for us.

That's not a particularly big project on the groovy side, but also important is the size of the files. 2.0.1 is coming out on Friday. After that, I plan on focusing on fixing refactoring bugs. This will be high on the list.

In reply to comment #20:
> That's not a particularly big project on the groovy side, but also important is
> the size of the files. 2.0.1 is coming out on Friday. After that, I plan on
> focusing on fixing refactoring bugs. This will be high on the list.

To be honest: That is too late. To restate this: It also breaks the Java refactoring. Only remedy at the moment: remove all Groovy code!

Is there any way to disable the Groovy Rename Refactoring?

Thanks, Jörg

OK. As a temporary measure, this is something I can do.

In reply to comment #22:
> OK. As a temporary measure, this is something I can do.

Great, that would improve the situation for us. Do you consider an option in the Window / Preferences?
I guess this will be an extra JIRA issue.

Thanks, Jörg

I'll keep updating this issue. And I'll add a new preference page: Groovy -> Refactoring

It will initially have one option (disable refactoring participation), but I can expand it later to have other options.

Fascinating...I am playing around with disabling groovy refactoring participation and it seems that it has become largely redundant.

As mentioned earlier, this code had been contributed by an external committer and was implemented around the time we were providing deeper integration with JDT. So, this code does not make use of Java Search to find Java references in groovy code. In fact, if any such references are found, they are actively removed.

Without the groovy participant enabled, then the Java refactoring can work as normal and groovy type inferencing is used to determine the types and things to be renamed. This appears to be a better search mechanism than what is currently being used for the refactoring support.

There are a few quirks that I have found:

1. Methods and fields that are private in a Java class will not be refactored in a groovy class (remember that groovy code can reference private members of Java classes). This is because when refactoring private members, JDT refactoring takes a shortcut and does not look for references outside the compilation unit.
2. Getters and setters from properties will not be found and renamed.

However, the surprising thing is that refactoring seems to work quite well with the participants disabled. In some cases, it even works better.

Clearly, there is a lot of work that needs to be done in this area. But, I will be committing the work I've done so far shortly. I'll let you know when the build is available and I'll ask you to provide some feedback if possible.

In reply to comment #25:
> Fascinating...I am playing around with disabling groovy refactoring
> participation and it seems that it has become largely redundant.

So it was good nagging you

> [...]
>
> There are a few quirks that I have found:
>
> # Methods and fields that are private in a Java class will not be refactored in
> a groovy class (remember that groovy code can reference private members of Java
> classes). This is because when refactoring private members, JDT refactoring
> takes a shortcut and does not look for references outside the compilation unit.

Maybe this can be addressed for Eclipse 3.6 Helios in the JDT part. Please talk to the Eclipse JDT guys commenting on
the referenced Eclipse bugs, most noteably Frederic Fusier. They seem to be quite helpful.

> # Getters and setters from properties will not be found and renamed.
>
> However, the surprising thing is that refactoring seems to work quite well with
> the participants disabled. In some cases, it even works better.
>
> Clearly, there is a lot of work that needs to be done in this area. But, I will
> be committing the work I've done so far shortly. I'll let you know when the
> build is available and I'll ask you to provide some feedback if possible.

My team is happy to give you any feedback quite soon

Looking forward to see this in 2.0.2Release.

Cheers, Jörg

New dev build of Groovy-Eclipse is available.

In reply to comment #27:
> New dev build of Groovy-Eclipse is available.

Thanks, will test tomorrow morning.

In reply to comment #28:
> In reply to comment #27:
> > New dev build of Groovy-Eclipse is available.
>
> Thanks, will test tomorrow morning.

Great, old performance of Java renames restored:

• public static constant renamed (5 references in workspace, but many ambiguous refs in Groovy):
  • enabled: 30 seconds (clicked away a Groovy rename pop-up)
  • disabled: 4 seconds including incremental build
• public method renamed (592 references in workspace)
  • enabled: 82 seconds
  • disabled: 12 seconds + 15 seconds to rebuild workspace

Also checked rename on Groovy methods which does not worked independent of enabled/disabled participant.

Interestingly, if pressing "Alt-Shift-R" (Rename short-shut) on a groovy method, the complete GUI locks up for minutes at least, disappearing in the deeps of the GroovyRecognizer and ANT-LR parser. Can provide jstack series if you like...

Is this expeceded?

Anyway, please, please provide this disable option for the 2.0.1Release!

Thanks, Jörg

Yes, this will be available in 2.0.1.

OK. I have now committed a first pass at completely rewriting the refactoring support. Rename type, field and method are available. Local variables will be available soon.

This new support uses JDT search to do all name and reference lookups. It is running much faster and should not affect Java projects at all.

See GRECLIPSE-587. If you try this out, I'd appreciate your feedback.

Closing this bug since the initial problem you describe is fixed. Follow all future progress on GRECLIPSE-587.

Thanks, Andrew.

I am looking forward to these improvements and will be happy to test them. Are they available in the dev builds already?

Yes, the snapshot update site is here:

http://dist.codehaus.org/groovy/distributions/greclipse/snapshot/e3.5/

Note that rename local variable is not available yet. I'll be adding that in the next day or so.

Thanks. Please note that I am still on vacation this week and will be able to test this starting next week at earliest.