From 7635bb04148773ad4b9deaa8ce282cd984296416 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Mon, 17 Aug 2020 01:51:15 -0700 Subject: [PATCH] [nullsafe] @Initializer annotation is inherited from parent signatures Summary: We already take into account inheritance if the method inherits a known modelled initializer method (e.g. Activity.onCreate()). But if the method is explicitly marked as Initializer, we require its overrides to be also marked. This diffs fixes the behavior and makes it consistent, now this is enough to annotate only parent class. Reviewed By: jvillard Differential Revision: D23135177 fbshipit-source-id: a21ff4a0e --- infer/src/nullsafe/Initializers.ml | 35 ++++++----- .../java/nullsafe/FieldNotInitialized.java | 61 +++++++++++++++++++ .../codetoanalyze/java/nullsafe/issues.exp | 4 +- 3 files changed, 85 insertions(+), 15 deletions(-) diff --git a/infer/src/nullsafe/Initializers.ml b/infer/src/nullsafe/Initializers.ml index caf034b25..276413858 100644 --- a/infer/src/nullsafe/Initializers.ml +++ b/infer/src/nullsafe/Initializers.ml @@ -103,24 +103,31 @@ let get_class pn = match pn with Procname.Java pn_java -> Some (Procname.Java.get_class_name pn_java) | _ -> None +let is_annotated_initializer tenv proc_name = + PatternMatch.lookup_attributes tenv proc_name + |> Option.value_map + ~f:(fun ProcAttributes.{method_annotation= {return}} -> Annotations.ia_is_initializer return) + ~default:false + + +let is_annotated_initializer_in_chain tenv proc_name = + PatternMatch.override_exists (is_annotated_initializer tenv) tenv proc_name + + +(* Should the (non-constructor) function be considered an initializer method *) +let is_initializer tenv proc_attributes = + (* Either modelled as initializer or the descendent of such a method *) + PatternMatch.Java.method_is_initializer tenv proc_attributes + || (* Or explicitly marked @Initializer or the descendent of such a method *) + is_annotated_initializer_in_chain tenv proc_attributes.ProcAttributes.proc_name + + (** Typestates after the current procedure and all initializer procedures. *) let final_initializer_typestates_lazy tenv curr_pname curr_pdesc typecheck_proc = lazy - (let is_initializer proc_attributes = - PatternMatch.Java.method_is_initializer tenv proc_attributes - || - let ia = - (* TODO(T62825735): support trusted callees for fields *) - (Models.get_modelled_annotated_signature ~is_callee_in_trust_list:false tenv - proc_attributes) - .AnnotatedSignature.ret - .ret_annotation_deprecated - in - Annotations.ia_is_initializer ia - in - let initializers_current_class = + (let initializers_current_class = pname_and_pdescs_with tenv curr_pname (function pname, proc_attributes -> - is_initializer proc_attributes + is_initializer tenv proc_attributes && equal_class_opt (get_class pname) (get_class curr_pname) ) in final_typestates ((curr_pname, curr_pdesc) :: initializers_current_class) tenv typecheck_proc diff --git a/infer/tests/codetoanalyze/java/nullsafe/FieldNotInitialized.java b/infer/tests/codetoanalyze/java/nullsafe/FieldNotInitialized.java index 2bdf99911..07b73a0f0 100644 --- a/infer/tests/codetoanalyze/java/nullsafe/FieldNotInitialized.java +++ b/infer/tests/codetoanalyze/java/nullsafe/FieldNotInitialized.java @@ -328,4 +328,65 @@ class TestInitializerAnnotation { // return some meaninful object return ""; } + + abstract class TestFieldNotInitializedBase { + @Initializer + protected abstract void markedInitializerInBase(); + + protected abstract void notMarkedInitializerInBase1(); + + protected abstract void notMarkedInitializerInBase2(); + } + + // Test to ensure we respect parent @Initializer annotations + class TestFieldNotInitializedDerived extends TestFieldNotInitializedBase { + private String field1_OK; + private String field2_BAD; + private String field3_OK; + + @Override + public void markedInitializerInBase() { + // OK: implicitly @Initializer (inherited from the base) + field1_OK = ""; + } + + @Override + public void notMarkedInitializerInBase1() { + // BAD: field not initialized + field2_BAD = ""; + } + + @Override + @Initializer + public void notMarkedInitializerInBase2() { + // OK: explicitly marked as an initializer + field3_OK = ""; + } + } + + // Ensure that chains with non-trivial length work as well + class TestFieldNotInitializedDerivedDerived extends TestFieldNotInitializedDerived { + private String field1_OK; + private String field2_BAD; + private String field3_OK; + + @Override + public void markedInitializerInBase() { + // OK: implicitly @Initializer (inherited from the base through the chain) + field1_OK = ""; + } + + @Override + public void notMarkedInitializerInBase1() { + // BAD: field not initialized, and the method is not marked as @Initializer in any of the + // bases + field2_BAD = ""; + } + + @Override + public void notMarkedInitializerInBase2() { + // OK: explicitly marked as an initializer in the direct base + field3_OK = ""; + } + } } diff --git a/infer/tests/codetoanalyze/java/nullsafe/issues.exp b/infer/tests/codetoanalyze/java/nullsafe/issues.exp index ee0103e7c..e0dcc178f 100644 --- a/infer/tests/codetoanalyze/java/nullsafe/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe/issues.exp @@ -37,7 +37,7 @@ codetoanalyze/java/nullsafe/ConditionRedundant.java, codetoanalyze.java.nullsafe codetoanalyze/java/nullsafe/ConditionRedundant.java, codetoanalyze.java.nullsafe.ConditionRedundant.testFlowSensitivity(java.lang.String,java.lang.String):void, 4, ERADICATE_CONDITION_REDUNDANT, no_bucket, ADVICE, [The condition nullable1 might be always true according to the existing annotations.] codetoanalyze/java/nullsafe/FieldNotInitialized.java, Linters_dummy_method, 25, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], FieldNotInitialized, codetoanalyze.java.nullsafe, issues: 31, curr_mode: "Default" codetoanalyze/java/nullsafe/FieldNotInitialized.java, Linters_dummy_method, 233, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], TestKnownInitializers, codetoanalyze.java.nullsafe, issues: 2, curr_mode: "Default" -codetoanalyze/java/nullsafe/FieldNotInitialized.java, Linters_dummy_method, 280, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], TestInitializerAnnotation, codetoanalyze.java.nullsafe, issues: 3, curr_mode: "Default" +codetoanalyze/java/nullsafe/FieldNotInitialized.java, Linters_dummy_method, 280, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], TestInitializerAnnotation, codetoanalyze.java.nullsafe, issues: 5, curr_mode: "Default" codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$InitCircular.(codetoanalyze.java.nullsafe.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `stillBad` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method] codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$InitCircular.(codetoanalyze.java.nullsafe.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `bad` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method] codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$InitIfNull.(codetoanalyze.java.nullsafe.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `shouldBeGood_FIXME` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method] @@ -63,6 +63,8 @@ codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsaf codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$Suppressions.(codetoanalyze.java.nullsafe.FieldNotInitialized,int), 2, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`FieldNotInitialized$Suppressions.f(...)`: parameter #1(`a`) is declared non-nullable but the argument is `null`.] codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$Suppressions.(codetoanalyze.java.nullsafe.FieldNotInitialized,int,int,int), 1, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`FieldNotInitialized$Suppressions.f(...)`: parameter #1(`a`) is declared non-nullable but the argument is `null`.] codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$WriteItselfIsBAD.(codetoanalyze.java.nullsafe.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `bad` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method] +codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.TestInitializerAnnotation$TestFieldNotInitializedDerived.(codetoanalyze.java.nullsafe.TestInitializerAnnotation), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `field2_BAD` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method] +codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.TestInitializerAnnotation$TestFieldNotInitializedDerivedDerived.(codetoanalyze.java.nullsafe.TestInitializerAnnotation), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `field2_BAD` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method] codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.TestInitializerAnnotation.(), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `dontInitAtAllIsBAD` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method] codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.TestInitializerAnnotation.(), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `initInAnyOtherMethodIsBAD` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method] codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.TestInitializerAnnotation.build():java.lang.Object, 5, ERADICATE_CONDITION_REDUNDANT, no_bucket, ADVICE, [The condition TestInitializerAnnotation.initInInitilizerMethod1IsOK might be always true according to the existing annotations.]