Skip to content

Commit

Permalink
Improve testing setup and make tweaks
Browse files Browse the repository at this point in the history
  • Loading branch information
@rickie @Stephan202
rickie authored and Stephan202 committed Oct 14, 2023
1 parent 0be55f5 commit 3e63f49
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 80 deletions.
63 changes: 28 additions & 35 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/NonStaticImport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
import static com.google.errorprone.BugPattern.LinkType.CUSTOM;
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.BugPattern.StandardTags.STYLE;
import static com.sun.tools.javac.code.Kinds.Kind.TYP;
import static tech.picnic.errorprone.bugpatterns.StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS;
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;

import com.google.auto.service.AutoService;
Expand All @@ -24,9 +22,12 @@
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.Symbol.ClassSymbol;
import com.sun.tools.javac.code.Symbol.TypeSymbol;
import tech.picnic.errorprone.bugpatterns.util.SourceCode;

/** A {@link BugChecker} that flags methods and constants that should not be statically imported. */
/**
* A {@link BugChecker} that flags methods and constants that should *not* be statically imported.
*/
// XXX: Also introduce checks that disallows the following candidates:
// - `ZoneOffset.ofHours` and other `ofXXX`-style methods.
// - Several other `java.time` classes.
Expand All @@ -47,21 +48,21 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM
* <p>Types listed here should be mutually exclusive with {@link
* StaticImport#STATIC_IMPORT_CANDIDATE_TYPES}.
*/
static final ImmutableSet<String> BAD_STATIC_IMPORT_CANDIDATE_TYPES =
static final ImmutableSet<String> NON_STATIC_IMPORT_CANDIDATE_TYPES =
ImmutableSet.of("com.google.common.base.Strings", "java.time.Clock", "java.time.ZoneOffset");

/**
* Type members that should never be statically imported.
*
* <p>Identifiers listed by {@link #BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should be omitted
* <p>Identifiers listed by {@link #NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should be omitted
* from this collection.
*
* <p>This should be mutually exclusive with {@link StaticImport#STATIC_IMPORT_CANDIDATE_MEMBERS}.
*/
// XXX: Perhaps the set of exempted `java.util.Collections` methods is too strict. For now any
// method name that could be considered "too vague" or could conceivably mean something else in a
// specific context is left out.
static final ImmutableSetMultimap<String, String> BAD_STATIC_IMPORT_CANDIDATE_MEMBERS =
static final ImmutableSetMultimap<String, String> NON_STATIC_IMPORT_CANDIDATE_MEMBERS =
ImmutableSetMultimap.<String, String>builder()
.put("com.google.common.base.Predicates", "contains")
.put("com.mongodb.client.model.Filters", "empty")
Expand Down Expand Up @@ -92,7 +93,7 @@ public final class NonStaticImport extends BugChecker implements IdentifierTreeM
* <p>This should be a superset of the identifiers flagged by {@link
* com.google.errorprone.bugpatterns.BadImport}.
*/
static final ImmutableSet<String> BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS =
static final ImmutableSet<String> NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS =
ImmutableSet.of(
"builder",
"copyOf",
Expand All @@ -118,45 +119,34 @@ public NonStaticImport() {}
@Override
public Description matchIdentifier(IdentifierTree tree, VisitorState state) {
Symbol symbol = ASTHelpers.getSymbol(tree);
if (symbol == null || !isMatch(symbol, state)) {
if (symbol == null || !shouldAddQualifier(symbol, state)) {
return Description.NO_MATCH;
}

SuggestedFix.Builder builder = SuggestedFix.builder();
String replacement = SuggestedFixes.qualifyType(state, builder, symbol.owner) + ".";

return describeMatch(
tree,
builder
.removeStaticImport(getImportToRemove(symbol))
.prefixWith(tree, replacement)
.build());
return describeMatch(tree, builder.prefixWith(tree, replacement).build());
}

private static String getImportToRemove(Symbol symbol) {
return String.join(".", symbol.owner.getQualifiedName(), symbol.getSimpleName());
}

private static boolean isMatch(Symbol symbol, VisitorState state) {
Symbol owner = symbol.owner;
if (owner == null || owner.kind != TYP) {
return false;
}

if (isDefinedInThisFile(symbol, state.getPath().getCompilationUnit())) {
private static boolean shouldAddQualifier(Symbol symbol, VisitorState state) {
if (symbol instanceof TypeSymbol) {
return false;
}
String identifierName = symbol.getSimpleName().toString();
if (!isIdentifierStaticallyImported(identifierName, state)) {
return false;
}
if (isIdentifierDefinedInFile(symbol, state.getPath().getCompilationUnit())) {
return false;
}

String qualifiedTypeName = owner.getQualifiedName().toString();
return !isExempted(qualifiedTypeName, identifierName)
&& isCandidate(qualifiedTypeName, identifierName);
String qualifiedTypeName = symbol.owner.getQualifiedName().toString();
return !isStaticImportCandidateMember(qualifiedTypeName, identifierName)
&& isNonStaticImportCandidate(qualifiedTypeName, identifierName);
}

private static boolean isDefinedInThisFile(Symbol symbol, CompilationUnitTree tree) {
private static boolean isIdentifierDefinedInFile(Symbol symbol, CompilationUnitTree tree) {
return tree.getTypeDecls().stream()
.anyMatch(
t -> {
Expand All @@ -166,14 +156,17 @@ private static boolean isDefinedInThisFile(Symbol symbol, CompilationUnitTree tr
});
}

private static boolean isCandidate(String qualifiedTypeName, String identifierName) {
return BAD_STATIC_IMPORT_CANDIDATE_TYPES.contains(qualifiedTypeName)
|| BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(qualifiedTypeName, identifierName)
|| BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(identifierName);
private static boolean isNonStaticImportCandidate(
String qualifiedTypeName, String identifierName) {
return NON_STATIC_IMPORT_CANDIDATE_TYPES.contains(qualifiedTypeName)
|| NON_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(qualifiedTypeName, identifierName)
|| NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(identifierName);
}

private static boolean isExempted(String qualifiedTypeName, String identifierName) {
return STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(qualifiedTypeName, identifierName);
private static boolean isStaticImportCandidateMember(
String qualifiedTypeName, String identifierName) {
return StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(
qualifiedTypeName, identifierName);
}

private static boolean isIdentifierStaticallyImported(String identifierName, VisitorState state) {
Expand Down
18 changes: 9 additions & 9 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
import static com.google.errorprone.BugPattern.SeverityLevel.SUGGESTION;
import static com.google.errorprone.BugPattern.StandardTags.SIMPLIFICATION;
import static java.util.Objects.requireNonNull;
import static tech.picnic.errorprone.bugpatterns.NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS;
import static tech.picnic.errorprone.bugpatterns.NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS;
import static tech.picnic.errorprone.bugpatterns.NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS;
import static tech.picnic.errorprone.bugpatterns.NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS;
import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL;

import com.google.auto.service.AutoService;
Expand Down Expand Up @@ -48,11 +48,11 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa

/**
* Types whose members should be statically imported, unless exempted by {@link
* NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_MEMBERS} or {@link
* NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS}.
* NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_MEMBERS} or {@link
* NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS}.
*
* <p>Types listed here should be mutually exclusive with {@link
* NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_TYPES}.
* NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_TYPES}.
*/
static final ImmutableSet<String> STATIC_IMPORT_CANDIDATE_TYPES =
ImmutableSet.of(
Expand Down Expand Up @@ -104,9 +104,9 @@ public final class StaticImport extends BugChecker implements MemberSelectTreeMa
* Type members that should be statically imported.
*
* <p>This should be mutually exclusive with {@link
* NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_MEMBERS}.
* NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_MEMBERS}.
*
* <p>Identifiers listed by {@link NonStaticImport#BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should
* <p>Identifiers listed by {@link NonStaticImport#NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS} should
* be mutually exclusive with identifiers listed here.
*/
static final ImmutableSetMultimap<String, String> STATIC_IMPORT_CANDIDATE_MEMBERS =
Expand Down Expand Up @@ -183,13 +183,13 @@ private static boolean isCandidateContext(VisitorState state) {

private static boolean isCandidate(MemberSelectTree tree) {
String identifier = tree.getIdentifier().toString();
if (BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(identifier)) {
if (NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS.contains(identifier)) {
return false;
}

Type type = ASTHelpers.getType(tree.getExpression());
return type != null
&& !BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(type.toString(), identifier);
&& !NON_STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(type.toString(), identifier);
}

private static Optional<String> getCandidateSimpleName(StaticImportInfo importInfo) {
Expand Down
63 changes: 27 additions & 36 deletions ...r-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/NonStaticImportTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,28 @@
final class NonStaticImportTest {
@Test
void candidateMembersAreNotRedundant() {
assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.keySet())
.doesNotContainAnyElementsOf(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES);
assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS.keySet())
.doesNotContainAnyElementsOf(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_TYPES);

assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.values())
.doesNotContainAnyElementsOf(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS);
assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS.values())
.doesNotContainAnyElementsOf(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS);
}

@Test
void badTypesDontClashWithStaticImportCandidates() {
assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_TYPES)
assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_TYPES)
.doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_TYPES);
}

@Test
void badMembersDontClashWithStaticImportCandidates() {
assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_MEMBERS.entries())
assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_MEMBERS.entries())
.doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.entries());
}

@Test
void badIdentifiersDontClashWithStaticImportCandidates() {
assertThat(NonStaticImport.BAD_STATIC_IMPORT_CANDIDATE_IDENTIFIERS)
assertThat(NonStaticImport.NON_STATIC_IMPORT_CANDIDATE_IDENTIFIERS)
.doesNotContainAnyElementsOf(StaticImport.STATIC_IMPORT_CANDIDATE_MEMBERS.values());
}

Expand All @@ -56,7 +56,6 @@ void identification() {
"import static java.lang.Integer.MAX_VALUE;",
"import static java.lang.Integer.MIN_VALUE;",
"import static java.time.Clock.systemUTC;",
"import static java.time.Instant.MAX;",
"import static java.time.Instant.MIN;",
"import static java.time.ZoneOffset.UTC;",
"import static java.util.Collections.min;",
Expand All @@ -80,6 +79,7 @@ void identification() {
" // BUG: Diagnostic contains:",
" systemUTC();",
" Clock.systemUTC();",
" ZoneOffset utcIsExempted = UTC;",
"",
" // BUG: Diagnostic contains:",
" Optional<Integer> optional1 = empty();",
Expand All @@ -92,46 +92,27 @@ void identification() {
" // BUG: Diagnostic contains:",
" Locale locale1 = ROOT;",
" Locale locale2 = Locale.ROOT;",
" Locale isNotACandidate = ENGLISH;",
"",
" // BUG: Diagnostic contains:",
" Instant instant1 = MIN;",
" Instant instant2 = Instant.MIN;",
" // BUG: Diagnostic contains:",
" Instant instant3 = MAX;",
" Instant instant4 = Instant.MAX;",
"",
" // BUG: Diagnostic contains:",
" ImmutableSet.of(min(ImmutableSet.of()));",
"",
" Object builder = null;",
" // Not flagged because identifier is variable",
" Object lBuilder = ImmutableList.of(builder);",
"",
" // Not flagged because member of type is not a candidate.",
" Locale lo3 = ENGLISH;",
"",
" // Not flagged because member of type is exempted.",
" ZoneOffset utc = UTC;",
" ImmutableList<Object> list = ImmutableList.of(builder);",
"",
" // Not flagged because method is not statically imported.",
" create();",
"",
" // Not flagged because identifier is not statically imported.",
" // A member variable did overwrite the statically imported identifier.",
" Integer i1 = MIN_VALUE;",
"",
" // Not flagged because identifier is not statically imported.",
" Integer i2 = new B().MIN;",
"",
" // Not flagged because identifier is not statically imported.",
" Object inst = new INSTANCE();",
" Integer refersToMemberVariable = MIN_VALUE;",
" Integer minIsNotStatic = new B().MIN;",
" Object regularImport = new INSTANCE();",
" MAX_VALUE maxValue = new MAX_VALUE();",
"",
" Integer from = 12;",
" // Not flagged because identifier is not statically imported.",
" Integer i3 = from;",
" Integer i1 = from;",
"",
" // Not flagged because identifier does not belong to a type.",
" MAX_VALUE maxValue = new MAX_VALUE();",
" create();",
" }",
"",
" void create() {",
Expand All @@ -149,6 +130,7 @@ void replacement() {
.addInputLines(
"A.java",
"import static com.google.common.collect.ImmutableList.copyOf;",
"import static com.google.common.collect.ImmutableSet.of;",
"import static java.time.Clock.systemUTC;",
"import static java.time.Instant.MAX;",
"import static java.time.Instant.MIN;",
Expand Down Expand Up @@ -180,11 +162,20 @@ void replacement() {
" Instant i1 = MIN;",
" Instant i2 = MAX;",
"",
" ImmutableSet.of(min(ImmutableSet.of()));",
" ImmutableSet.of(min(of()));",
" }",
"}")
.addOutputLines(
"A.java",
"import static com.google.common.collect.ImmutableList.copyOf;",
"import static com.google.common.collect.ImmutableSet.of;",
"import static java.time.Clock.systemUTC;",
"import static java.time.Instant.MAX;",
"import static java.time.Instant.MIN;",
"import static java.util.Collections.min;",
"import static java.util.Locale.ROOT;",
"import static java.util.Optional.empty;",
"",
"import com.google.common.collect.ImmutableList;",
"import com.google.common.collect.ImmutableSet;",
"import java.time.Clock;",
Expand Down

0 comments on commit 3e63f49

Please sign in to comment.