Skip to content

Commit

Permalink
Improve JUnitMethodDeclaration method rename heuristics
Browse files Browse the repository at this point in the history
These changes make it less likely that the check suggests a method
rename that would yield invalid code.
  • Loading branch information
Stephan202 committed Dec 22, 2024
1 parent 53fe15c commit 0e476ac
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ void identification() {
CompilationTestHelper.newInstance(JUnitMethodDeclaration.class, getClass())
.addSourceLines(
"A.java",
"import static org.junit.jupiter.params.provider.Arguments.arguments;",
"import static org.junit.jupiter.params.provider.Arguments.*;",
"",
"import org.junit.jupiter.api.AfterAll;",
"import org.junit.jupiter.api.AfterEach;",
Expand Down Expand Up @@ -154,8 +154,10 @@ void identification() {
" void overload() {}",
"",
" @Test",
" // BUG: Diagnostic contains: (but note that `arguments` is already statically imported)",
" void testArguments() {}",
" // BUG: Diagnostic contains: (but note that another method named `arguments` is in scope)",
" void testArguments() {",
" arguments();",
" }",
"",
" @Test",
" // BUG: Diagnostic contains: (but note that `public` is not a valid identifier)",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@

import com.google.errorprone.VisitorState;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ImportTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.util.TreeScanner;
import com.sun.tools.javac.code.Symbol.MethodSymbol;
import com.sun.tools.javac.code.Type;
import java.util.Optional;
import org.jspecify.annotations.Nullable;

/** A set of helper methods for detecting conflicts that would be caused when applying fixes. */
public final class ConflictDetection {
Expand All @@ -22,9 +26,10 @@ private ConflictDetection() {}
* <ul>
* <li>Whether the rename would merely introduce a method overload, rather than clashing with an
* existing method declaration in its class or a supertype.
* <li>Whether the rename would in fact clash with a static import. (It could be that a static
* import of the same name is only referenced from lexical scopes in which the method under
* consideration cannot be referenced directly.)
* <li>Whether the rename would in fact change the target of an existing method invocation in
* the scope of its containing class. (It could e.g. be that said invocation targets an
* identically-named method with different parameter types in some non-static nested type
* declaration.)
* </ul>
*
* @param method The method considered for renaming.
Expand All @@ -41,8 +46,8 @@ public static Optional<String> findMethodRenameBlocker(
"a method named `%s` is already defined in this class or a supertype", newName));
}

if (isSimpleNameStaticallyImported(newName, state)) {
return Optional.of(String.format("`%s` is already statically imported", newName));
if (isLocalMethodInvocation(newName, state)) {
return Optional.of(String.format("another method named `%s` is in scope", newName));
}

if (!SourceCode.isValidIdentifier(newName)) {
Expand All @@ -58,16 +63,38 @@ private static boolean isExistingMethodName(Type clazz, String name, VisitorStat
.isPresent();
}

private static boolean isSimpleNameStaticallyImported(String simpleName, VisitorState state) {
return state.getPath().getCompilationUnit().getImports().stream()
.filter(ImportTree::isStatic)
.map(ImportTree::getQualifiedIdentifier)
.map(tree -> getStaticImportSimpleName(tree, state))
.anyMatch(simpleName::contentEquals);
}
private static boolean isLocalMethodInvocation(String name, VisitorState state) {
return Boolean.TRUE.equals(
new TreeScanner<Boolean, @Nullable Void>() {
@Override
public Boolean visitClass(ClassTree tree, @Nullable Void unused) {
if (ASTHelpers.getSymbol(tree).isStatic()) {
/*
* Don't descend into static type definitions: in those context, any unqualified
* method invocation cannot refer to a method in the outer scope.
*/
return Boolean.FALSE;
}

return super.visitClass(tree, null);
}

@Override
public Boolean visitMethodInvocation(MethodInvocationTree tree, @Nullable Void unused) {
ExpressionTree methodSelect = tree.getMethodSelect();
if (methodSelect instanceof IdentifierTree identifier) {
if (name.contentEquals(identifier.getName())) {
return Boolean.TRUE;
}
}

return super.visitMethodInvocation(tree, null);
}

private static CharSequence getStaticImportSimpleName(Tree tree, VisitorState state) {
String source = SourceCode.treeToString(tree, state);
return source.subSequence(source.lastIndexOf('.') + 1, source.length());
@Override
public Boolean reduce(Boolean r1, Boolean r2) {
return Boolean.TRUE.equals(r1) || Boolean.TRUE.equals(r2);
}
}.scan(state.findEnclosing(ClassTree.class).getMembers(), null));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,13 @@ void matcher() {
"pkg/A.java",
"package pkg;",
"",
"import static pkg.A.B.method3t;",
"import static pkg.A.StaticType.method3t;",
"import static pkg.A.StaticType.method4t;",
"",
"import pkg.A.method4t;",
"",
"class A {",
" void method1() {",
" method3t();",
" method4(method4t.class);",
" }",
"",
Expand All @@ -37,16 +37,42 @@ void matcher() {
"",
" void method2t() {}",
"",
" // BUG: Diagnostic contains: `method3t` is already statically imported",
" // BUG: Diagnostic contains: another method named `method3t` is in scope",
" void method3() {}",
"",
" void method4(Object o) {}",
"",
" // BUG: Diagnostic contains: `int` is not a valid identifier",
" void in() {}",
"",
" static class B {",
" static void method3t() {}",
" class InstanceType {",
" void m() {",
" System.out.println(method3t());",
" }",
" }",
"",
" static class StaticType {",
" static int method3t() {",
" return 0;",
" }",
"",
" static void method4t() {",
" method4t();",
" }",
" }",
"",
" record RecordType() {",
" void m() {",
" method4t();",
" }",
" }",
"",
" enum EnumType {",
" ELEM;",
"",
" void m() {",
" method4t();",
" }",
" }",
"",
" class method4t {}",
Expand Down

0 comments on commit 0e476ac

Please sign in to comment.