GRECLIPSE

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

is related to

Improvement - An improvement or enhancement to an existing feature or task. GRECLIPSE-1256 Use a classfile attribute in the classfile to help determine the original method declaration of a method that uses default parameters

  • Major - Major loss of function.
  • Open - The issue is open and ready for the assignee to start work on it.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: