-
Notifications
You must be signed in to change notification settings - Fork 92
Add ExplicitThis recipe to make 'this.' prefix explicit. #791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Can you add a test with a typical setter implementation? |
| import org.openrewrite.marker.Markers; | ||
|
|
||
| import java.time.Duration; | ||
| import java.util.Collections; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import java.util.Collections; | |
| import static java.util.Collections.emptyList; |
| public J visitFieldAccess(FieldAccess fieldAccess, ExecutionContext executionContext) { | ||
| boolean previousIsInsideFieldAccess = this.isInsideFieldAccess; | ||
| this.isInsideFieldAccess = true; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public J visitFieldAccess(FieldAccess fieldAccess, ExecutionContext executionContext) { | |
| boolean previousIsInsideFieldAccess = this.isInsideFieldAccess; | |
| this.isInsideFieldAccess = true; | |
| public J visitFieldAccess(FieldAccess fieldAccess, ExecutionContext ctx) { | |
| J result = super.visitFieldAccess(fieldAccess, ctx); | |
| public J visitIdentifier(J.Identifier identifier, ExecutionContext ctx) { | |
| J.Identifier id = (J.Identifier) super.visitIdentifier(identifier, ctx); |
| public J visitBlock(J.Block block, ExecutionContext executionContext) { | ||
| if (!block.isStatic()) { | ||
| return super.visitBlock(block, executionContext); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public J visitBlock(J.Block block, ExecutionContext executionContext) { | |
| if (!block.isStatic()) { | |
| return super.visitBlock(block, executionContext); | |
| } | |
| public J visitBlock(J.Block block, ExecutionContext ctx) { | |
| return super.visitBlock(block, ctx); | |
| J.Block result = (J.Block) super.visitBlock(block, ctx); | |
| public J visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { |
| this.isStatic = (methodType.getFlagsBitMap() & 0x0008L) != 0; | ||
| } | ||
|
|
||
| J.MethodDeclaration result = (J.MethodDeclaration) super.visitMethodDeclaration(method, executionContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| J.MethodDeclaration result = (J.MethodDeclaration) super.visitMethodDeclaration(method, executionContext); | |
| J.MethodDeclaration result = (J.MethodDeclaration) super.visitMethodDeclaration(method, ctx); |
| public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) { | ||
| J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, executionContext); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) { | |
| J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, executionContext); | |
| public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { | |
| J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, ctx); | |
| if ("super".equals(m.getName().getSimpleName()) || "this".equals(m.getName().getSimpleName())) { |
| String currentClassName = this.getSimpleClassName(currentClassType.getFullyQualifiedName()); | ||
| boolean currentIsAnonymous = this.isAnonymousClassName(currentClassName); | ||
| return new ClassContext(currentClassType, currentIsAnonymous); | ||
| } else if (currentCursor.getValue() instanceof J.NewClass) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } else if (currentCursor.getValue() instanceof J.NewClass) { | |
| } | |
| if (currentCursor.getValue() instanceof J.NewClass) { |
| spec.recipe(new ExplicitThis()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @Test | |
| @Test |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
| import java.time.Duration; | ||
| import java.util.Collections; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import java.time.Duration; | |
| import java.util.Collections; | |
| import java.time.Duration; |
| public J visitFieldAccess(FieldAccess fieldAccess, ExecutionContext executionContext) { | ||
| boolean previousIsInsideFieldAccess = this.isInsideFieldAccess; | ||
| this.isInsideFieldAccess = true; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public J visitFieldAccess(FieldAccess fieldAccess, ExecutionContext executionContext) { | |
| boolean previousIsInsideFieldAccess = this.isInsideFieldAccess; | |
| this.isInsideFieldAccess = true; | |
| public J visitFieldAccess(FieldAccess fieldAccess, ExecutionContext ctx) { | |
| J result = super.visitFieldAccess(fieldAccess, ctx); | |
| public J visitIdentifier(J.Identifier identifier, ExecutionContext ctx) { | |
| J.Identifier id = (J.Identifier) super.visitIdentifier(identifier, ctx); |
| public J visitBlock(J.Block block, ExecutionContext executionContext) { | ||
| if (!block.isStatic()) { | ||
| return super.visitBlock(block, executionContext); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public J visitBlock(J.Block block, ExecutionContext executionContext) { | |
| if (!block.isStatic()) { | |
| return super.visitBlock(block, executionContext); | |
| } | |
| public J visitBlock(J.Block block, ExecutionContext ctx) { | |
| return super.visitBlock(block, ctx); | |
| J.Block result = (J.Block) super.visitBlock(block, ctx); | |
| public J visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { |
| this.isStatic = (methodType.getFlagsBitMap() & 0x0008L) != 0; | ||
| } | ||
|
|
||
| J.MethodDeclaration result = (J.MethodDeclaration) super.visitMethodDeclaration(method, executionContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| J.MethodDeclaration result = (J.MethodDeclaration) super.visitMethodDeclaration(method, executionContext); | |
| J.MethodDeclaration result = (J.MethodDeclaration) super.visitMethodDeclaration(method, ctx); |
| public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) { | ||
| J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, executionContext); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) { | |
| J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, executionContext); | |
| public J visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { | |
| J.MethodInvocation m = (J.MethodInvocation) super.visitMethodInvocation(method, ctx); | |
| if ("super".equals(m.getName().getSimpleName()) || "this".equals(m.getName().getSimpleName())) { |
| p instanceof J.ClassDeclaration || | ||
| (p instanceof J.NewClass && ((J.NewClass) p).getBody() != null) || | ||
| p == Cursor.ROOT_VALUE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| p instanceof J.ClassDeclaration || | |
| (p instanceof J.NewClass && ((J.NewClass) p).getBody() != null) || | |
| p == Cursor.ROOT_VALUE | |
| p instanceof J.ClassDeclaration || | |
| (p instanceof J.NewClass && ((J.NewClass) p).getBody() != null) || | |
| p == Cursor.ROOT_VALUE |
| String currentClassName = this.getSimpleClassName(currentClassType.getFullyQualifiedName()); | ||
| boolean currentIsAnonymous = this.isAnonymousClassName(currentClassName); | ||
| return new ClassContext(currentClassType, currentIsAnonymous); | ||
| } else if (currentCursor.getValue() instanceof J.NewClass) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } else if (currentCursor.getValue() instanceof J.NewClass) { | |
| } | |
| if (currentCursor.getValue() instanceof J.NewClass) { |
| spec.recipe(new ExplicitThis()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @Test | |
| @Test |
1e30b0d to
a556fe4
Compare
a556fe4 to
d379d75
Compare
| return m; | ||
| } | ||
|
|
||
| if (m.getName().getSimpleName().equals("super") || m.getName().getSimpleName().equals("this")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (m.getName().getSimpleName().equals("super") || m.getName().getSimpleName().equals("this")) { | |
| if ("super".equals(m.getName().getSimpleName()) || "this".equals(m.getName().getSimpleName())) { |
| @Nullable | ||
| private ClassContext getCurrentClassContext() { | ||
| Cursor currentCursor = this.getCursor().dropParentUntil(p -> | ||
| p instanceof J.ClassDeclaration || | ||
| (p instanceof J.NewClass && ((J.NewClass) p).getBody() != null) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @Nullable | |
| private ClassContext getCurrentClassContext() { | |
| Cursor currentCursor = this.getCursor().dropParentUntil(p -> | |
| p instanceof J.ClassDeclaration || | |
| (p instanceof J.NewClass && ((J.NewClass) p).getBody() != null) || | |
| private @Nullable ClassContext getCurrentClassContext() { | |
| p instanceof J.ClassDeclaration || | |
| (p instanceof J.NewClass && ((J.NewClass) p).getBody() != null) || | |
| p == Cursor.ROOT_VALUE |
| String currentClassName = this.getSimpleClassName(currentClassType.getFullyQualifiedName()); | ||
| boolean currentIsAnonymous = this.isAnonymousClassName(currentClassName); | ||
| return new ClassContext(currentClassType, currentIsAnonymous); | ||
| } else if (currentCursor.getValue() instanceof J.NewClass) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } else if (currentCursor.getValue() instanceof J.NewClass) { | |
| } | |
| if (currentCursor.getValue() instanceof J.NewClass) { |
| @Nullable | ||
| private Expression createQualifiedThisExpression(ClassContext currentContext, JavaType.FullyQualified targetType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @Nullable | |
| private Expression createQualifiedThisExpression(ClassContext currentContext, JavaType.FullyQualified targetType) { | |
| private @Nullable Expression createQualifiedThisExpression(ClassContext currentContext, JavaType.FullyQualified targetType) { |
| spec.recipe(new ExplicitThis()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @Test | |
| @Test |
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