GRECLIPSE
  1. GRECLIPSE
  2. GRECLIPSE-682

[refactoring] Fix import statements after move/copy of a groovy compilation unit

    Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0Release
    • Fix Version/s: 2.5.2.Release
    • Component/s: Refactoring
    • Labels:
      None
    • Number of attachments :
      0

      Description

      The last remaining problem (that I know of) with moving and copying of groovy compilation units are that import statements are not properly updated.

      For Java files, an import rewriter is used, but this does not work for groovy files. Perhaps, we can make a change in the import rewriter to delegate to a groovy version when required. We already have a "groovy organize imports" action. This can be refactored a bit and moved from UI to core and then this can be used to rewrite import statements.

        Activity

        Hide
        Andrew Eisenberg added a comment -

        We should really get this working for 2.1.0.

        Show
        Andrew Eisenberg added a comment - We should really get this working for 2.1.0.
        Hide
        Andrew Eisenberg added a comment -

        As I say above, we really should get this working for 2.1.2.

        Show
        Andrew Eisenberg added a comment - As I say above, we really should get this working for 2.1.2.
        Hide
        Rene Scheibe added a comment -

        Thanks a lot. This really saves time for refactorings.

        Show
        Rene Scheibe added a comment - Thanks a lot. This really saves time for refactorings.
        Hide
        Andrew Eisenberg added a comment -

        It's shocking that we haven't gotten this to work yet. We really should. I just had a look at what we need to do and it is much simpler than I had originally thought. We do not need to do the import updating ourselves, that can all be delegated to existing JDT code. All we need to do is re-implement the TypeReferenceSearchRequestor so that it can handle wildcard types.

        Show
        Andrew Eisenberg added a comment - It's shocking that we haven't gotten this to work yet. We really should. I just had a look at what we need to do and it is much simpler than I had originally thought. We do not need to do the import updating ourselves, that can all be delegated to existing JDT code. All we need to do is re-implement the TypeReferenceSearchRequestor so that it can handle wildcard types.
        Hide
        Andrew Eisenberg added a comment -

        I have a prototype implementation working locally, but I uncovered a fairly serious JDT bug. I raised an issue here:

        https://bugs.eclipse.org/350205

        Still need to write some tests.

        Also, this will not be released until after the 2.5.1 release.

        Show
        Andrew Eisenberg added a comment - I have a prototype implementation working locally, but I uncovered a fairly serious JDT bug. I raised an issue here: https://bugs.eclipse.org/350205 Still need to write some tests. Also, this will not be released until after the 2.5.1 release.
        Hide
        Rene Scheibe added a comment -

        What's missing in the current implementation? I just tested a Groovy class that holds an instance of a Java and one of a Groovy class. Moving these two classes to another package updates the imports correctly.

        Show
        Rene Scheibe added a comment - What's missing in the current implementation? I just tested a Groovy class that holds an instance of a Java and one of a Groovy class. Moving these two classes to another package updates the imports correctly.
        Hide
        Andrew Eisenberg added a comment -

        Interesting...there are a number of cases that do seem to be working without any fix (something I wasn't aware of), but I am focussing on this situation:

        In src/first/ToMove.groovy:

        package first;
        
        public class ToMove {
        	public Foo x;
        }
        

        In src/first/Foo.groovy:

        package first;
        
        public class Foo {
        }
        

        Now, ToMove.groovy move to a new package and no import statement is added.

        It looks like if Foo.groovy is moved, then an import statement is correctly added to ToMove.groovy. But, this all needs some tests to make sure that it is not accidentally broken in the future.

        Show
        Andrew Eisenberg added a comment - Interesting...there are a number of cases that do seem to be working without any fix (something I wasn't aware of), but I am focussing on this situation: In src/first/ToMove.groovy : package first; public class ToMove { public Foo x; } In src/first/Foo.groovy : package first; public class Foo { } Now, ToMove.groovy move to a new package and no import statement is added. It looks like if Foo.groovy is moved, then an import statement is correctly added to ToMove.groovy. But, this all needs some tests to make sure that it is not accidentally broken in the future.
        Hide
        Andrew Eisenberg added a comment -

        Fixed. Will go into 2.5.2 release.

        Show
        Andrew Eisenberg added a comment - Fixed. Will go into 2.5.2 release.

          People

          • Assignee:
            Andrew Eisenberg
            Reporter:
            Andrew Eisenberg
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: