Skip to content
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.

Sign up for GitHub

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

StaticImportCheck support exemption of methods #22

Merged
merged 4 commits into from
Jan 3, 2022
Merged

StaticImportCheck support exemption of methods #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c87972f
PSM-1165 `StaticImportCheck` support exempting methods
rickie Dec 7, 2021
1c3b13b
PSM-1165 `StaticImportCheck` handle `package-info.java` files
rickie Dec 7, 2021
626d740
PSM-1165 Suggestions
Stephan202 Jan 2, 2022
fdd738c
PSM-1134 Drop excess test
Stephan202 Jan 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 62 additions & 9 deletions error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/StaticImportCheck.java
View file
Open in desktop
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The summary is now updated to have Identifier shouldn't that also be updated here in the Javadoc? @Stephan202

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that, but here I was thinking this might be a bit clearer albeit less accurate. (The summary is shown the context of a specific "did you mean" suggestion, in which case the "X or Y" feels a bit awkward.)

* 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")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add "valueOf" here as well, to make it clear. However, the case is already in BAD_STATIC_IDENTIFIERS so I left it out.

.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
Open in desktop
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);
}
}