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 1 commit
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
Original file line number Diff line number Diff line change
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
24 changes: 23 additions & 1 deletion core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualified.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
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;
Expand Down Expand Up @@ -60,6 +61,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 +73,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> exemptedTypes;

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

@Override
public Description matchCompilationUnit(CompilationUnitTree tree, VisitorState state) {
if (tree.getTypeDecls().stream()
Expand Down Expand Up @@ -138,7 +159,8 @@ private void handle(TreePath path) {
if (!isFullyQualified(tree)) {
return;
}
if (BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString())) {
if (exemptedTypes.contains(tree.getExpression().toString())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is fine because of the guarantees made by the checks in isFullyQualified, but we generally try to avoid using Tree#toString. There's some performance cost for larger trees, and the string representation isn't stable.

What do you think about something like:

if (!BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString())) {
  return;
}
if (!(tree.getExpression() instanceof MemberSelectTree)) {
  return;
}
Symbol sym = getSymbol(tree.getExpression());
if (!(sym instanceof ClassSymbol)) {
  return;
}
if (exemptedTypes.contains(((ClassSymbol) sym).getQualifiedName().toString())) {
  return;
}
handle(new TreePath(path, tree.getExpression()));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👀 We shouldn't directly return if it's not a bad nested class, but I think I got your point.

I tried to refactor in 67aa36d, PTAL.

|| BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString())) {
if (tree.getExpression() instanceof MemberSelectTree
&& getSymbol(tree.getExpression()) instanceof ClassSymbol) {
handle(new TreePath(path, tree.getExpression()));
Expand Down
59 changes: 59 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/BadImportTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
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();
}
}
66 changes: 66 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualifiedTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,4 +224,70 @@ public void packageInfo() {
"package b;")
.doTest();
}

@Test
public void exemptedTypes() {
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 exemptedTypes_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();
}
}