-
Notifications
You must be signed in to change notification settings - Fork 39
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Introduce support for detecting conflicts when renaming methods
by moving reusable conflict detection code from `JUnitMethodDeclaration` to a new `ConflictDetection` class.
- Loading branch information
1 parent
14b5fa1
commit 1146e2a
Showing
3 changed files
with
147 additions
and
61 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
78 changes: 78 additions & 0 deletions
78
...rone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetection.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| package tech.picnic.errorprone.bugpatterns.util; | ||
|
|
||
| import static tech.picnic.errorprone.bugpatterns.util.JavaKeywords.isValidIdentifier; | ||
|
|
||
| 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.tools.javac.code.Symbol.MethodSymbol; | ||
| import com.sun.tools.javac.code.Type; | ||
| import java.util.Optional; | ||
|
|
||
| /** | ||
| * A set of helper methods for finding conflicts which would be caused by applying certain fixes. | ||
| */ | ||
| public final class ConflictDetection { | ||
| private ConflictDetection() {} | ||
|
|
||
| /** | ||
| * If applicable, returns a human-readable argument against assigning the given name to an | ||
| * existing method. | ||
| * | ||
| * <p>This method implements imperfect heuristics. Things it currently does not consider include | ||
| * the following: | ||
| * | ||
| * <ul> | ||
| * <li>Whether the rename would merely introduce a method overload, rather than clashing with an | ||
| * existing method declaration. | ||
| * <li>Whether the rename would cause a method in a superclass to be overridden. | ||
| * <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.) | ||
| * </ul> | ||
| * | ||
| * @param method The method considered for being renamed. | ||
| * @param newName The proposed name to assign. | ||
| * @param state The {@link VisitorState} to use for searching for blockers. | ||
| * @return A human-readable argument against assigning the proposed name to an existing method, or | ||
| * {@link Optional#empty()} if no blocker was found. | ||
| */ | ||
| public static Optional<String> findMethodRenameBlocker( | ||
| MethodSymbol method, String newName, VisitorState state) { | ||
| if (isExistingMethodName(method.owner.type, newName, state)) { | ||
| return Optional.of( | ||
| String.format( | ||
| "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 (!isValidIdentifier(newName)) { | ||
| return Optional.of(String.format("`%s` is not a valid identifier", newName)); | ||
| } | ||
|
|
||
| return Optional.empty(); | ||
| } | ||
|
|
||
| private static boolean isExistingMethodName(Type clazz, String name, VisitorState state) { | ||
| return ASTHelpers.matchingMethods(state.getName(name), method -> true, clazz, state.getTypes()) | ||
| .findAny() | ||
| .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 CharSequence getStaticImportSimpleName(Tree tree, VisitorState state) { | ||
| String source = SourceCode.treeToString(tree, state); | ||
| return source.subSequence(source.lastIndexOf('.') + 1, source.length()); | ||
| } | ||
| } |
67 changes: 67 additions & 0 deletions
67
...-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/util/ConflictDetectionTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| package tech.picnic.errorprone.bugpatterns.util; | ||
|
|
||
| import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; | ||
|
|
||
| import com.google.errorprone.BugPattern; | ||
| import com.google.errorprone.CompilationTestHelper; | ||
| import com.google.errorprone.VisitorState; | ||
| import com.google.errorprone.bugpatterns.BugChecker; | ||
| import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; | ||
| import com.google.errorprone.matchers.Description; | ||
| import com.google.errorprone.util.ASTHelpers; | ||
| import com.sun.source.tree.MethodTree; | ||
| import com.sun.tools.javac.code.Symbol; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| final class ConflictDetectionTest { | ||
| @Test | ||
| void matcher() { | ||
| CompilationTestHelper.newInstance(RenameBlockerFlagger.class, getClass()) | ||
| .addSourceLines( | ||
| "/A.java", | ||
| "import static pkg.B.foo3t;", | ||
| "", | ||
| "class A {", | ||
| " private void foo1() {", | ||
| " foo3t();", | ||
| " }", | ||
| "", | ||
| " // BUG: Diagnostic contains: a method named `foo2t` is already defined in this class or a", | ||
| " // supertype", | ||
| " private void foo2() {}", | ||
| "", | ||
| " private void foo2t() {}", | ||
| "", | ||
| " // BUG: Diagnostic contains: `foo3t` is already statically imported", | ||
| " private void foo3() {}", | ||
| "", | ||
| " // BUG: Diagnostic contains: `int` is not a valid identifier", | ||
| " private void in() {}", | ||
| "}") | ||
| .addSourceLines( | ||
| "/pkg/B.java", | ||
| "package pkg;", | ||
| "", | ||
| "public class B {", | ||
| " public static void foo3t() {}", | ||
| "}") | ||
| .doTest(); | ||
| } | ||
|
|
||
| /** | ||
| * A {@link BugChecker} that flags method rename blockers found by {@link | ||
| * ConflictDetection#findMethodRenameBlocker(Symbol.MethodSymbol, String, VisitorState)}. | ||
| */ | ||
| @BugPattern(summary = "Flags blockers for renaming methods", severity = ERROR) | ||
| public static final class RenameBlockerFlagger extends BugChecker implements MethodTreeMatcher { | ||
| private static final long serialVersionUID = 1L; | ||
|
|
||
| @Override | ||
| public Description matchMethod(MethodTree tree, VisitorState state) { | ||
| return ConflictDetection.findMethodRenameBlocker( | ||
| ASTHelpers.getSymbol(tree), tree.getName() + "t", state) | ||
| .map(blocker -> buildDescription(tree).setMessage(blocker).build()) | ||
| .orElse(Description.NO_MATCH); | ||
| } | ||
| } | ||
| } |