diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java index e5725f74a0d..235a1d0577e 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/CollectorMutability.java @@ -82,13 +82,14 @@ private Description suggestToCollectionAlternatives( String toCollectionSelect = SuggestedFixes.qualifyStaticImport( "java.util.stream.Collectors.toCollection", mutableFix, state); + String mutableCollection = + SuggestedFixes.qualifyType(state, mutableFix, "java.util." + mutableReplacement); return buildDescription(tree) .addFix(replaceMethodInvocation(tree, fullyQualifiedImmutableReplacement, state)) .addFix( mutableFix - .addImport(String.format("java.util.%s", mutableReplacement)) - .replace(tree, String.format("%s(%s::new)", toCollectionSelect, mutableReplacement)) + .replace(tree, String.format("%s(%s::new)", toCollectionSelect, mutableCollection)) .build()) .build(); } @@ -99,17 +100,19 @@ private Description suggestToMapAlternatives(MethodInvocationTree tree, VisitorS return Description.NO_MATCH; } + SuggestedFix.Builder mutableFix = SuggestedFix.builder(); + String hashMap = SuggestedFixes.qualifyType(state, mutableFix, "java.util.HashMap"); + return buildDescription(tree) .addFix( replaceMethodInvocation( tree, "com.google.common.collect.ImmutableMap.toImmutableMap", state)) .addFix( - SuggestedFix.builder() - .addImport("java.util.HashMap") + mutableFix .postfixWith( tree.getArguments().get(argCount - 1), (argCount == 2 ? ", (a, b) -> { throw new IllegalStateException(); }" : "") - + ", HashMap::new") + + String.format(", %s::new", hashMap)) .build()) .build(); } diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImportSuggestion.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImportSuggestion.java new file mode 100644 index 00000000000..61a93baeec6 --- /dev/null +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/ImportSuggestion.java @@ -0,0 +1,66 @@ +package tech.picnic.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.LinkType.CUSTOM; +import static com.google.errorprone.BugPattern.SeverityLevel.WARNING; +import static com.google.errorprone.BugPattern.StandardTags.FRAGILE_CODE; +import static com.google.errorprone.matchers.Matchers.instanceMethod; +import static tech.picnic.errorprone.bugpatterns.util.Documentation.BUG_PATTERNS_BASE_URL; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; + +/** + * A {@link BugChecker} that flags suggested fixes that involve unconditional imports. + * + *

Such unconditional imports may clash with other imports of the same identifier. + */ +@AutoService(BugChecker.class) +@BugPattern( + summary = "Avoid direct invocation of `SuggestedFix#add{,Static}Import`", + link = BUG_PATTERNS_BASE_URL + "ImportSuggestion", + linkType = CUSTOM, + severity = WARNING, + tags = FRAGILE_CODE) +public final class ImportSuggestion extends BugChecker implements MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + private static final Matcher FIX_BUILDER_METHOD = + instanceMethod().onDescendantOf(SuggestedFix.Builder.class.getCanonicalName()); + + /** Instantiates a new {@link ImportSuggestion} instance. */ + public ImportSuggestion() {} + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (!FIX_BUILDER_METHOD.matches(tree, state)) { + return Description.NO_MATCH; + } + + switch (ASTHelpers.getSymbol(tree).name.toString()) { + case "addImport": + return createDescription( + tree, "SuggestedFix.Builder#addImport", "SuggestedFixes#qualifyType"); + case "addStaticImport": + return createDescription( + tree, "SuggestedFix.Builder#addStaticImport", "SuggestedFixes#qualifyStaticImport"); + default: + return Description.NO_MATCH; + } + } + + private Description createDescription( + MethodInvocationTree tree, String method, String alternative) { + return buildDescription(tree) + .setMessage( + String.format("Prefer `%s` over direct invocation of `%s`", alternative, method)) + .build(); + } +} diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java index e6fe6df8529..ff7ef3a12b8 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/JUnitValueSource.java @@ -30,6 +30,7 @@ import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; @@ -199,16 +200,21 @@ private static Optional tryConstructValueSourceFix( return getSingleReturnExpression(valueFactoryMethod) .flatMap(expression -> tryExtractValueSourceAttributeValue(expression, state)) .map( - valueSourceAttributeValue -> - SuggestedFix.builder() - .addImport("org.junit.jupiter.params.provider.ValueSource") - .replace( - methodSourceAnnotation, - String.format( - "@ValueSource(%s = %s)", - toValueSourceAttributeName(parameterType), valueSourceAttributeValue)) - .delete(valueFactoryMethod) - .build()); + valueSourceAttributeValue -> { + SuggestedFix.Builder fix = SuggestedFix.builder(); + String valueSource = + SuggestedFixes.qualifyType( + state, fix, "org.junit.jupiter.params.provider.ValueSource"); + return fix.replace( + methodSourceAnnotation, + String.format( + "@%s(%s = %s)", + valueSource, + toValueSourceAttributeName(parameterType), + valueSourceAttributeValue)) + .delete(valueFactoryMethod) + .build(); + }); } // XXX: This pattern also occurs a few times inside Error Prone; contribute upstream. diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java index 6c75d67e823..3b812822b41 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/MethodReferenceUsage.java @@ -196,6 +196,8 @@ private static ImmutableList getVariables(LambdaExpressionTree tree) { return tree.getParameters().stream().map(VariableTree::getName).collect(toImmutableList()); } + // XXX: Resolve this suppression. + @SuppressWarnings("ImportSuggestion") private static Optional constructFix( LambdaExpressionTree lambdaExpr, Symbol target, Object methodName) { Name sName = target.getSimpleName(); diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparison.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparison.java index 9f2d2f82198..d195713119d 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparison.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/PrimitiveComparison.java @@ -17,6 +17,7 @@ import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; import com.google.errorprone.fixes.Fix; import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.util.ASTHelpers; @@ -167,10 +168,11 @@ private static Fix suggestFix( ExpressionTree expr = tree.getMethodSelect(); switch (expr.getKind()) { case IDENTIFIER: - return SuggestedFix.builder() - .addStaticImport(Comparator.class.getName() + '.' + preferredMethodName) - .replace(expr, preferredMethodName) - .build(); + SuggestedFix.Builder fix = SuggestedFix.builder(); + String replacement = + SuggestedFixes.qualifyStaticImport( + Comparator.class.getName() + '.' + preferredMethodName, fix, state); + return fix.replace(expr, replacement).build(); case MEMBER_SELECT: MemberSelectTree ms = (MemberSelectTree) tree.getMethodSelect(); return SuggestedFix.replace( diff --git a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpringMvcAnnotation.java b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpringMvcAnnotation.java index 97cad608058..1b2119ab12f 100644 --- a/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpringMvcAnnotation.java +++ b/error-prone-contrib/src/main/java/tech/picnic/errorprone/bugpatterns/SpringMvcAnnotation.java @@ -18,6 +18,7 @@ import com.google.errorprone.bugpatterns.BugChecker.AnnotationTreeMatcher; import com.google.errorprone.fixes.Fix; import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.AssignmentTree; @@ -114,9 +115,8 @@ private static Fix replaceAnnotation( .map(arg -> SourceCode.treeToString(arg, state)) .collect(joining(", ")); - return SuggestedFix.builder() - .addImport(ANN_PACKAGE_PREFIX + newAnnotation) - .replace(tree, String.format("@%s(%s)", newAnnotation, newArguments)) - .build(); + SuggestedFix.Builder fix = SuggestedFix.builder(); + String annotation = SuggestedFixes.qualifyType(state, fix, ANN_PACKAGE_PREFIX + newAnnotation); + return fix.replace(tree, String.format("@%s(%s)", annotation, newArguments)).build(); } } diff --git a/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImportSuggestionTest.java b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImportSuggestionTest.java new file mode 100644 index 00000000000..d719e838d65 --- /dev/null +++ b/error-prone-contrib/src/test/java/tech/picnic/errorprone/bugpatterns/ImportSuggestionTest.java @@ -0,0 +1,44 @@ +package tech.picnic.errorprone.bugpatterns; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.Test; + +final class ImportSuggestionTest { + @Test + void identification() { + CompilationTestHelper.newInstance(ImportSuggestion.class, getClass()) + .expectErrorMessage( + "IMPORT", + m -> + m.contains( + "Prefer `SuggestedFixes#qualifyType` over direct invocation of `SuggestedFix.Builder#addImport`")) + .expectErrorMessage( + "STATIC_IMPORT", + m -> + m.contains( + "Prefer `SuggestedFixes#qualifyStaticImport` over direct invocation of `SuggestedFix.Builder#addStaticImport`")) + .addSourceLines( + "A.java", + "import com.google.errorprone.fixes.SuggestedFix;", + "", + "class A {", + " void m() {", + " System.out.println(\"foo\");", + " addImport(\"bar\");", + " addStaticImport(\"baz\");", + "", + " SuggestedFix.Builder builder = SuggestedFix.builder();", + " // BUG: Diagnostic matches: IMPORT", + " builder.addImport(\"java.lang.String\");", + " // BUG: Diagnostic matches: STATIC_IMPORT", + " builder.addStaticImport(\"java.lang.String.toString\");", + " builder.build();", + " }", + "", + " private void addImport(String s) {}", + "", + " private void addStaticImport(String s) {}", + "}") + .doTest(); + } +}