-
Notifications
You must be signed in to change notification settings - Fork 37
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
Introduce ConflictDetection
utility class
#478
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
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 detecting conflicts that would be caused when applying 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 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.) | ||
* </ul> | ||
* | ||
* @param method The method considered for renaming. | ||
* @param newName The newly proposed name for the method. | ||
* @param state The {@link VisitorState} to use when searching for blockers. | ||
* @return A human-readable argument against assigning the proposed name to the given 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()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,73 @@ | ||||||
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.MethodSymbol; | ||||||
import org.junit.jupiter.api.Test; | ||||||
|
||||||
final class ConflictDetectionTest { | ||||||
@Test | ||||||
void matcher() { | ||||||
CompilationTestHelper.newInstance(RenameBlockerFlagger.class, getClass()) | ||||||
.addSourceLines( | ||||||
"pkg/A.java", | ||||||
"package pkg;", | ||||||
"", | ||||||
"import static pkg.A.B.method3t;", | ||||||
"", | ||||||
"import pkg.A.method4t;", | ||||||
"", | ||||||
"class A {", | ||||||
" void method1() {", | ||||||
" method3t();", | ||||||
" method4(method4t.class);", | ||||||
" }", | ||||||
"", | ||||||
" // BUG: Diagnostic contains: a method named `method2t` is already defined in this class or a", | ||||||
" // supertype", | ||||||
" void method2() {}", | ||||||
"", | ||||||
" void method2t() {}", | ||||||
"", | ||||||
" // BUG: Diagnostic contains: `method3t` is already statically imported", | ||||||
" void method3() {}", | ||||||
"", | ||||||
" void method4(Object o) {}", | ||||||
"", | ||||||
" // BUG: Diagnostic contains: `int` is not a valid identifier", | ||||||
" void in() {}", | ||||||
"", | ||||||
" static class B {", | ||||||
" static void method3t() {}", | ||||||
" }", | ||||||
"", | ||||||
" class method4t {}", | ||||||
"}") | ||||||
.doTest(); | ||||||
} | ||||||
|
||||||
/** | ||||||
* A {@link BugChecker} that uses {@link ConflictDetection#findMethodRenameBlocker(MethodSymbol, | ||||||
* String, VisitorState)} to flag methods of which the name cannot be suffixed with a {@code t}. | ||||||
*/ | ||||||
@BugPattern(summary = "Interacts with `ConflictDetection` for testing purposes", 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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clearer (or some variation hereof):
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes but then indeed the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exactly 😄. Forgot to remove this comment 👍 |
||||||
.map(blocker -> buildDescription(tree).setMessage(blocker).build()) | ||||||
.orElse(Description.NO_MATCH); | ||||||
} | ||||||
} | ||||||
} |
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.
Fine for now, but I suspect that eventually we'll want a more generic name. Time will tell :)