Skip to content

Commit

Permalink
Optionally recommend against static import of Truth8.assertThat.
Browse files Browse the repository at this point in the history
The methods there will be moving into the main `Truth` class. Users will have various options for how to migrate, but one option involves avoiding static imports. We will provide more instructions as part of our Truth releases. (See, e.g., [the release notes for Truth 1.3.0](https://github.com/google/truth/releases/tag/v1.3.0).)

Rough outline of the option that involves avoiding static imports:

- The problem with adding `Truth8.assertThat(OptionalInt)` (or a similar overload) to `Truth` is that anyone who static imports both `Truth.assertThat` and `Truth8.assertThat` will see ambiguity errors if they use that method.
- The solution is to stop using static import for `Truth8.assertThat`. Once all its static imports are gone, we can add the new methods to `Truth` without breaking anyone, and then we can migrate users.
- Avoiding static import for `Truth8.assertThat` would be tough to swallow for the most common types, `Stream` and `Optional`. Luckily, I found an alternative approach that works for those, as discussed in the linked release notes. (That approach works for those types because they're generic types.)

PiperOrigin-RevId: 601232868
  • Loading branch information
cpovirk authored and Error Prone Team committed Jan 26, 2024
1 parent 7d1ae83 commit 8f4c5b5
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 14 deletions.
38 changes: 24 additions & 14 deletions core/src/main/java/com/google/errorprone/bugpatterns/BadImport.java
Original file line number Diff line number Diff line change
Expand Up @@ -116,15 +116,19 @@ public class BadImport extends BugChecker implements ImportTreeMatcher {
*/
private final ImmutableSet<String> badEnclosingTypes;

private final boolean warnAboutTruth8AssertThat;

@Inject
BadImport(ErrorProneFlags errorProneFlags) {
this.badEnclosingTypes = errorProneFlags.getSetOrEmpty("BadImport:BadEnclosingTypes");
this.warnAboutTruth8AssertThat = errorProneFlags.getBoolean("BadImport:Truth8").orElse(false);
}

@Override
public Description matchImport(ImportTree tree, VisitorState state) {
Symbol symbol;
ImmutableSet<Symbol> symbols;
boolean useTruth8Message = false;

if (!tree.isStatic()) {
symbol = getSymbol(tree.getQualifiedIdentifier());
Expand All @@ -143,7 +147,10 @@ public Description matchImport(ImportTree tree, VisitorState state) {

// Pick an arbitrary symbol. They've all got the same simple name, so it doesn't matter which.
symbol = symbols.iterator().next();
if (isAcceptableImport(symbol, BAD_STATIC_IDENTIFIERS)) {
if (warnAboutTruth8AssertThat && symbol.owner.name.contentEquals("Truth8")) {
useTruth8Message = true;
// Now we fall through, which treats the import as an unacceptable.
} else if (isAcceptableImport(symbol, BAD_STATIC_IDENTIFIERS)) {
return Description.NO_MATCH;
}
}
Expand All @@ -169,7 +176,19 @@ public Description matchImport(ImportTree tree, VisitorState state) {
SuggestedFixes.qualifyType(getCheckState(state), builder, symbol.getEnclosingElement())
+ ".";

return buildDescription(builder, symbols, replacement, state);
String message =
useTruth8Message
? "Avoid static import for Truth8.assertThat. While we usually recommend static import"
+ " for assertThat methods, static imports of Truth8.assertThat prevent us from"
+ " copying those methods to the main Truth class."
: String.format(
"Importing nested classes/static methods/static fields with commonly-used names can"
+ " make code harder to read, because it may not be clear from the context"
+ " exactly which type is being referred to. Qualifying the name with that of"
+ " the containing class can make the code clearer. Here we recommend using"
+ " qualified class: %s",
replacement);
return buildDescription(builder, symbols, replacement, state, message);
}

private static VisitorState getCheckState(VisitorState state) {
Expand Down Expand Up @@ -201,7 +220,8 @@ private Description buildDescription(
SuggestedFix.Builder builder,
Set<Symbol> symbols,
String enclosingReplacement,
VisitorState state) {
VisitorState state,
String message) {
CompilationUnitTree compilationUnit = state.getPath().getCompilationUnit();
TreePath path = TreePath.getPath(compilationUnit, compilationUnit);
IdentifierTree firstFound =
Expand Down Expand Up @@ -263,17 +283,7 @@ private void moveTypeAnnotations(
// import fix.
return Description.NO_MATCH;
}
return buildDescription(firstFound)
.setMessage(
String.format(
"Importing nested classes/static methods/static fields with commonly-used names can"
+ " make code harder to read, because it may not be clear from the context"
+ " exactly which type is being referred to. Qualifying the name with that of"
+ " the containing class can make the code clearer. Here we recommend using"
+ " qualified class: %s",
enclosingReplacement))
.addFix(builder.build())
.build();
return buildDescription(firstFound).setMessage(message).addFix(builder.build()).build();
}

private static boolean isTypeAnnotation(AnnotationTree t) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,23 @@ public void positive_static_differentOverloadsInvoked() {
.doTest();
}

@Test
public void positive_truth8AssertThatTrueFlag() {
compilationTestHelper
.setArgs("-XepOpt:BadImport:Truth8=true")
.addSourceLines(
"Test.java",
"import static com.google.common.truth.Truth8.assertThat;",
"import java.util.stream.IntStream;",
"class Test {",
" void x(IntStream s) {",
" // BUG: Diagnostic contains: usually recommend",
" assertThat(s).isEmpty();",
" }",
"}")
.doTest();
}

@Test
public void positive_static_locallyDefinedMethod() {
refactoringTestHelper
Expand Down Expand Up @@ -274,6 +291,36 @@ public void nestedTypeUseAnnotation() {
.doTest();
}

@Test
public void negative_truth8AssertThatFalseFlag() {
compilationTestHelper
.setArgs("-XepOpt:BadImport:Truth8=false")
.addSourceLines(
"Test.java",
"import static com.google.common.truth.Truth8.assertThat;",
"import java.util.stream.IntStream;",
"class Test {",
" void x(IntStream s) {",
" assertThat(s).isEmpty();",
" }",
"}")
.doTest();
}

@Test
public void negative_otherAssertThat() {
compilationTestHelper
.addSourceLines(
"Test.java",
"import static com.google.common.truth.Truth.assertThat;",
"class Test {",
" void x(Iterable<?> i) {",
" assertThat(i).isEmpty();",
" }",
"}")
.doTest();
}

@Test
public void suppressed_class() {
compilationTestHelper
Expand Down

0 comments on commit 8f4c5b5

Please sign in to comment.