[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
master
Mitya Lyubarskiy 5 years ago committed by Facebook GitHub Bot
parent fccf66968f
commit 0b6c5172a8

@ -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

@ -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();
}
}
}
}

@ -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"

Loading…
Cancel
Save