From 9abc44f9349b465716cfd890ff49435f8dc7afdb Mon Sep 17 00:00:00 2001 From: cpovirk Date: Thu, 9 Oct 2025 06:32:29 -0700 Subject: [PATCH] Produce a build error for calls like `containsExactly(null)` that always lead to NPE. These calls need to be written as `containsExactly((Object) null)` instead. javac produces a warning for such cases, but it's easy to ignore and somewhat difficult to understand. PiperOrigin-RevId: 817159968 --- .../bugpatterns/NullNeedsCastForVarargs.java | 200 ++++++++ .../scanner/BuiltInCheckerSuppliers.java | 2 + .../NullNeedsCastForVarargsTest.java | 427 ++++++++++++++++++ .../nullness/NullNeedsCastForVarargs.md | 2 + 4 files changed, 631 insertions(+) create mode 100644 core/src/main/java/com/google/errorprone/bugpatterns/NullNeedsCastForVarargs.java create mode 100644 core/src/test/java/com/google/errorprone/bugpatterns/NullNeedsCastForVarargsTest.java create mode 100644 docs/bugpattern/nullness/NullNeedsCastForVarargs.md diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NullNeedsCastForVarargs.java b/core/src/main/java/com/google/errorprone/bugpatterns/NullNeedsCastForVarargs.java new file mode 100644 index 00000000000..aa396314c53 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NullNeedsCastForVarargs.java @@ -0,0 +1,200 @@ +/* + * Copyright 2025 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns; + +import static com.google.errorprone.BugPattern.SeverityLevel.ERROR; +import static com.google.errorprone.fixes.SuggestedFix.replace; +import static com.google.errorprone.fixes.SuggestedFixes.qualifyType; +import static com.google.errorprone.matchers.Description.NO_MATCH; +import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.method.MethodMatchers.instanceMethod; +import static com.google.errorprone.matchers.method.MethodMatchers.staticMethod; +import static com.google.errorprone.suppliers.Suppliers.OBJECT_TYPE; +import static com.google.errorprone.suppliers.Suppliers.arrayOf; +import static com.google.errorprone.util.ASTHelpers.getType; +import static com.google.errorprone.util.TargetType.targetType; +import static com.sun.source.tree.Tree.Kind.NULL_LITERAL; +import static javax.lang.model.type.TypeKind.ARRAY; + +import com.google.common.collect.ImmutableList; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.suppliers.Supplier; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.TypeCastTree; +import com.sun.tools.javac.code.Type; +import com.sun.tools.javac.code.Type.ArrayType; +import java.util.List; + +/** A {@link BugChecker}; see the associated {@link BugPattern} annotation for details. */ +@BugPattern( + summary = + "This call passes a null *array*, so it always produces NullPointerException. To pass a" + + " null *element*, cast to the element type.", + severity = ERROR) +public final class NullNeedsCastForVarargs extends BugChecker + implements MethodInvocationTreeMatcher { + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + var arguments = tree.getArguments(); + + if (METHOD_WITH_SOLE_PARAMETER_OBJECT_VARARGS.matches(tree, state)) { + return matchObjectVarargs(arguments, /* varargsStart= */ 0); + } else if (METHOD_WITH_SECOND_PARAMETER_OBJECT_VARARGS.matches(tree, state)) { + return matchObjectVarargs(arguments, /* varargsStart= */ 1); + } else if (METHOD_WITH_THIRD_PARAMETER_OBJECT_VARARGS.matches(tree, state)) { + return matchObjectVarargs(arguments, /* varargsStart= */ 2); + } else if (METHOD_WITH_SOLE_PARAMETER_GENERIC_VARARGS.matches(tree, state)) { + return matchGenericVarargs(state, arguments); + } + + return NO_MATCH; + } + + private Description matchObjectVarargs( + List arguments, int varargsStart) { + if (arguments.size() != varargsStart + 1) { + return NO_MATCH; + } + var arg = arguments.getLast(); + if (arg.getKind() == NULL_LITERAL || isCastOfNullToArray(arg)) { + return describeMatch(arg, replace(arg, "(Object) null")); + } + return NO_MATCH; + } + + private Description matchGenericVarargs( + VisitorState state, List arguments) { + if (arguments.size() != 1) { + return NO_MATCH; + } + var arg = arguments.getLast(); + if (arg.getKind() == NULL_LITERAL || isCastOfNullToArrayOfTargetTypeElementType(arg, state)) { + var elementType = targetTypElementType(state); + var fix = SuggestedFix.builder(); + var prettyName = qualifyType(state, fix, elementType); + fix.replace(arg, "(%s) null".formatted(prettyName)); + return describeMatch(arg, fix.build()); + } + return NO_MATCH; + } + + private static boolean isCastOfNullToArray(ExpressionTree arg) { + if (!(arg instanceof TypeCastTree castTree)) { + return false; + } + if (castTree.getExpression().getKind() != NULL_LITERAL) { + return false; + } + var castType = getType(castTree.getType()); + return castType.getKind() == ARRAY; + } + + /** + * Returns whether {@code arg} is being cast to {@code Foo[]} and the target type of the method + * call at the path of {@code state} is {@code SomeGenericType}. + */ + private static boolean isCastOfNullToArrayOfTargetTypeElementType( + ExpressionTree arg, VisitorState state) { + if (!isCastOfNullToArray(arg)) { + return false; + } + + var castTree = (TypeCastTree) arg; + var castType = (ArrayType) getType(castTree.getType()); + var elementType = targetTypElementType(state); + return state.getTypes().isSubtype(castType.getComponentType(), elementType); + } + + private static Type targetTypElementType(VisitorState state) { + var targetType = targetType(state); + return (targetType != null && !targetType.type().getTypeArguments().isEmpty()) + // The target is likely List/Stream/..., so the signature has `T...`. Return `T`. + ? targetType.type().getTypeArguments().getFirst() + : state.getSymtab().objectType; + } + + /** + * A single parameter whose erasure is type {@code Object[]}. This includes {@code Object...} and + * (when {@code } is an unbounded type parameter) {@code T[]}. + */ + private static final ImmutableList> JUST_OBJECT_ARRAY = + ImmutableList.of(arrayOf(OBJECT_TYPE)); + + private static final Matcher METHOD_WITH_SOLE_PARAMETER_OBJECT_VARARGS = + instanceMethod() + .onDescendantOf("com.google.common.truth.Subject") + .named("containsExactly") + .withParametersOfType(JUST_OBJECT_ARRAY); + + private static final Matcher METHOD_WITH_SECOND_PARAMETER_OBJECT_VARARGS = + instanceMethod() + .onDescendantOf("com.google.common.reflect.Invokable") + .named("invoke") + .withParametersOfType(ImmutableList.of(OBJECT_TYPE, arrayOf(OBJECT_TYPE))); + + // TODO: b/429160687 - Also cover the methods in UsingCorrespondence. + + private static final Matcher METHOD_WITH_THIRD_PARAMETER_OBJECT_VARARGS = + instanceMethod() + .onDescendantOf("com.google.common.truth.Subject") + .namedAnyOf("containsAnyOf", "containsAtLeast", "containsNoneOf", "isAnyOf", "isNoneOf") + .withParametersOfType(ImmutableList.of(OBJECT_TYPE, OBJECT_TYPE, arrayOf(OBJECT_TYPE))); + + private static final Matcher METHOD_WITH_SOLE_PARAMETER_GENERIC_VARARGS = + anyOf( + staticMethod() + .onClass("com.google.common.collect.AndroidAccessToCompactDataStructures") + .namedAnyOf("newCompactHashSet", "newCompactLinkedHashSet") + .withParametersOfType(JUST_OBJECT_ARRAY), + staticMethod() + .onClassAny( + "com.google.common.collect.CompactHashSet", + "com.google.common.collect.CompactLinkedHashSet") + .named("create") + .withParametersOfType(JUST_OBJECT_ARRAY), + staticMethod() + .onClassAny( + "com.google.common.collect.Iterables", "com.google.common.collect.Iterators") + .namedAnyOf("cycle", "forArray") + .withParametersOfType(JUST_OBJECT_ARRAY), + staticMethod() + .onClass("com.google.common.collect.Lists") + .namedAnyOf("newArrayList", "newLinkedList") + .withParametersOfType(JUST_OBJECT_ARRAY), + staticMethod() + .onClass("com.google.common.collect.Sets") + .namedAnyOf("newHashSet", "newLinkedHashSet") + .withParametersOfType(JUST_OBJECT_ARRAY), + staticMethod().onClass("java.util.Arrays").named("asList"), + staticMethod() + .onClass("java.util.stream.Stream") + .named("of") + /* + * The check probably handles the `of(T)` overload fine even if we don't exclude it + * explicitly here, but why chance it? + * + * (Similarly, we may or may not need to call `withParametersOfType` for the other + * cases above in which overloads exist, but we do anyway.) + */ + .withParametersOfType(JUST_OBJECT_ARRAY)); +} diff --git a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java index 92613313534..009d35c8d8e 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -292,6 +292,7 @@ import com.google.errorprone.bugpatterns.NonFinalStaticField; import com.google.errorprone.bugpatterns.NonOverridingEquals; import com.google.errorprone.bugpatterns.NonRuntimeAnnotation; +import com.google.errorprone.bugpatterns.NullNeedsCastForVarargs; import com.google.errorprone.bugpatterns.NullOptional; import com.google.errorprone.bugpatterns.NullTernary; import com.google.errorprone.bugpatterns.NullableConstructor; @@ -813,6 +814,7 @@ public static ScannerSupplier warningChecks() { NonFinalCompileTimeConstant.class, NonRuntimeAnnotation.class, NullArgumentForNonNullParameter.class, + NullNeedsCastForVarargs.class, NullTernary.class, NullableOnContainingClass.class, OptionalEquality.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/NullNeedsCastForVarargsTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/NullNeedsCastForVarargsTest.java new file mode 100644 index 00000000000..18eb61aa18a --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/NullNeedsCastForVarargsTest.java @@ -0,0 +1,427 @@ +/* + * Copyright 2025 The Error Prone Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.google.errorprone.bugpatterns; + +import com.google.errorprone.BugCheckerRefactoringTestHelper; +import com.google.errorprone.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class NullNeedsCastForVarargsTest { + private final BugCheckerRefactoringTestHelper refactoringHelper = + BugCheckerRefactoringTestHelper.newInstance(NullNeedsCastForVarargs.class, getClass()); + + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(NullNeedsCastForVarargs.class, getClass()); + + @Test + public void containsExactly_bareNull() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import com.google.common.collect.ImmutableList; + + class Test { + void test() { + assertThat(ImmutableList.of("a")).containsExactly(null); + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import com.google.common.collect.ImmutableList; + + class Test { + void test() { + assertThat(ImmutableList.of("a")).containsExactly((Object) null); + } + } + """) + .doTest(); + } + + @Test + public void containsExactly_objectArrayCastNull() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import com.google.common.collect.ImmutableList; + + class Test { + void test() { + assertThat(ImmutableList.of("a")).containsExactly((Object[]) null); + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import com.google.common.collect.ImmutableList; + + class Test { + void test() { + assertThat(ImmutableList.of("a")).containsExactly((Object) null); + } + } + """) + .doTest(); + } + + @Test + public void containsExactly_stringArrayCastNull() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import com.google.common.collect.ImmutableList; + + class Test { + void test() { + assertThat(ImmutableList.of("a")).containsExactly((String[]) null); + } + } + """) + .addOutputLines( + "Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import com.google.common.collect.ImmutableList; + + class Test { + void test() { + assertThat(ImmutableList.of("a")).containsExactly((Object) null); + } + } + """) + .doTest(); + } + + @Test + public void streamSubject_containsExactly_bareNull() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import java.util.stream.Stream; + + class Test { + void test() { + // BUG: Diagnostic contains: + assertThat(Stream.of("a")).containsExactly(null); + } + } + """) + .doTest(); + } + + @Test + public void containsAnyOf_bareNull() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import com.google.common.collect.ImmutableList; + + class Test { + void test() { + // BUG: Diagnostic contains: + assertThat(ImmutableList.of("a")).containsAnyOf("b", "c", null); + } + } + """) + .doTest(); + } + + @Test + public void containsAnyOf_objectArrayCastNull() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import com.google.common.collect.ImmutableList; + + class Test { + void test() { + // BUG: Diagnostic contains: + assertThat(ImmutableList.of("a")).containsAnyOf("b", "c", (Object[]) null); + } + } + """) + .doTest(); + } + + @Test + public void arraysAsList_bareNull() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.util.Arrays; + import java.util.List; + + class Test { + void test() { + List list = Arrays.asList(null); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.util.Arrays; + import java.util.List; + + class Test { + void test() { + List list = Arrays.asList((String) null); + } + } + """) + .doTest(); + } + + @Test + public void arraysAsList_integerTargetType_withArrayCast() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.util.Arrays; + import java.util.List; + + class Test { + void test() { + List list = Arrays.asList((String[]) null); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.util.Arrays; + import java.util.List; + + class Test { + void test() { + List list = Arrays.asList((String) null); + } + } + """) + .doTest(); + } + + @Test + public void arraysAsList_arrayTargetType_withArrayCast() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.Arrays; + import java.util.List; + + class Test { + void test() { + // BUG: Diagnostic contains: + List list = Arrays.asList((Integer[][]) null); + } + } + """) + // The suggested replacement, `(Integer[]) null`, actually doesn't compile! + // The next test shows that it works with an explicit type argument. + // This is an edge case that I'm comfortable not handling. + .doTest(); + } + + @Test + public void arraysAsList_arrayTargetType_withArrayCastAndTypeArgument() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.util.Arrays; + import java.util.List; + + class Test { + void test() { + List list = Arrays.asList((Integer[][]) null); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.util.Arrays; + import java.util.List; + + class Test { + void test() { + List list = Arrays.asList((Integer[]) null); + } + } + """) + .doTest(); + } + + @Test + public void arraysAsList_arrayTargetType_withArrayCastAndTypeArgumentAndVar() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.Arrays; + import java.util.List; + + class Test { + void test() { + // BUG: Diagnostic contains: + var list = Arrays.asList((Integer[][]) null); + } + } + """) + .doTest(); + } + + @Test + public void arraysAsList_noTargetType() { + refactoringHelper + .addInputLines( + "Test.java", + """ + import java.util.Arrays; + + class Test { + void test() { + var list = Arrays.asList(null); + } + } + """) + .addOutputLines( + "Test.java", + """ + import java.util.Arrays; + + class Test { + void test() { + var list = Arrays.asList((Object) null); + } + } + """) + .doTest(); + } + + @Test + public void streamOf() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import java.util.stream.Stream; + + class Test { + void test() { + // BUG: Diagnostic contains: + var list = Stream.of(null); + } + } + """) + .doTest(); + } + + @Test + public void moreGuavaMethods() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import com.google.common.collect.Iterables; + import com.google.common.collect.Iterators; + import com.google.common.collect.Lists; + import com.google.common.collect.Sets; + import com.google.common.reflect.Invokable; + import java.util.Iterator; + import java.util.List; + import java.util.Set; + + class Test { + void test(Invokable invokable) throws Exception { + // BUG: Diagnostic contains: + List arrayList = Lists.newArrayList((String[]) null); + // BUG: Diagnostic contains: + Set hashSet = Sets.newHashSet((String[]) null); + // BUG: Diagnostic contains: + Iterable cycle = Iterables.cycle((String[]) null); + // BUG: Diagnostic contains: + Iterator forArray = Iterators.forArray(null); + // BUG: Diagnostic contains: + invokable.invoke("", null); + } + } + """) + .doTest(); + } + + @Test + public void negativeCases() { + compilationHelper + .addSourceLines( + "Test.java", + """ + import static com.google.common.truth.Truth.assertThat; + import com.google.common.collect.ImmutableList; + import com.google.common.collect.Lists; + import com.google.common.collect.Sets; + import java.util.Arrays; + import java.util.List; + import java.util.Set; + import java.util.stream.Stream; + + class Test { + void test() { + assertThat(ImmutableList.of("a")).containsExactly("a"); + assertThat(ImmutableList.of("a")).containsExactly((Object) null); + assertThat(ImmutableList.of("a")).containsExactly(null, null); + + assertThat(ImmutableList.of("a")).containsAnyOf("a", "b"); + assertThat(ImmutableList.of("a")).containsAnyOf("a", "b", (Object) null); + assertThat(ImmutableList.of("a")).containsAnyOf("a", "b", null, null); + + List listOfObjects = Arrays.asList((Object) null); + List listOfStrings = Arrays.asList((String) null); + List listOfIntegerArrays = Arrays.asList((Integer[]) null); + + List someNewArrayList = Lists.newArrayList(listOfObjects); + Set someNewHashSet = Sets.newHashSet(listOfObjects); + Stream streamOfArrays = Stream.of((Object[]) null); + } + } + """) + .doTest(); + } +} diff --git a/docs/bugpattern/nullness/NullNeedsCastForVarargs.md b/docs/bugpattern/nullness/NullNeedsCastForVarargs.md new file mode 100644 index 00000000000..4d6f94944fa --- /dev/null +++ b/docs/bugpattern/nullness/NullNeedsCastForVarargs.md @@ -0,0 +1,2 @@ +Note: The Truth methods covered by this check still accept a literal null in +Truth 1.4.5, but we expect to change that in a forthcoming version.