diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/NullableOptional.java b/core/src/main/java/com/google/errorprone/bugpatterns/NullableOptional.java new file mode 100644 index 00000000000..99f949c6be7 --- /dev/null +++ b/core/src/main/java/com/google/errorprone/bugpatterns/NullableOptional.java @@ -0,0 +1,81 @@ +/* + * Copyright 2023 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.common.collect.ImmutableSet; +import com.google.errorprone.BugPattern; +import com.google.errorprone.BugPattern.SeverityLevel; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.VariableTreeMatcher; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.predicates.TypePredicate; +import com.google.errorprone.predicates.TypePredicates; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.MethodTree; +import com.sun.source.tree.ModifiersTree; +import com.sun.source.tree.VariableTree; +import com.sun.tools.javac.code.Type; + +/** + * A Checker that catches {@link java.util.Optional}/{@link com.google.common.base.Optional} with + * {@code Nullable} annotation. + */ +@BugPattern( + summary = + "Using an Optional variable which is expected to possibly be null is discouraged. It is" + + " best to indicate the absence of the value by assigning it an empty optional.", + severity = SeverityLevel.WARNING) +public final class NullableOptional extends BugChecker + implements MethodTreeMatcher, VariableTreeMatcher { + private static final TypePredicate IS_OPTIONAL_TYPE = + TypePredicates.isExactTypeAny( + ImmutableSet.of( + java.util.Optional.class.getCanonicalName(), + com.google.common.base.Optional.class.getCanonicalName())); + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + if (hasNullableAnnotation(tree.getModifiers()) + && isOptional(ASTHelpers.getType(tree.getReturnType()), state)) { + return describeMatch(tree); + } + return Description.NO_MATCH; + } + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + if (hasNullableAnnotation(tree.getModifiers()) && isOptional(ASTHelpers.getType(tree), state)) { + return describeMatch(tree); + } + return Description.NO_MATCH; + } + + /** Check if the input ModifiersTree has any kind of "Nullable" annotation. */ + private static boolean hasNullableAnnotation(ModifiersTree modifiersTree) { + return ASTHelpers.getAnnotationWithSimpleName(modifiersTree.getAnnotations(), "Nullable") + != null; + } + + /** + * Check if the input Type is either {@link java.util.Optional} or{@link + * com.google.common.base.Optional}. + */ + private static boolean isOptional(Type type, VisitorState state) { + return IS_OPTIONAL_TYPE.apply(type, state); + } +} 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 a4b4cba9b21..e05e7602f41 100644 --- a/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java +++ b/core/src/main/java/com/google/errorprone/scanner/BuiltInCheckerSuppliers.java @@ -271,6 +271,7 @@ import com.google.errorprone.bugpatterns.NullTernary; import com.google.errorprone.bugpatterns.NullableConstructor; import com.google.errorprone.bugpatterns.NullableOnContainingClass; +import com.google.errorprone.bugpatterns.NullableOptional; import com.google.errorprone.bugpatterns.NullablePrimitive; import com.google.errorprone.bugpatterns.NullablePrimitiveArray; import com.google.errorprone.bugpatterns.NullableVoid; @@ -979,6 +980,7 @@ public static ScannerSupplier warningChecks() { NotJavadoc.class, NullOptional.class, NullableConstructor.class, + NullableOptional.class, NullablePrimitive.class, NullablePrimitiveArray.class, NullableVoid.class, diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/NullableOptionalTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/NullableOptionalTest.java new file mode 100644 index 00000000000..8da09a598ab --- /dev/null +++ b/core/src/test/java/com/google/errorprone/bugpatterns/NullableOptionalTest.java @@ -0,0 +1,160 @@ +/* + * Copyright 2023 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.CompilationTestHelper; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +@RunWith(JUnit4.class) +public final class NullableOptionalTest { + + private final CompilationTestHelper compilationHelper = + CompilationTestHelper.newInstance(NullableOptional.class, getClass()); + + @Test + public void optionalFieldWithNullableAnnotation_showsError() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "import javax.annotation.Nullable;", + "final class Test {", + " // BUG: Diagnostic contains:", + " @Nullable private Optional foo;", + "}") + .doTest(); + } + + @Test + public void guavaOptionalFieldWithNullableAnnotation_showsError() { + compilationHelper + .addSourceLines( + "Test.java", + "import com.google.common.base.Optional;", + "import javax.annotation.Nullable;", + "final class Test {", + " @Nullable", + " // BUG: Diagnostic contains:", + " private Optional foo;", + "}") + .doTest(); + } + + @Test + public void methodReturnsOptionalWithNullableAnnotation_showsError() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "import javax.annotation.Nullable;", + "final class Test {", + " @Nullable", + " // BUG: Diagnostic contains:", + " private Optional foo() {", + " return Optional.empty();", + " }", + "}") + .doTest(); + } + + @Test + public void methodReturnsOptionalWithAnotherNullableAnnotation_showsError() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "import org.jspecify.nullness.Nullable;", + "final class Test {", + " @Nullable", + " // BUG: Diagnostic contains:", + " private Optional foo() {", + " return Optional.empty();", + " }", + "}") + .doTest(); + } + + @Test + public void methodHasNullableOptionalAsParameter_showsError() { + compilationHelper + .addSourceLines( + "Test.java", + "import java.util.Optional;", + "import javax.annotation.Nullable;", + "final class Test {", + " // BUG: Diagnostic contains:", + " private void foo(@Nullable Optional optional) {}", + "}") + .doTest(); + } + + @Test + public void objectFieldWithNullableAnnotation_noError() { + compilationHelper + .addSourceLines( + "Test.java", + "import javax.annotation.Nullable;", + "final class Test {", + " @Nullable Object field;", + "}") + .doTest(); + } + + @Test + public void methodReturnsNonOptionalWithNullableAnnotation_noError() { + compilationHelper + .addSourceLines( + "Test.java", + "import javax.annotation.Nullable;", + "final class Test {", + " @Nullable", + " private Object foo() {", + " return null;", + " }", + "}") + .doTest(); + } + + @Test + public void methodReturnsNonOptionalWithAnotherNullableAnnotation_noError() { + compilationHelper + .addSourceLines( + "Test.java", + "import org.jspecify.nullness.Nullable;", + "final class Test {", + " @Nullable", + " private Object foo() {", + " return null;", + " }", + "}") + .doTest(); + } + + @Test + public void methodHasNullableNonOptionalAsParameter_noError() { + compilationHelper + .addSourceLines( + "Test.java", + "import org.jspecify.nullness.Nullable;", + "final class Test {", + " private void foo(@Nullable Object object) {}", + "}") + .doTest(); + } +} diff --git a/docs/bugpattern/NullableOptional.md b/docs/bugpattern/NullableOptional.md new file mode 100644 index 00000000000..0a7c3b6a148 --- /dev/null +++ b/docs/bugpattern/NullableOptional.md @@ -0,0 +1,9 @@ +`Optional` is a container object which may or may not contain a value. The +presence or absence of the contained value should be demonstrated by the +`Optional` object itself. + +Using an Optional variable which is expected to possibly be null is discouraged. +An nullable Optional which uses `null` to indicate the absence of the value will +lead to extra work for `null` checking when using the object and even cause +exceptions such as `NullPointerException`. It is best to indicate the absence of +the value by assigning it an empty optional.