eclipse-jdt / eclipse-jdt/eclipse.jdt.ui
Proposal: add generic pre/post condition checks for name-binding stability in Java refactorings
- Dominant language
- Java
- Stars
- 59
- Forks
- 127
- Avg merge
- 23h 30m
- Merged PRs (30d)
- 35
Description
## Description
This is a proposal / request for feedback, not a report for one specific refactoring bug.
Several recent refactoring bugs reported from the refactoring-engine test suites seem to share a common semantic pattern: after a refactoring, an existing Java source reference may resolve to a different declaration because the refactoring introduced, moved, pulled up/down, extracted, or renamed a declaration.
At a high level, the common pattern is:
1. The refactoring introduces a declaration `f`, or transforms an old declaration `g` into a new declaration `f`.
2. A source reference that used to resolve to some existing declaration `f'` now resolves to the newly introduced `f`; or
3. A source reference that was intended to move from `g` to the new `f` is instead captured by another existing declaration `f'`.
This may silently change behavior, change a method/field binding, or produce a compile error.
We would like to ask whether the JDT team would be open to a more generic refactoring safety check for this class of problems, instead of fixing each refactoring type with separate shadowing checks.
## Motivation
There are a number of Eclipse JDT refactoring bugs where the symptom is a changed call/field relationship after refactoring:
Examples that look directly related to name-binding, shadowing, hiding, or overload capture include:
### Field / variable binding changes
- #1744: Move Field causes an assignment to bind to a different `i`.
- #1745: Move Field changes which `b` is printed.
- #1738: Pull Down Field changes behavior around an inherited/local `b`.
### Method call binding changes
- #1865: Move Method introduces `m(String)` into `B`, causing an existing `m("1")` call to resolve differently.
- #1757: Extract Method creates a new helper `m(String)`, causing an existing `m("1")` call to resolve differently.
- #1758: Extract Method affects a member/inner class call; a call that used to target the outer class method becomes resolved through the superclass.
- #1748: Inline Method moves/copies a method body into a subclass context, causing `m("1")` to call the subclass method instead of the superclass method.
- #1756: Pull Down Method moves a method body into a subclass where `m("1")` resolves to a different method.
- #1750: Change Method Signature renames `foo2(Long)` to `foo1(Long)`, causing an existing `foo1(n)` call to select a different overload.
- #1751: Change Method Signature renames `m()` to `k()`, causing an inner-class call to become recursive.
- #1870: Pull Up Method introduces `foo1(Long)` into a type that already has `foo1(Number)`, leading to a changed/invalid call.
- #1869: Pull Up Method introduces `m(String)` into `B`, changing the resolution of an existing `m("1")` call.
Related same-name member conflicts:
- #1866: Move Method introduces an instance method with the same name as a static method in a subtype, producing a compile error.
- #1868: Pull Up Method introduces an instance method with the same name as a static method in a subtype, producing a compile error.
## Existing fixes and their limitation
Some of the above bugs have already been fixed in JDT, and those fixes are very useful. However, the existing fixes appear to be implemented as local, refactoring-specific checks.
Examples:
- PR #1690 fixed #1679 by changing `PushDownRefactoringProcessor` to look for method invocations and field accesses to superclass members that collide with members in destination types, and then rewrite them with `super`.
- PR #1761 fixed #1758 by modifying `ExtractMethodRefactoring.checkForMethodOverride()` to also check member classes.
- PR #1767 fixed #1751 by modifying `ChangeSignatureProcessor` to check whether the new method name is already accessible at a location that calls the old method name.
- PR #1790 fixed #1750 by adding a new `checkShadowing2` check in `ChangeSignatureProcessor`, recognizing cases where a method rename may affect existing same-name method calls.
These fixes suggest that JDT already has the right building blocks: Java Search, AST parsing, binding resolution, `RefactoringStatus`, and AST rewriting. However, they also show a potential maintainability problem:
1. Similar name-binding bugs are fixed separately in individual refactoring processors.
2. Each fix covers a specific refactoring type and a specific syntactic pattern.
3. The same underlying problem can reappear in another refactoring type, e.g. Move Method, Inline Method, Pull Up/Down, Extract Method, Change Signature, Move Field, Inline Variable, etc.
4. Some fixes report an error, while others rewrite with `super`; behavior may become inconsistent across refactorings.
5. Each new bug may require another `checkShadowingX`-style method, rather than reusing a shared semantic condition.
6. It is easy to miss related scopes such as member classes, local classes, anonymous classes, nested classes, inherited members, static imports, receiver static type changes, etc.
For example, #1750 and #1751 are both Change Method Signature bugs and were fixed inside `ChangeSignatureProcessor`, but #1865 is a Move Method bug with a very similar binding-capture pattern. Likewise, #1679 was handled inside Push Down, but #1748 has a similar moved/copied-body binding problem in Inline Method.
This suggests that the root problem is not specific to Change Signature, Extract Method, or Push Down. The root problem is that the refactoring result may violate the intended binding of existing source references.
## Proposed direction
Would JDT be open to a generic precondition and/or postcondition framework for refactorings that checks name-binding stability?
The intended requirement would be:
> For each source reference that should preserve its meaning across a refactoring, the resolved declaration after refactoring should be the expected declaration.
>
> If the reference originally resolved to a declaration intentionally changed by the refactoring, it should resolve to the corresponding new declaration.
>
> Otherwise, it should continue to resolve to the same declaration as before.
This could be used as a generic guard against:
- local variable / parameter shadowing
- field hiding
- method overload capture
- method calls captured by newly introduced methods
- calls inside moved/copied method bodies resolving against a different implicit receiver
- inner/member/local/anonymous class name capture
- static/instance same-name conflicts
We are not proposing a specific implementation in this issue yet. The first question is whether such a cross-refactoring safety condition would be considered desirable and acceptable in JDT.
## Possible shape
A possible design would be to have refactoring processors expose enough semantic intent to a shared checker, for example:
- declarations introduced by the refactoring
- declarations renamed or moved by the refactoring
- code bodies moved or copied to another type/context
- references that are expected to be redirected to a new declaration
- references that are expected to keep their existing binding
Then a precondition check could detect likely name-binding conflicts before applying the refactoring.
A postcondition or final-condition check could also validate the generated preview/change before it is applied, as a safety net.
The goal would be to avoid adding more ad-hoc `checkShadowingX` logic to each individual refactoring type, and instead have a shared mechanism that can be reused by Change Method Signature, Move Method, Extract Method, Inline Method, Pull Up/Down Method, Move Field, Pull Down Field, etc.
## Questions
Would the JDT team consider this kind of generic name-binding stability check a useful enhancement?
If yes:
1. Should it be implemented primarily as a precondition check, a final-condition check after preview generation, or both?
2. Would it be preferable to start with methods/fields only, then extend to variables, types, imports, and receiver-static-type changes?
3. Is there an existing shared refactoring infrastructure in JDT where this kind of cross-refactoring check should live?
4. Would a conservative checker that reports potential binding changes be acceptable as a first step, even if some cases are later refined to automatic fixes?
We are interested in contributing a prototype or more focused follow-up issues if this direction is acceptable.
Contributor guide
Research direction
Start by reviewing the refactoring processors named in the proposal, including ChangeSignatureProcessor, ExtractMethodRefactoring, and PushDownRefactoringProcessor, along with the listed related issues and fixes. The immediate outcome is a decision on whether a shared name-binding stability mechanism is desirable and a focused scope for a prototype; no implementation location or test is specified yet.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- devtools, tooling
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100