Add support for specifying badEnclosingTypes for `BadImport` via flags by hisener · Pull Request #4228 · google/error-prone

hisener · 2024-01-01T10:20:20Z

Following the discussions in #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:

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

->

final class Test {
  @Value.Immutable
  abstract class AbstractType {}
}

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

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 #2236.

hisener · 2024-01-01T10:24:57Z

+  @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();",


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)) {

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.

cushon

LGTM aside from the comment about Tree#toString. Thanks for the PR!

cushon · 2024-01-02T17:22:07Z

          return;
        }
-        if (BadImport.BAD_NESTED_CLASSES.contains(tree.getIdentifier().toString())) {
+        if (exemptedTypes.contains(tree.getExpression().toString())


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()));

👀 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.

cushon · 2024-01-02T17:41:20Z

+  @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();",


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.

hisener · 2024-01-02T20:14:50Z

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


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;

}

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

Leaving it as-is then 👍

Following the discussions in #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 #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 #2236. Fixes #4228 FUTURE_COPYBARA_INTEGRATE_REVIEW=#4228 from hisener:halil.sener/bad-import-bad-enclosing-types 67aa36d PiperOrigin-RevId: 595179563

Add support for specifying badEnclosingTypes for BadImport via flags

0c02c70

hisener commented Jan 1, 2024

View reviewed changes

hisener marked this pull request as ready for review January 1, 2024 10:25

hisener mentioned this pull request Jan 1, 2024

Exempt Immutable from UnnecessarilyFullyQualified #2236

Closed

cushon requested changes Jan 2, 2024

View reviewed changes

Avoid Tree#toString call

67aa36d

cushon approved these changes Jan 2, 2024

View reviewed changes

hisener commented Jan 2, 2024

View reviewed changes

copybara-service Bot mentioned this pull request Jan 2, 2024

Add support for specifying badEnclosingTypes for BadImport via flags #4231

Closed

copybara-service Bot closed this in bd0ba3f Jan 12, 2024

hisener deleted the halil.sener/bad-import-bad-enclosing-types branch January 12, 2024 12:08

jaredstehler mentioned this pull request Apr 26, 2024

Upgrade to v2.27.0 HubSpot/error-prone#86

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for specifying badEnclosingTypes for `BadImport` via flags#4228

Add support for specifying badEnclosingTypes for `BadImport` via flags#4228
hisener wants to merge 2 commits intogoogle:masterfrom
hisener:halil.sener/bad-import-bad-enclosing-types

hisener commented Jan 1, 2024 •

edited

Loading

Uh oh!

hisener Jan 1, 2024 •

edited

Loading

Uh oh!

cushon Jan 2, 2024

Uh oh!

cushon left a comment

Uh oh!

cushon Jan 2, 2024

Uh oh!

hisener Jan 2, 2024

Uh oh!

cushon Jan 2, 2024

Uh oh!

hisener Jan 2, 2024

Uh oh!

cushon Jan 2, 2024

Uh oh!

hisener Jan 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

Conversation

hisener commented Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hisener Jan 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cushon Jan 2, 2024

Choose a reason for hiding this comment

Uh oh!

cushon left a comment

Choose a reason for hiding this comment

Uh oh!

cushon Jan 2, 2024

Choose a reason for hiding this comment

Uh oh!

hisener Jan 2, 2024

Choose a reason for hiding this comment

Uh oh!

cushon Jan 2, 2024

Choose a reason for hiding this comment

Uh oh!

hisener Jan 2, 2024

Choose a reason for hiding this comment

Uh oh!

cushon Jan 2, 2024

Choose a reason for hiding this comment

Uh oh!

hisener Jan 2, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hisener commented Jan 1, 2024 •

edited

Loading

hisener Jan 1, 2024 •

edited

Loading