GRECLIPSE
  1. GRECLIPSE
  2. GRECLIPSE-1233

Renaming a method that has a parameter with a default value doesn't work

    Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.5.2.Release
    • Fix Version/s: 2.6.1.M1
    • Component/s: Parser, Refactoring, Search
    • Labels:
      None
    • Environment:
    • Number of attachments :
      0

      Description

      When attempting to rename a method with a parameter that has a default value, the rename does not work correctly. Here is the before and after code:

      Before
      class Summer {
      	def sumWithDefaults(a, b, d=0){
      		return a + b + d
      	}
      }
      
      def summer = new Summer()
      
      assert 2 == summer.sumWithDefaults(1,1)
      assert 3 == summer.sumWithDefaults(1,1,1)
      
      After
      refactoredSumlass Summer {
      	def sumWithDefaults(a, b, d=0){
      		return a + b + d
      	}
      }
      
      def summer = new Summer()
      
      assert 2 == summer.refactoredSum(1,1)
      assert 3 == summer.sumWithDefaults(1,1,1)
      

      This is the expected result:

      Expected
      class Summer {
      	def refactoredSum(a, b, d=0){
      		return a + b + d
      	}
      }
      
      def summer = new Summer()
      
      assert 2 == summer.refactoredSum(1,1)
      assert 3 == summer.refactoredSum(1,1,1)
      

      These are the specific problems observed:

      1. The new method name replaces the first character in the file
      2. The new method name does not actually replace the old method name in the method declaration
      3. Not all method calls with the old name and a matching signature are refactored
        • Only method calls who's call signature matches the parameters with no default value are updated

        Issue Links

          Activity

          Hide
          Andrew Eisenberg added a comment -

          This problem happens because of the way that we represent methods with default values internally. Each variant of the method is represented as a new method, but with a source location of [0,1]. It looks like the rename is attempting to change the name of the declaration of the default method, but since it doesn't exist, it winds up replacing the first char of the file with the new name.

          Show
          Andrew Eisenberg added a comment - This problem happens because of the way that we represent methods with default values internally. Each variant of the method is represented as a new method, but with a source location of [0,1] . It looks like the rename is attempting to change the name of the declaration of the default method, but since it doesn't exist, it winds up replacing the first char of the file with the new name.
          Hide
          Andrew Eisenberg added a comment -

          Also need to work on find occurrences and search for methods with default values.

          Show
          Andrew Eisenberg added a comment - Also need to work on find occurrences and search for methods with default values.
          Hide
          Andrew Eisenberg added a comment -

          Fundamentally, this problem is with source locations. I need to found out why the source location for the synthetic variant of the method has invalid values.

          Show
          Andrew Eisenberg added a comment - Fundamentally, this problem is with source locations. I need to found out why the source location for the synthetic variant of the method has invalid values.
          Hide
          Andrew Eisenberg added a comment -

          OK. I was able to fix the initial source location problem. And the remaining part is to be able to map method references that take advantage of defaults to their actual definition.

          Show
          Andrew Eisenberg added a comment - OK. I was able to fix the initial source location problem. And the remaining part is to be able to map method references that take advantage of defaults to their actual definition.
          Hide
          Andrew Eisenberg added a comment -

          Fix the second part where we go looking for method declarations based on references. Essentially, for each potential match, we need to search through all other methods of the same name for the original method variant (ie- the method that is actually declared instead of the synthetic ones that use defaults).

          This solution is a bit less satisfying because a better way would be to store a back reference in each MethodNode to the original method variant. This would be a much faster check, but it would also require making our patch of the groovy compiler bigger and adding a field that is only needed for the IDE and this would be something hard to feed back into groovy core.

          Show
          Andrew Eisenberg added a comment - Fix the second part where we go looking for method declarations based on references. Essentially, for each potential match, we need to search through all other methods of the same name for the original method variant (ie- the method that is actually declared instead of the synthetic ones that use defaults). This solution is a bit less satisfying because a better way would be to store a back reference in each MethodNode to the original method variant. This would be a much faster check, but it would also require making our patch of the groovy compiler bigger and adding a field that is only needed for the IDE and this would be something hard to feed back into groovy core.
          Hide
          Andrew Eisenberg added a comment -

          Fixed and committed with regression tests. I added a method inside of MethodNode called getOriginal(). This method returns the MethodNode corresponding to the one where no default methods have been applied. It is a much simpler solution than the one I had implemented earlier.

          Show
          Andrew Eisenberg added a comment - Fixed and committed with regression tests. I added a method inside of MethodNode called getOriginal(). This method returns the MethodNode corresponding to the one where no default methods have been applied. It is a much simpler solution than the one I had implemented earlier.

            People

            • Assignee:
              Andrew Eisenberg
              Reporter:
              Rick Jensen
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: