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

Jump to bottom

Add support for specifying badEnclosingTypes for BadImport via flags #4228

Closed
wants to merge 2 commits into from
Closed

Add support for specifying badEnclosingTypes for BadImport via flags #4228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c02c70
Add support for specifying badEnclosingTypes for `BadImport` via flags
hisener Jan 1, 2024
67aa36d
Avoid `Tree#toString` call
hisener Jan 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 26 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/BadImport.java
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@

import com.google.common.collect.ImmutableSet;
import com.google.errorprone.BugPattern;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.ImportTreeMatcher;
import com.google.errorprone.bugpatterns.StaticImports.StaticImportInfo;
Expand All @@ -47,6 +48,7 @@
import java.util.Arrays;
import java.util.List;
import java.util.Set;
import javax.inject.Inject;
import javax.lang.model.element.Name;

/**
Expand Down Expand Up @@ -99,6 +101,26 @@ public class BadImport extends BugChecker implements ImportTreeMatcher {

private static final String MESSAGE_LITE = "com.google.protobuf.MessageLite";

/**
* Enclosing types that their nested type imports are vague.
*
* <p>Some types are meant to provide a namespace; therefore, imports for their nested types can
* be confusing.
*
* <p>For instance, unlike its name suggests, {@code org.immutables.value.Value.Immutable} is used
* to generate immutable value types, and its import can be misleading. So, importing {@code
* org.immutables.value.Value} and using {@code @Value.Immutable} is more favorable than importing
* {@code org.immutables.value.Value.Immutable} and using {@code @Immutable}.
*
* <p>Note that this does not disallow import an enclosing type but its nested types instead.
*/
private final ImmutableSet<String> badEnclosingTypes;

@Inject
BadImport(ErrorProneFlags errorProneFlags) {
this.badEnclosingTypes = errorProneFlags.getSetOrEmpty("BadImport:BadEnclosingTypes");
}

@Override
public Description matchImport(ImportTree tree, VisitorState state) {
Symbol symbol;
Expand Down Expand Up @@ -168,9 +190,11 @@ private static VisitorState getCheckState(VisitorState state) {
TreePath.getPath(compilationUnit, ((ClassTree) tree).getMembers().get(0)));
}

private static boolean isAcceptableImport(Symbol symbol, Set<String> badNames) {
private boolean isAcceptableImport(Symbol symbol, Set<String> badNames) {
Name ownerName = symbol.owner.getQualifiedName();
Name simpleName = symbol.getSimpleName();
return badNames.stream().noneMatch(simpleName::contentEquals);
return badEnclosingTypes.stream().noneMatch(ownerName::contentEquals)
&& badNames.stream().noneMatch(simpleName::contentEquals);
}

private Description buildDescription(
Expand Down
46 changes: 41 additions & 5 deletions core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualified.java
View file
Open in desktop
Expand Up @@ -33,13 +33,15 @@
import com.google.common.collect.Table;
import com.google.errorprone.BugPattern;
import com.google.errorprone.BugPattern.SeverityLevel;
import com.google.errorprone.ErrorProneFlags;
import com.google.errorprone.VisitorState;
import com.google.errorprone.bugpatterns.BugChecker.CompilationUnitTreeMatcher;
import com.google.errorprone.fixes.SuggestedFix;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.util.ASTHelpers;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.CompilationUnitTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.IdentifierTree;
import com.sun.source.tree.ImportTree;
import com.sun.source.tree.MemberSelectTree;
Expand All @@ -60,6 +62,7 @@
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.inject.Inject;
import javax.lang.model.element.Name;

/** Flags uses of fully qualified names which are not ambiguous if imported. */
Expand All @@ -71,6 +74,25 @@ public final class UnnecessarilyFullyQualified extends BugChecker

private static final ImmutableSet<String> EXEMPTED_NAMES = ImmutableSet.of("Annotation");

/**
* Exempted types that fully qualified name usages are acceptable for their nested types when
* importing the enclosing type is ambiguous.
*
* <p>Some types are meant to provide a namespace; therefore, imports for their nested types can
* be confusing.
*
* <p>For instance, unlike its name suggests, {@code org.immutables.value.Value.Immutable} is used
* to generate immutable value types, and its import can be misleading. So, importing {@code
* org.immutables.value.Value} and using {@code @Value.Immutable} is more favorable than importing
* {@code org.immutables.value.Value.Immutable} and using {@code @Immutable}.
*/
private final ImmutableSet<String> exemptedEnclosingTypes;

@Inject
UnnecessarilyFullyQualified(ErrorProneFlags errorProneFlags) {
this.exemptedEnclosingTypes = errorProneFlags.getSetOrEmpty("BadImport:BadEnclosingTypes");
}

@Override
public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
if (tree.getTypeDecls().stream()
Expand Down Expand Up @@ -138,11 +160,8 @@ private void handle(TreePath path) {
if (!isFullyQualified(tree)) {
return;
}
if (BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString())) {
if (tree.getExpression() instanceof MemberSelectTree
&& getSymbol(tree.getExpression()) instanceof ClassSymbol) {
handle(new TreePath(path, tree.getExpression()));
}
if (isBadNestedClass(tree) || isExemptedEnclosingType(tree)) {
handle(new TreePath(path, tree.getExpression()));
return;
}
Symbol symbol = getSymbol(tree);
Expand All @@ -160,6 +179,23 @@ && getSymbol(tree.getExpression()) instanceof ClassSymbol) {
treePaths.add(path);
}

private boolean isBadNestedClass(MemberSelectTree tree) {
return BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString());
}

private boolean isExemptedEnclosingType(MemberSelectTree tree) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it make sense to early-exit if the flag is not set? Or, is the cost negligible? 🤔

Suggested change
private boolean isExemptedEnclosingType(MemberSelectTree tree) {
private boolean isExemptedEnclosingType(MemberSelectTree tree) {
if (exemptedEnclosingTypes.isEmpty()) {
return false;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Either way is fine with me, I think the cost is probably negligible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leaving it as-is then 👍

ExpressionTree expression = tree.getExpression();
if (!(expression instanceof MemberSelectTree)) {
return false;
}
Symbol symbol = getSymbol(expression);
if (!(symbol instanceof ClassSymbol)) {
return false;
}

return exemptedEnclosingTypes.contains(symbol.getQualifiedName().toString());
}

private boolean isFullyQualified(MemberSelectTree tree) {
AtomicBoolean isFullyQualified = new AtomicBoolean();
new SimpleTreeVisitor<Void, Void>() {
Expand Down
59 changes: 59 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/BadImportTest.java
View file
Open in desktop
Expand Up @@ -388,4 +388,63 @@ public void doesNotMatchProtos() {
"}")
.doTest();
}

@Test
public void badEnclosingTypes() {
refactoringTestHelper
.setArgs("-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value")
.addInputLines(
"org/immutables/value/Value.java",
"package org.immutables.value;",
"",
"public @interface Value {",
" @interface Immutable {}",
"}")
.expectUnchanged()
.addInputLines(
"Test.java",
"import org.immutables.value.Value.Immutable;",
"",
"@Immutable",
"interface Test {}")
.addOutputLines(
"Test.java",
"import org.immutables.value.Value;",
"",
"@Value.Immutable",
"interface Test {}")
.doTest();
}

@Test
public void badEnclosingTypes_doesNotMatchFullyQualifiedName() {
compilationTestHelper
.setArgs("-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value")
.addSourceLines(
"org/immutables/value/Value.java",
"package org.immutables.value;",
"",
"public @interface Value {",
" @interface Immutable {}",
"}")
.addSourceLines("Test.java", "@org.immutables.value.Value.Immutable", "interface Test {}")
.doTest();
}

@Test
public void badEnclosingTypes_staticMethod() {
compilationTestHelper
.setArgs("-XepOpt:BadImport:BadEnclosingTypes=com.google.common.collect.ImmutableList")
.addSourceLines(
"Test.java",
"import static com.google.common.collect.ImmutableList.toImmutableList;",
"import com.google.common.collect.ImmutableList;",
"import java.util.stream.Collector;",
"",
"class Test {",
" // BUG: Diagnostic contains: ImmutableList.toImmutableList()",
" Collector<?, ?, ImmutableList<Object>> immutableList = toImmutableList();",
Comment on lines +434 to +446
Copy link
Contributor Author

@hisener hisener Jan 1, 2024 •
edited

Choose a reason for hiding this comment

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

Is disallowing static import desirable? Should we change the behavior only for nested types? 🤔

error-prone/core/src/main/java/com/google/errorprone/bugpatterns/BadImport.java

Lines 107 to 109 in 63cf192

if (!tree.isStatic()) {
symbol = getSymbol(tree.getQualifiedIdentifier());
if (symbol == null || isAcceptableImport(symbol, BAD_NESTED_CLASSES)) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

The current behaviour of treating static imports consistently seems OK to me. Obviously toImmutableList is just an example, in general if com.example.Foo is supposed to provide a namespace and not have its members be imports, it seems OK to discourage both import com.example.Foo.Bar and import static com.example.Foo.baz.

"}")
.doTest();
}
}
91 changes: 91 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualifiedTest.java
View file
Open in desktop
Expand Up @@ -224,4 +224,95 @@ public void packageInfo() {
"package b;")
.doTest();
}

@Test
public void staticNestedClass() {
helper
.addInputLines(
"test/EnclosingType.java",
"package test;",
"",
"public final class EnclosingType {",
" public static final class StaticNestedClass {}",
"}")
.expectUnchanged()
.addInputLines(
"Test.java",
"interface Test {",
" test.EnclosingType.StaticNestedClass method();",
"}")
.addOutputLines(
"Test.java",
"import test.EnclosingType.StaticNestedClass;",
"interface Test {",
" StaticNestedClass method();",
"}")
.doTest();
}

@Test
public void exemptedEnclosingTypes() {
helper
.setArgs("-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value")
.addInputLines(
"org/immutables/value/Value.java",
"package org.immutables.value;",
"",
"public @interface Value {",
" @interface Immutable {}",
"}")
.expectUnchanged()
.addInputLines(
"Test.java",
"import org.immutables.value.Value.Immutable;",
"",
"class Test {",
" @org.immutables.value.Value.Immutable",
" abstract class AbstractType {}",
"}")
.addOutputLines(
"Test.java",
"import org.immutables.value.Value;",
"import org.immutables.value.Value.Immutable;",
"",
"class Test {",
" @Value.Immutable",
" abstract class AbstractType {}",
"}")
.doTest();
}

@Test
public void exemptedEnclosingTypes_importWouldBeAmbiguous() {
helper
.setArgs("-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value")
.addInputLines(
"org/immutables/value/Value.java",
"package org.immutables.value;",
"",
"public @interface Value {",
" @interface Immutable {}",
"}")
.expectUnchanged()
.addInputLines(
"annotation/Value.java",
"package annotation;",
"",
"public @interface Value {",
" String value();",
"}")
.expectUnchanged()
.addInputLines(
"Test.java",
"import annotation.Value;",
"",
"final class Test {",
" Test(@Value(\"test\") String value) {}",
"",
" @org.immutables.value.Value.Immutable",
" abstract class AbstractType {}",
"}")
.expectUnchanged()
.doTest();
}
}