Add support for specifying badEnclosingTypes for BadImport via flags

[hisener]

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]

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

[cushon]

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]

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

[hisener]

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

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]

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;
	}

[cushon]

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

[hisener]

Leaving it as-is then 👍