From 237aac4cd0adb1a148ad7ba3543cf82727a0a98a Mon Sep 17 00:00:00 2001 From: Artem Pianykh Date: Wed, 29 Jan 2020 01:25:10 -0800 Subject: [PATCH] [nullsafe] Introduce @Nullsafe annotation Summary: - Add `Nullsafe` annotation as a general mechanism to specify type-checking behaviour for nullsafe. - Document annotation params and provide usage examples with explanations. - Add tests to demonstrate the behaviour with different type-checking modes. No implementation is added. This diff serves as an RFC to hash out the details before I dive into code. Reviewed By: ngorogiannis Differential Revision: D19578329 fbshipit-source-id: b1a9f6162 --- .../facebook/infer/annotation/Nullsafe.java | 125 ++++++++++++++++++ .../java/nullsafe-default/NullsafeMode.java | 95 +++++++++++++ .../java/nullsafe-default/issues.exp | 5 + 3 files changed, 225 insertions(+) create mode 100644 infer/annotations/src/main/java/com/facebook/infer/annotation/Nullsafe.java create mode 100644 infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java diff --git a/infer/annotations/src/main/java/com/facebook/infer/annotation/Nullsafe.java b/infer/annotations/src/main/java/com/facebook/infer/annotation/Nullsafe.java new file mode 100644 index 000000000..8602a932c --- /dev/null +++ b/infer/annotations/src/main/java/com/facebook/infer/annotation/Nullsafe.java @@ -0,0 +1,125 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package com.facebook.infer.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +@Retention(RetentionPolicy.CLASS) +@Target({ElementType.TYPE}) +/** + * Configures nullability checking mode of annotated classes; a more general version of {@link + * NullsafeStrict}. + * + *

To specify the null-checking behaviour we need to define the following. + * + *

Code can be either *third-party* or *internal*. Whether the code is third-party is defined + * based on files in third-party signatures repo. This can be set up per-project (see nullsafe + * documentation for more details on this). + * + *

Code can be either *checked* or *unchecked*. + * + *

+ * + *

For internal code our default assumption is that lack of {@code @Nullable} annotation is + * equivalent to presence of {@code @NonNull} annotation. + * + *

When something is said to be "treated pessimistically" it means that return value is + * considered to be nullable and parameters to be non-nullable unless they have explicit {@code + * Nullable} annotations. + * + *

Now let's consider *usage examples* and describe nullability checking behaviour in each case: + * + * 

+ * import com.facebook.infer.annotation.Nullsafe;
+ *
+ * @Nullsafe(Nullsafe.Mode.LOCAL)
+ * public class LocallyCheckedAgainstDependencies { ... }
+ * // 1. This mode:
+ * //    a. Requires that all third-party calls are to checked code. Calls to unchecked third-party
+ * //       code are treated pessimistically.
+ * //    b. Calls to internal checked code treated as usual based on existing nullability annotations.
+ * //    c. Calls to internal unchecked code are allowed and default assumptions on the meaning of
+ * //       presence/absence of nullability annotations are in effect.
+ * // In other words, we trust all checked and unchecked internal code and
+ * // treat it as if all annotations were correct.
+ *
+ *
+ * @Nullsafe(value = Nullsafe.Mode.LOCAL,
+ *           trustOnly = @Nullsafe.TrustList({UncheckedComponent.class}))
+ * public class LocallyCheckedWithFewTrustedDependencies { ... }
+ * // 2. Same as above but calls to unchecked code are allowed only for classes specified
+ * //    in {@code trustOnly} list. Calls to other unchecked code are treated pessimistically
+ * //    by the checker.
+ *
+ *
+ * @Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = @Nullsafe.TrustList({}))
+ * public class LocallyCheckedRequiringOnlyCheckedDirectDependencies { ... }
+ * // 3. Same as above but with empty trust list, which means that all calls to unchecked code
+ * //    are treated pessimistically.
+ * // In practice, it usually means that all direct dependencies of the class
+ * // should be @Nullsafe themselves.
+ *
+ *
+ * @Nullsafe(Nullsafe.Mode.STRICT)
+ * public class StrictClass { ... }
+ * // 4. This is analogous to marking a class as @NullsafeStrict. In particular:
+ * //    a. Third-party calls should be to checked code. Calls to unchecked third-party
+ * //       code are treated pessimistically.
+ * //    b. Only calls to internal classes checked under strict mode are trusted.
+ * //    c. All other calls to checked and unchecked code are treated pessimistically.
+ * // In practice it means that transitive dependencies of the class should
+ * // themselves be @NullsafeStrict.
+ *
+ */ +public @interface Nullsafe { + enum Mode { + LOCAL, + STRICT + } + + @interface TrustList { + Class[] value(); + + /** + * Analogous to a wildcard "*" in a trust list when set to true. When set to false only classes + * listed in {@code value} parameter are trusted. + */ + boolean trustAll() default false; + } + + /** + * Specifies the null-checking mode. The parameter is named {@code value} instead of {@code mode} + * to enable a single-element annotation shorthand for @Nullsafe annotation, i.e. + * use @Nullsafe(Nullsafe.Mode.Strict) instead of @Nullsafe(mode = Nullsafe.Mode.STRICT). + */ + Mode value(); + + /** + * Provides fine-grained control over which unchecked internal classes to trust. Only affects + * LOCAL null-checking mode, as strict requires all dependencies to be STRICT themselves. + */ + TrustList trustOnly() default + @TrustList( + value = {}, + trustAll = true); +} diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java b/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java new file mode 100644 index 000000000..f18826632 --- /dev/null +++ b/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeMode.java @@ -0,0 +1,95 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +package codetoanalyze.java.nullsafe_default; + +import com.facebook.infer.annotation.Nullsafe; +import com.facebook.infer.annotation.NullsafeStrict; +import javax.annotation.Nullable; + +public class NullsafeMode { + abstract class VariousMethods { + public String returnVal() { + return "OK"; + } + + @Nullable + public String returnNull() { + return null; + } + } + + class NonNullsafe extends VariousMethods {} + + class AnotherNonNullsafe extends VariousMethods {} + + @Nullsafe(Nullsafe.Mode.LOCAL) + class TrustAllNullsafe extends VariousMethods { + String OK_returnFromAnyNonNullsafe() { + String a = new NonNullsafe().returnVal(); + String b = new AnotherNonNullsafe().returnVal(); + return a.concat(b); + } + + String BAD_returnNullFromNonNulsafe() { + return (new NonNullsafe()).returnNull(); + } + } + + @Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = @Nullsafe.TrustList({NonNullsafe.class})) + class TrustSomeNullsafe extends VariousMethods { + String OK_returnFromNonNullsafe() { + return new NonNullsafe().returnVal(); + } + + String BAD_returnFromAnotherNonNullsafe() { + return new AnotherNonNullsafe().returnVal(); + } + + @Nullable + String OK_returnFromAnotherNonNullsafeAsNullable() { + return new AnotherNonNullsafe().returnVal(); + } + + String BAD_returnNullFromNonNulsafe() { + return new NonNullsafe().returnNull(); + } + } + + @Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = @Nullsafe.TrustList({})) + class TrustNoneNullsafe extends VariousMethods { + String BAD_returnFromNonNullsafe() { + return new NonNullsafe().returnVal(); + } + + String OK_returnFromNullsafe() { + return new TrustSomeNullsafe().returnVal(); + } + } + + @Nullsafe(Nullsafe.Mode.STRICT) + class NullsafeWithStrictMode extends VariousMethods { + String BAD_returnFromNonStrict() { + return new TrustNoneNullsafe().returnVal(); + } + + String OK_returnFromNullsafeStrict() { + return new StrictNullsafe().returnVal(); + } + } + + @NullsafeStrict + class StrictNullsafe extends VariousMethods { + String BAD_returnFromNonNullsafe() { + return new NonNullsafe().returnVal(); + } + + String OK_returnFromNullsafeWithStrictMode() { + return new NullsafeWithStrictMode().returnVal(); + } + } +} diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index ec1f605ec..8f64940a5 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -173,6 +173,11 @@ codetoanalyze/java/nullsafe-default/NullMethodCall.java, codetoanalyze.java.null codetoanalyze/java/nullsafe-default/NullMethodCall.java, codetoanalyze.java.nullsafe_default.NullMethodCall.testSystemGetenvBad():int, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`envValue` is nullable and is not locally checked for null when calling `length()`: call to System.getenv(...) at line 240 (nullable according to nullsafe internal models)] codetoanalyze/java/nullsafe-default/NullMethodCall.java, codetoanalyze.java.nullsafe_default.NullMethodCall.withConditionalAssignemnt(codetoanalyze.java.nullsafe_default.NullMethodCall$AnotherI,boolean,java.lang.Object,java.lang.Object):void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`i` is nullable and is not locally checked for null when calling `withObjectParameter(...)`.] codetoanalyze/java/nullsafe-default/NullMethodCall.java, codetoanalyze.java.nullsafe_default.NullMethodCall.withConjuction(codetoanalyze.java.nullsafe_default.NullMethodCall$AnotherI,boolean,boolean):void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`i` is nullable and is not locally checked for null when calling `withBooleanParameter(...)`.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$StrictNullsafe.BAD_returnFromNonNullsafe():java.lang.String, 0, ERADICATE_UNCHECKED_NONSTRICT_FROM_STRICT, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 88. Either add a local check for null or assertion, or strictify NullsafeMode$VariousMethods.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$StrictNullsafe.OK_returnFromNullsafeWithStrictMode():java.lang.String, 0, ERADICATE_UNCHECKED_NONSTRICT_FROM_STRICT, no_bucket, ERROR, [`NullsafeMode$VariousMethods.returnVal()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 92. Either add a local check for null or assertion, or strictify NullsafeMode$VariousMethods.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustAllNullsafe.BAD_returnNullFromNonNulsafe():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [`BAD_returnNullFromNonNulsafe()`: return type is declared non-nullable but the method returns a nullable value: call to returnNull() at line 39.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.BAD_returnNullFromNonNulsafe():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [`BAD_returnNullFromNonNulsafe()`: return type is declared non-nullable but the method returns a nullable value: call to returnNull() at line 59.] +codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.OK_returnFromAnotherNonNullsafeAsNullable():java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `OK_returnFromAnotherNonNullsafeAsNullable()` is annotated with `@Nullable` but never returns null.] codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, codetoanalyze.java.nullsafe_default.ParameterNotNullable$ConstructorCall.(codetoanalyze.java.nullsafe_default.ParameterNotNullable,int), 0, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable$ConstructorCall(...)`: parameter #2(`s`) is declared non-nullable but the argument is `null`.] codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, codetoanalyze.java.nullsafe_default.ParameterNotNullable.callNull():void, 1, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable.test(...)`: parameter #1(`s`) is declared non-nullable but the argument `s` is `null`: null constant at line 39.] codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, codetoanalyze.java.nullsafe_default.ParameterNotNullable.callNullable(java.lang.String):void, 0, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ParameterNotNullable.test(...)`: parameter #1(`s`) is declared non-nullable but the argument `s` is nullable.]