From 0b6c5172a8ac92a4f9781dfb9dfe06e36ee62026 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Fri, 24 Apr 2020 09:36:59 -0700 Subject: [PATCH] [nullsafe] Nested classes should inherit mode from the outer one Summary: From the user perspective, the current behavior is confusing. The users intutitively expect the inner class to inherit Nullsafe mode from the outer one. Having a class that is Nullsafe but the inner is not is hence dangerous and misleading. For the sake of completeness and to support gradual strictification, we allow the nested class to improse additional strictness. Particularly, the inner class can be Nullsafe but the outer can be not. A follow up to this diff will include warnings telling about redundant and wrong usages of nested annotations. Reviewed By: artempyanykh Differential Revision: D21228055 fbshipit-source-id: 75755ad1d --- infer/src/nullsafe/NullsafeMode.ml | 44 ++++++- .../NullsafeModeNestedClasses.java | 112 ++++++++++++++++++ .../java/nullsafe-default/issues.exp | 23 ++++ 3 files changed, 176 insertions(+), 3 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java diff --git a/infer/src/nullsafe/NullsafeMode.ml b/infer/src/nullsafe/NullsafeMode.ml index bbe946028..ade484d9e 100644 --- a/infer/src/nullsafe/NullsafeMode.ml +++ b/infer/src/nullsafe/NullsafeMode.ml @@ -68,6 +68,18 @@ module Trust = struct is_stricter_trust_list stricter_trust_list weaker_trust_list + let intersect trust1 trust2 = + match (trust1, trust2) with + | Only classes, All -> + Only classes + | All, Only classes -> + Only classes + | Only list1, Only list2 -> + Only (JavaClassName.Set.inter list1 list2) + | All, All -> + All + + let pp fmt t = match t with | All -> @@ -116,9 +128,8 @@ let extract_user_defined_class_name java_class_name = |> Option.value ~default:java_class_name -let of_class tenv class_name = - let user_defined_class = extract_user_defined_class_name class_name in - match PatternMatch.type_name_get_annotation tenv (Typ.JavaClass user_defined_class) with +let extract_mode_from_explicit_class_annotation tenv classname = + match PatternMatch.type_name_get_annotation tenv (Typ.JavaClass classname) with | Some annots -> ( if Annotations.ia_is_nullsafe_strict annots then Strict else @@ -132,6 +143,33 @@ let of_class tenv class_name = Default +(** Get the minimal mode that is stricter or equal than both of given modes *) +let intersect mode1 mode2 = + match (mode1, mode2) with + | Strict, _ | _, Strict -> + Strict + | Local trust1, Local trust2 -> + Local (Trust.intersect trust1 trust2) + | Local trust, Default | Default, Local trust -> + Local trust + | Default, Default -> + Default + + +let of_class tenv class_name = + (* The mode of the class is the strictest over this class's mode annotation and its outer classes *) + let rec of_class_and_outer_classes class_name = + let curr_class_mode = extract_mode_from_explicit_class_annotation tenv class_name in + match JavaClassName.get_outer_class_name class_name with + | Some outer_name -> + intersect curr_class_mode (of_class_and_outer_classes outer_name) + | None -> + curr_class_mode + in + let user_defined_class = extract_user_defined_class_name class_name in + of_class_and_outer_classes user_defined_class + + let of_procname tenv pname = let class_name = match pname with diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java b/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java new file mode 100644 index 000000000..1662c45ea --- /dev/null +++ b/infer/tests/codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java @@ -0,0 +1,112 @@ +/* + * 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; + +/** Test to ensure we correctly evaluate mode for nested classes */ +@Nullsafe(Nullsafe.Mode.LOCAL) +class NullsafeLocal { + + public String shouldBeNullsafeModeError() { + return null; + } + + // Mode should be inherited from the parent class + class Nested { + public String shouldBeNullsafeModeError() { + return null; + } + + // Mode propagation should be transitive + class DeeplyNested { + public String shouldBeNullsafeModeError() { + return null; + } + } + + // This is Local, but not Strict mode + public String returningDefaultNotNullIsOK() { + return Default.getString(); + } + } + + // It is OK to make nested classes more strict + @Nullsafe(Nullsafe.Mode.STRICT) + class NestedStrict { + public String returningDefaultNotNullIsError() { + return Default.getString(); + } + } + + // No need to repeat the mode - it is redundant + @Nullsafe(Nullsafe.Mode.LOCAL) + class NestedExplicitLocal { + public String shouldBeNullsafeModeError() { + return null; + } + } +} + +@Nullsafe(Nullsafe.Mode.STRICT) +class NullsafeStrict { + public String returningDefaultNotNullIsError() { + return Default.getString(); + } + + // STRICT mode is propagated to the nested class + class Nested { + public String returningDefaultNotNullIsError() { + return Default.getString(); + } + + // Impossible to downgrade the level of nested class, even if the nested mode + // is implicit + @Nullsafe(Nullsafe.Mode.LOCAL) + class DeeplyNestedLocalIsStillStrict { + public String returningDefaultNotNullIsError() { + return Default.getString(); + } + } + } + + // Impossible to downgrade the level of nested class + @Nullsafe(Nullsafe.Mode.LOCAL) + class NestedLocalIsStillStrict { + public String returningDefaultNotNullIsError() { + return Default.getString(); + } + } +} + +class Default { + public static String getString() { + return ""; + } + + // OK for nested to be @Nullsafe but the outer is not + @Nullsafe(Nullsafe.Mode.LOCAL) + class NestedLocal { + public String shouldBeNullsafeModeError() { + return null; + } + + // This is Local, but not Strict mode + public String returningDefaultNotNullIsOK() { + return Default.getString(); + } + + // And we can increase strictness even more + @Nullsafe(Nullsafe.Mode.STRICT) + class DeeplyNestedStrict { + public String returningDefaultNotNullIsError() { + return Default.getString(); + } + } + } +} diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index cf9a1cac3..4273e23e8 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -262,6 +262,29 @@ codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsa codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.BAD_returnNullFromNonNulsafe():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`BAD_returnNullFromNonNulsafe()`: return type is declared non-nullable but the method returns a nullable value: call to returnNull() at line 144.] codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.FP_OK_accessFieldFromNonNullsafe():java.lang.String, 1, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`NullsafeMode$NonNullsafe.valField`: `@NullsafeLocal(trust=selected)` mode prohibits using values coming from non-nullsafe classes without a check. This field is used at line 147. Either add a local check for null or assertion, or make NullsafeMode$NonNullsafe nullsafe.] codetoanalyze/java/nullsafe-default/NullsafeMode.java, codetoanalyze.java.nullsafe_default.NullsafeMode$TrustSomeNullsafe.OK_returnFromUntrustedNonNullsafeAsNullable():java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, ADVICE, [Method `OK_returnFromUntrustedNonNullsafeAsNullable()` is annotated with `@Nullable` but never returns null.] +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 14, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], NullsafeLocal, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "LocalTrustAll" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 21, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], NullsafeLocal$Nested, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "LocalTrustAll" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 27, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], NullsafeLocal$Nested$DeeplyNested, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "LocalTrustAll" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 41, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], NullsafeLocal$NestedStrict, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "Strict" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 49, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], NullsafeLocal$NestedExplicitLocal, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "LocalTrustAll" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 57, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], NullsafeStrict, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "Strict" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 63, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], NullsafeStrict$Nested, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "Strict" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 71, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], NullsafeStrict$Nested$DeeplyNestedLocalIsStillStrict, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "Strict" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 80, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], NullsafeStrict$NestedLocalIsStillStrict, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "Strict" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 87, ERADICATE_META_CLASS_CAN_BE_NULLSAFE, no_bucket, ADVICE, [Congrats! `Default` is free of nullability issues. Mark it `@Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = @Nullsafe.TrustList({}))` to prevent regressions.], Default, codetoanalyze.java.nullsafe_default, issues: 0, curr_mode: "Default", promote_mode: "Strict" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 94, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], Default$NestedLocal, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "LocalTrustAll" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, Linters_dummy_method, 106, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], Default$NestedLocal$DeeplyNestedStrict, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "Strict" +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, codetoanalyze.java.nullsafe_default.Default$NestedLocal$DeeplyNestedStrict.returningDefaultNotNullIsError():java.lang.String, 1, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`Default.getString()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 107. Either add a local check for null or assertion, or make Default nullsafe strict.] +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, codetoanalyze.java.nullsafe_default.Default$NestedLocal.shouldBeNullsafeModeError():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`shouldBeNullsafeModeError()`: return type is declared non-nullable but the method returns `null`: null constant at line 96.] +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, codetoanalyze.java.nullsafe_default.NullsafeLocal$Nested$DeeplyNested.shouldBeNullsafeModeError():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`shouldBeNullsafeModeError()`: return type is declared non-nullable but the method returns `null`: null constant at line 29.] +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, codetoanalyze.java.nullsafe_default.NullsafeLocal$Nested.shouldBeNullsafeModeError():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`shouldBeNullsafeModeError()`: return type is declared non-nullable but the method returns `null`: null constant at line 23.] +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, codetoanalyze.java.nullsafe_default.NullsafeLocal$NestedExplicitLocal.shouldBeNullsafeModeError():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`shouldBeNullsafeModeError()`: return type is declared non-nullable but the method returns `null`: null constant at line 51.] +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, codetoanalyze.java.nullsafe_default.NullsafeLocal$NestedStrict.returningDefaultNotNullIsError():java.lang.String, 1, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`Default.getString()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 42. Either add a local check for null or assertion, or make Default nullsafe strict.] +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, codetoanalyze.java.nullsafe_default.NullsafeLocal.shouldBeNullsafeModeError():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, ERROR, [`shouldBeNullsafeModeError()`: return type is declared non-nullable but the method returns `null`: null constant at line 17.] +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, codetoanalyze.java.nullsafe_default.NullsafeStrict$Nested$DeeplyNestedLocalIsStillStrict.returningDefaultNotNullIsError():java.lang.String, 1, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`Default.getString()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 72. Either add a local check for null or assertion, or make Default nullsafe strict.] +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, codetoanalyze.java.nullsafe_default.NullsafeStrict$Nested.returningDefaultNotNullIsError():java.lang.String, 1, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`Default.getString()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 64. Either add a local check for null or assertion, or make Default nullsafe strict.] +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, codetoanalyze.java.nullsafe_default.NullsafeStrict$NestedLocalIsStillStrict.returningDefaultNotNullIsError():java.lang.String, 1, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`Default.getString()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 81. Either add a local check for null or assertion, or make Default nullsafe strict.] +codetoanalyze/java/nullsafe-default/NullsafeModeNestedClasses.java, codetoanalyze.java.nullsafe_default.NullsafeStrict.returningDefaultNotNullIsError():java.lang.String, 1, ERADICATE_UNCHECKED_USAGE_IN_NULLSAFE, no_bucket, ERROR, [`Default.getString()`: `@NullsafeStrict` mode prohibits using values coming from non-strict classes without a check. Result of this call is used at line 58. Either add a local check for null or assertion, or make Default nullsafe strict.] codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, Linters_dummy_method, 20, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], ParameterNotNullable, codetoanalyze.java.nullsafe_default, issues: 37, curr_mode: "Default" codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, Linters_dummy_method, 64, ERADICATE_META_CLASS_CAN_BE_NULLSAFE, no_bucket, ADVICE, [Congrats! `ParameterNotNullable$Builder` is free of nullability issues. Mark it `@Nullsafe(value = Nullsafe.Mode.LOCAL, trustOnly = @Nullsafe.TrustList({}))` to prevent regressions.], ParameterNotNullable$Builder, codetoanalyze.java.nullsafe_default, issues: 0, curr_mode: "Default", promote_mode: "Strict" codetoanalyze/java/nullsafe-default/ParameterNotNullable.java, Linters_dummy_method, 93, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], ParameterNotNullable$ConstructorCall, codetoanalyze.java.nullsafe_default, issues: 1, curr_mode: "Default"