Skip to content

Commit

Permalink
Define assorted StaticImportCheck exemptions
Browse files Browse the repository at this point in the history 
While there, require that most `org.springframework.http.MediaType` members are
statically imported.
  • Loading branch information
@rickie
rickie authored Jan 3, 2022
1 parent 08e99fb commit c163806
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 12 deletions.
71 changes: 62 additions & 9 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,18 @@
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.fixes.SuggestedFixes;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.MemberSelectTree;
import com.sun.source.tree.MethodInvocationTree;
import com.sun.source.tree.Tree;
import com.sun.tools.javac.code.Type;
import java.util.Optional;

/** A {@link BugChecker} which flags methods that can and should be statically imported. */
/**
* A {@link BugChecker} which flags methods and constants that can and should be statically
* imported.
*/
// XXX: Tricky cases:
// - `org.springframework.http.MediaType` (do except for `ALL`?)
// - `org.springframework.http.HttpStatus` (not always an improvement, and `valueOf` must
// certainly be excluded)
// - `com.google.common.collect.Tables`
Expand All @@ -42,15 +46,19 @@
@AutoService(BugChecker.class)
@BugPattern(
name = "StaticImport",
summary = "Method should be statically imported",
summary = "Identifier should be statically imported",
linkType = LinkType.NONE,
severity = SeverityLevel.SUGGESTION,
tags = StandardTags.SIMPLIFICATION)
public final class StaticImportCheck extends BugChecker implements MemberSelectTreeMatcher {
private static final long serialVersionUID = 1L;

/**
* Types whose members should be statically imported, unless exempted by {@link
* #STATIC_IMPORT_EXEMPTED_MEMBERS} or {@link #STATIC_IMPORT_EXEMPTED_IDENTIFIERS}.
*/
@VisibleForTesting
static final ImmutableSet<String> STATIC_IMPORT_CANDIDATE_CLASSES =
static final ImmutableSet<String> STATIC_IMPORT_CANDIDATE_TYPES =
ImmutableSet.of(
"com.google.common.base.Preconditions",
"com.google.common.base.Predicates",
Expand Down Expand Up @@ -84,11 +92,13 @@ public final class StaticImportCheck extends BugChecker implements MemberSelectT
"org.springframework.format.annotation.DateTimeFormat.ISO",
"org.springframework.http.HttpHeaders",
"org.springframework.http.HttpMethod",
"org.springframework.http.MediaType",
"org.testng.Assert",
"reactor.function.TupleUtils");

/** Type members that should be statically imported. */
@VisibleForTesting
static final ImmutableSetMultimap<String, String> STATIC_IMPORT_CANDIDATE_METHODS =
static final ImmutableSetMultimap<String, String> STATIC_IMPORT_CANDIDATE_MEMBERS =
ImmutableSetMultimap.<String, String>builder()
.putAll(
"com.google.common.collect.ImmutableListMultimap",
Expand Down Expand Up @@ -123,9 +133,41 @@ public final class StaticImportCheck extends BugChecker implements MemberSelectT
.putAll("com.google.common.collect.Comparators", "emptiesFirst", "emptiesLast")
.build();

/**
* Type members that should never be statically imported.
*
* <p>Identifiers listed by {@link #STATIC_IMPORT_EXEMPTED_IDENTIFIERS} should be omitted from
* this collection.
*/
@VisibleForTesting
static final ImmutableSetMultimap<String, String> STATIC_IMPORT_EXEMPTED_MEMBERS =
ImmutableSetMultimap.<String, String>builder()
.put("com.mongodb.client.model.Filters", "empty")
.put("org.springframework.http.MediaType", "ALL")
.build();

/**
* Identifiers that should never be statically imported.
*
* <p>This should be a superset of the identifiers flagged by {@link
* com.google.errorprone.bugpatterns.BadImport}.
*/
@VisibleForTesting
static final ImmutableSet<String> STATIC_IMPORT_EXEMPTED_IDENTIFIERS =
ImmutableSet.of(
"builder",
"create",
"copyOf",
"from",
"getDefaultInstance",
"INSTANCE",
"newBuilder",
"of",
"valueOf");

@Override
public Description matchMemberSelect(MemberSelectTree tree, VisitorState state) {
if (!isCandidate(state)) {
if (!isCandidateContext(state) || !isCandidate(tree)) {
return Description.NO_MATCH;
}

Expand All @@ -140,7 +182,7 @@ public Description matchMemberSelect(MemberSelectTree tree, VisitorState state)
.orElse(Description.NO_MATCH);
}

private static boolean isCandidate(VisitorState state) {
private static boolean isCandidateContext(VisitorState state) {
Tree parentTree =
requireNonNull(state.getPath().getParentPath(), "MemberSelectTree lacks enclosing node")
.getLeaf();
Expand All @@ -155,15 +197,26 @@ private static boolean isCandidate(VisitorState state) {
}
}

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

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

private static Optional<String> getCandidateSimpleName(StaticImportInfo importInfo) {
String canonicalName = importInfo.canonicalName();
return importInfo
.simpleName()
.toJavaUtil()
.filter(
name ->
STATIC_IMPORT_CANDIDATE_CLASSES.contains(canonicalName)
|| STATIC_IMPORT_CANDIDATE_METHODS.containsEntry(canonicalName, name));
STATIC_IMPORT_CANDIDATE_TYPES.contains(canonicalName)
|| STATIC_IMPORT_CANDIDATE_MEMBERS.containsEntry(canonicalName, name));
}

private static Optional<Fix> tryStaticImport(
Expand Down
41 changes: 38 additions & 3 deletions ...prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/StaticImportCheckTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static org.assertj.core.api.Assertions.assertThat;

import com.google.errorprone.BugCheckerRefactoringTestHelper;
import com.google.errorprone.BugCheckerRefactoringTestHelper.TestMode;
import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.Test;

Expand All @@ -14,8 +15,20 @@ public final class StaticImportCheckTest {

@Test
void candidateMethodsAreNotRedundant() {
assertThat(StaticImportCheck.STATIC_IMPORT_CANDIDATE_METHODS.keySet())
.doesNotContainAnyElementsOf(StaticImportCheck.STATIC_IMPORT_CANDIDATE_CLASSES);
assertThat(StaticImportCheck.STATIC_IMPORT_CANDIDATE_MEMBERS.keySet())
.doesNotContainAnyElementsOf(StaticImportCheck.STATIC_IMPORT_CANDIDATE_TYPES);
}

@Test
void exemptedMembersAreNotVacuous() {
assertThat(StaticImportCheck.STATIC_IMPORT_EXEMPTED_MEMBERS.keySet())
.isSubsetOf(StaticImportCheck.STATIC_IMPORT_CANDIDATE_TYPES);
}

@Test
void exemptedMembersAreNotRedundant() {
assertThat(StaticImportCheck.STATIC_IMPORT_EXEMPTED_MEMBERS.values())
.doesNotContainAnyElementsOf(StaticImportCheck.STATIC_IMPORT_EXEMPTED_IDENTIFIERS);
}

@Test
Expand All @@ -38,6 +51,7 @@ void identification() {
"import java.util.function.Predicate;",
"import org.springframework.boot.test.context.SpringBootTest;",
"import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;",
"import org.springframework.http.MediaType;",
"",
"class A {",
" void m() {",
Expand Down Expand Up @@ -75,6 +89,11 @@ void identification() {
" Object e1 = WebEnvironment.RANDOM_PORT;",
" Object e2 = RANDOM_PORT;",
"",
" // Not flagged because `MediaType.ALL` is exempted.",
" MediaType t1 = MediaType.ALL;",
" // BUG: Diagnostic contains:",
" MediaType t2 = MediaType.APPLICATION_JSON;",
"",
" Optional.empty();",
" }",
"",
Expand All @@ -100,6 +119,7 @@ void replacement() {
"import org.springframework.format.annotation.DateTimeFormat.ISO;",
"import org.springframework.boot.test.context.SpringBootTest;",
"import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;",
"import org.springframework.http.MediaType;",
"",
"class A {",
" void m1() {",
Expand All @@ -117,6 +137,12 @@ void replacement() {
" Objects.requireNonNull(\"bar\");",
"",
" Object o = StandardCharsets.UTF_8;",
"",
" ImmutableSet.of(",
" MediaType.ALL,",
" MediaType.APPLICATION_XHTML_XML,",
" MediaType.TEXT_HTML,",
" MediaType.valueOf(\"image/webp\"));",
" }",
"",
" void m2(",
Expand Down Expand Up @@ -144,6 +170,8 @@ void replacement() {
"import static org.springframework.format.annotation.DateTimeFormat.ISO.DATE;",
"import static org.springframework.format.annotation.DateTimeFormat.ISO.DATE_TIME;",
"import static org.springframework.format.annotation.DateTimeFormat.ISO.TIME;",
"import static org.springframework.http.MediaType.APPLICATION_XHTML_XML;",
"import static org.springframework.http.MediaType.TEXT_HTML;",
"",
"import com.google.common.base.Predicates;",
"import com.google.common.collect.ImmutableMap;",
Expand All @@ -155,6 +183,7 @@ void replacement() {
"import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;",
"import org.springframework.format.annotation.DateTimeFormat;",
"import org.springframework.format.annotation.DateTimeFormat.ISO;",
"import org.springframework.http.MediaType;",
"",
"class A {",
" void m1() {",
Expand All @@ -172,6 +201,12 @@ void replacement() {
" requireNonNull(\"bar\");",
"",
" Object o = UTF_8;",
"",
" ImmutableSet.of(",
" MediaType.ALL,",
" APPLICATION_XHTML_XML,",
" TEXT_HTML,",
" MediaType.valueOf(\"image/webp\"));",
" }",
"",
" void m2(",
Expand All @@ -187,6 +222,6 @@ void replacement() {
" @SpringBootTest(webEnvironment = RANDOM_PORT)",
" final class Test {}",
"}")
.doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
.doTest(TestMode.TEXT_MATCH);
}
}

0 comments on commit c163806

Please sign in to comment.