Skip to content

Commit

Permalink
Flag comparisons of SomeEnum.valueOf(...) to null.
Browse files Browse the repository at this point in the history
(also `SomeNumber.valueOf(...)`, but that comes up very rarely, enough so that I didn't bother to explore `Boolean.valueOf` or `String.valueOf`)

PiperOrigin-RevId: 629692421
  • Loading branch information
cpovirk authored and Error Prone Team committed May 1, 2024
1 parent 6a8f493 commit bc3309a
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,14 @@ private static boolean isNull(ExpressionTree tree) {

private final boolean matchTestAssertions;
private final boolean checkPrimitives;
private final boolean checkValueOf;

@Inject
ImpossibleNullComparison(ErrorProneFlags flags) {
this.matchTestAssertions =
flags.getBoolean("ProtoFieldNullComparison:MatchTestAssertions").orElse(true);
this.checkPrimitives = flags.getBoolean("ImmutableNullComparison:CheckPrimitives").orElse(true);
this.checkValueOf = flags.getBoolean("ImpossibleNullComparison:CheckValueOf").orElse(true);
}

@Override
Expand Down Expand Up @@ -262,6 +264,7 @@ private Optional<Fixer> getFixer(ExpressionTree tree, VisitorState state) {
}
return stream(GetterTypes.values())
.filter(gt -> !gt.equals(GetterTypes.PRIMITIVE) || checkPrimitives)
.filter(gt -> !gt.equals(GetterTypes.VALUE_OF) || checkValueOf)
.map(type -> type.match(resolvedTree, state))
.filter(Objects::nonNull)
.findFirst();
Expand Down Expand Up @@ -311,6 +314,12 @@ private interface Fixer {
private static final Matcher<ExpressionTree> TABLE_COLUMN_MATCHER =
instanceMethod().onDescendantOf("com.google.common.collect.Table").named("column");

private static final Matcher<ExpressionTree> NON_NULL_VALUE_OF =
staticMethod()
.onDescendantOfAny("java.lang.Enum", "java.lang.Number")
.named("valueOf")
.withParameters("java.lang.String");

private enum GetterTypes {
OPTIONAL_GET {
@Nullable
Expand Down Expand Up @@ -394,6 +403,17 @@ Fixer match(ExpressionTree tree, VisitorState state) {
return type != null && type.isPrimitive() ? GetterTypes::emptyFix : null;
}
},
VALUE_OF {
@Nullable
@Override
Fixer match(ExpressionTree tree, VisitorState state) {
if (!NON_NULL_VALUE_OF.matches(tree, state)) {
return null;
}
// TODO(cpovirk): Suggest Enums.getIfPresent, Ints.tryParse, etc.
return GetterTypes::emptyFix;
}
},
/** {@code proto.getFoo()} */
SCALAR {
@Nullable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -497,4 +497,22 @@ public void primitives() {
"}")
.doTest();
}

@Test
public void valueOf() {
compilationHelper
.addSourceLines(
"Test.java",
"import static com.google.common.truth.Truth.assertThat;",
"import java.util.concurrent.TimeUnit;",
"public class Test {",
" public void o(String s) {",
" // BUG: Diagnostic contains:",
" assertThat(TimeUnit.valueOf(s)).isNotNull();",
" // BUG: Diagnostic contains:",
" assertThat(Integer.valueOf(s)).isNotNull();",
" }",
"}")
.doTest();
}
}

0 comments on commit bc3309a

Please sign in to comment.