Add ExplicitThis recipe to make this. prefix explicit.#791
Add ExplicitThis recipe to make this. prefix explicit.#791timtebeek merged 9 commits intoopenrewrite:mainfrom
this. prefix explicit.#791Conversation
|
Can you add a test with a typical setter implementation? |
c485d95 to
1e30b0d
Compare
| Test(String value) { | ||
| super(value); | ||
| // Shadowed parameter: looks like a bug but replacing would change semantics | ||
| value = value; |
There was a problem hiding this comment.
Can you add a test with a typical setter implementation? This is probably the most common example where of variable name shadowing. One being a field and another being a local.
@greg-at-moderne I added tests for value = value in this constructor, and field = field in a setter. While these are "obvious" bugs - setting a parameter to itself - it was a deliberate choice not to replace these and the test shows they do not get replaced. The replacement would change semantics. It's not a bad idea to find and fix these but my preference is to separate recipes that may change semantics from recipes that perform pure refactorings and don't need much scrutiny. What do you think?
a556fe4 to
d379d75
Compare
760a442 to
49ce8ae
Compare
3450bc5 to
bf790bb
Compare
timtebeek
left a comment
There was a problem hiding this comment.
I ran this recipe at scale against the Spring + Netflix organizations (~3000+ Java files across 77 repos) and found two bugs, plus several code simplification opportunities. I've pushed fixes to a branch with the suggested changes.
Bug fixes
1. Inherited members incorrectly qualified as SuperClass.this.method()
When a field or method is declared in a superclass, the recipe was generating AbstractBinderTests.this.getBinder() instead of this.getBinder(). This affected 763 out of 3074 patches.
Root cause: createQualifiedThisExpression only checked exact type match (isOfType). When the current class inherits from the declaring class, the types don't match exactly, so it fell through to createOuterThisReference.
Fix: Added TypeUtils.isAssignableTo(targetType.getFullyQualifiedName(), currentContext.type) to check if the current class is a subtype of the declaring class. Uses the FQN-based overload to handle parameterized types correctly.
2. Recipe was modifying Kotlin files
54 Kotlin files were incorrectly modified. Added Preconditions.check(Preconditions.not(new KotlinFileChecker<>()), ...) to exclude Kotlin, consistent with other Java-specific recipes.
Code simplifications
- Replace magic
0x0008LwithhasFlags(Flag.Static)andmethod.hasModifier(J.Modifier.Type.Static)(3 occurrences) - Use
JavaElementFactory.newThis()instead of manualJ.Identifierconstruction (2 occurrences) - Use
TypeUtils.isOfType()for type comparison instead of FQN string equality - Simplify
createFieldAccessby accepting pre-validated owner type parameter, removing redundant null checks - Use
instanceof JavaType.FullyQualified(broader) instead ofinstanceof JavaType.Class
Net result: -55 lines, +16 lines.
Validation results (after fixes)
- 3020 Java patches, 0 Kotlin patches
- 108 remaining
OuterClass.this.patterns are all legitimate (inner classes,@Nestedtest classes, anonymous classes)
Branch with changes: explicit-this on upstream
|
I guess it gets even more interesting when the inner class also extends its outer class. Would be good to have a test for. |
|
@timtebeek this is great, thank you. I realize I could have found the first bug myself by first deleting |
|
Added test cases I described earlier. |
- Replace magic number 0x0008L with hasFlags(Flag.Static) and hasModifier() - Use JavaElementFactory.newThis() instead of manual identifier construction - Use TypeUtils.isOfType() for type comparison instead of FQN string equality - Simplify createFieldAccess by accepting pre-validated owner type parameter - Use instanceof JavaType.FullyQualified for safer type checking
- Use TypeUtils.isAssignableTo with FQN to handle inherited fields/methods from superclasses (including parameterized types) — use plain this. instead of incorrectly generating SuperClass.this. - Add Preconditions.check to exclude Kotlin files via KotlinFileChecker
When a nested class C accesses a member declared in A through an intermediate class B that extends A, the recipe now correctly produces B.this.field instead of A.this.field. This is done by walking the enclosing class chain to find the nearest class assignable to the declaring type. Also fixes JavaToCsharp compilation for updated rewrite-csharp API.
|
Anything left to do here? |
this. prefix explicit.
timtebeek
left a comment
There was a problem hiding this comment.
Thanks a lot for the help (and patience) here! Looks good after some light polish, and at scale validation. That latter part is what took a little time to get to, but good to have this hardened and added now.
I've not yet added this to any larger sets, as it's a bit of an opinionated change, and it might be good that you try this copy on your own projects again to ensure that it's making all the right changes.
|
I just want to confirm what Tim wrote. Thanks for the contribution! It wasn't an easy one. |
What's changed?
Added a new
ExplicitThisrecipe that automatically adds the explicitthis.prefix to instance field accesses and method invocations.What's your motivation?
This recipe improves code clarity by making it immediately obvious when a field or method belongs to the current instance versus being a local variable, parameter, or static member.
The explicit
this.prefix follows a common coding style preference in Java codebases.Anything in particular you'd like reviewers to focus on?
I wound up writing quite a bit of code to determine whether we're looking at a non-static field that's not already prefixed with this. Is there a better way?
Should I register the recipe in any suite? Should I add the recipe to common-static-analysis.yml (commented out initially)?
Anyone you would like to review specifically?
Have you considered any alternatives or workarounds?
An alternative would be to make this configurable (e.g., only apply to fields, or only to methods), but starting with a simple "always add
this." approach keeps the recipe straightforward.Any additional context
Checklist