Skip to content

Commit

Permalink
Add support for specifying badEnclosingTypes for BadImport via flags
Browse files Browse the repository at this point in the history 
Following the discussions in google#2236, this PR introduces the `BadImport:BadEnclosingTypes` flag to disallow nested types of specified enclosing type.

This PR also takes the flag into account in `UnnecessarilyFullyQualified` to suggest importing `EnclosingType.NestedType` when possible.

For instance, when `-XepOpt:BadImport:BadEnclosingTypes=org.immutables.value.Value` is set, it would suggest the following:

```java
final class Test {
  @org.immutables.value.Value.Immutable
  abstract class AbstractType {}
}
```
->
```java
final class Test {
  @Value.Immutable
  abstract class AbstractType {}
}
```

Also, it won't require suppressing `UnnecessarilyFullyQualified` in the example in google#2236.

```java
import org.springframework.beans.factory.annotation.Value;

final class Test {
  Demo(@value("some.property") String property) {}

  @org.immutables.value.Value.Immutable
  abstract class AbstractType {}
}
```

Closes google#2236.

Fixes google#4228

COPYBARA_INTEGRATE_REVIEW=google#4228 from hisener:halil.sener/bad-import-bad-enclosing-types 67aa36d
PiperOrigin-RevId: 597798125
  • Loading branch information
@hisener
hisener authored and Error Prone Team committed Jan 12, 2024
1 parent 5a68588 commit bd0ba3f
Show file tree
Hide file tree
Showing 4 changed files with 217 additions and 7 deletions.
28 changes: 26 additions & 2 deletions core/src/main/java/com/google/errorprone/bugpatterns/BadImport.java
View file
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
46 changes: 41 additions & 5 deletions core/src/main/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualified.java
View file
Original file line number Diff line number Diff line change
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) {
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
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();",
"}")
.doTest();
}
}
91 changes: 91 additions & 0 deletions core/src/test/java/com/google/errorprone/bugpatterns/UnnecessarilyFullyQualifiedTest.java
View file
Original file line number Diff line number Diff line change
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();
}
}

0 comments on commit bd0ba3f

Please sign in to comment.