GRECLIPSE

Spurious warning when rename refactoring field reference inside of closure

Details

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

Description

Inside of this code:

class Result {
	def val
	def doOperation= {
		 val
	}
}

Perform rename refactor on val. And after committing the refactoring change, the processor comes up with a warning:

The reference to val2 will be shadowed by a renamed declaration.

This is happening because we are visiting the closure expression twice when looking for references. The first time as part of the field declaration and the second time as part of the default constructor (groovy stuffs field initializers into the default constructor). So, we need to be more careful about visiting here.

Just to note, if the user clicks Continue the refactoring completes as expected with no errors.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Andrew Eisenberg added a comment -

I think the solution here is as simple as not visiting the field initializer directly from the inferencing engine. Let the visit happen indirectly when the default constructor is visited. However, I'm not confident enough yet that this won't fail under certain situations such as when there is an existing default constructor.

Show
Andrew Eisenberg added a comment - I think the solution here is as simple as not visiting the field initializer directly from the inferencing engine. Let the visit happen indirectly when the default constructor is visited. However, I'm not confident enough yet that this won't fail under certain situations such as when there is an existing default constructor.
Hide
Permalink
Andrew Eisenberg added a comment -

It's a little more complicated than what I describe above. I also need to take into account initializers (both static and non-static).

Show
Andrew Eisenberg added a comment - It's a little more complicated than what I describe above. I also need to take into account initializers (both static and non-static).
Hide
Permalink
Andrew Eisenberg added a comment -

The problem is that field initializers are copied to one or more constructors. The constructors that they are copied to are the base constructors (ie- the constructors that don't call any other constructors in the class). Also, object initializers are copied as well using the same heuristics. But, they are also removed from the class.

Rather than trying to do some really smart visitation so that each statement is visited exactly once, a simpler approach would be to visit the entire AST (duplicates and all). But, ensure that the requestor does not accept duplicates.

Show
Andrew Eisenberg added a comment - The problem is that field initializers are copied to one or more constructors. The constructors that they are copied to are the base constructors (ie- the constructors that don't call any other constructors in the class). Also, object initializers are copied as well using the same heuristics. But, they are also removed from the class. Rather than trying to do some really smart visitation so that each statement is visited exactly once, a simpler approach would be to visit the entire AST (duplicates and all). But, ensure that the requestor does not accept duplicates.
Hide
Permalink
Andrew Eisenberg added a comment -

Fix committed with regression tests.

Show
Andrew Eisenberg added a comment - Fix committed with regression tests.

People

  • Assignee:
    Andrew Eisenberg
    Reporter:
    Andrew Eisenberg
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: