From 5aa714b237e415547ca1046466b22d983ae914e7 Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Wed, 8 Mar 2017 05:49:49 -0800 Subject: [PATCH] [Eradicate] Improve detection of circularities in field initialization Summary: Eradicate detects circular field initializations (e.g. a field initialized with itself) by checking in the typestate at the end of the constructor whether the origin of the field is a field name in the current class. This has the problem that the following initialization pattern is not recognized as correct: C(C x) { this.field = x.field } To fix the issue, the origin information for field accesses x.f is extended with the origin information of the inner object x. Circularities are detected if the origin of x is "this". Reviewed By: jberdine Differential Revision: D4672472 fbshipit-source-id: 9277bdd --- infer/src/eradicate/eradicateChecks.ml | 6 ++-- infer/src/eradicate/typeCheck.ml | 33 +++++++++++++------ infer/src/eradicate/typeOrigin.ml | 24 +++++++++----- infer/src/eradicate/typeOrigin.mli | 2 +- .../java/eradicate/FieldNotInitialized.java | 30 +++++++++++++++++ .../codetoanalyze/java/eradicate/issues.exp | 1 + 6 files changed, 72 insertions(+), 24 deletions(-) diff --git a/infer/src/eradicate/eradicateChecks.ml b/infer/src/eradicate/eradicateChecks.ml index 3fa7ad326..0da57ad9f 100644 --- a/infer/src/eradicate/eradicateChecks.ml +++ b/infer/src/eradicate/eradicateChecks.ml @@ -268,10 +268,8 @@ let check_constructor_initialization tenv let origin_is_initialized = function | TypeOrigin.Undef -> false - | TypeOrigin.Field (f, _) -> - (* field initialized with another field needing initialization *) - let circular = - List.exists ~f:(fun (f', _, _) -> Ident.equal_fieldname f f') fields in + | TypeOrigin.Field (TypeOrigin.Formal name, _, _) -> + let circular = String.equal (Mangled.to_string name) "this" in not circular | _ -> true in diff --git a/infer/src/eradicate/typeCheck.ml b/infer/src/eradicate/typeCheck.ml index c0de05779..a17595d23 100644 --- a/infer/src/eradicate/typeCheck.ml +++ b/infer/src/eradicate/typeCheck.ml @@ -187,13 +187,16 @@ let rec typecheck_expr let (_, ta, locs') = typecheck_expr find_canonical_duplicate visited checks tenv node instr_ref curr_pdesc typestate exp - (typ, TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.ONone, locs) loc - in + (typ, + TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.ONone, + locs) + loc in + let exp_origin = TypeAnnotation.get_origin ta in let tr_new = match EradicateChecks.get_field_annotation tenv fn typ with | Some (t, ia) -> ( t, - TypeAnnotation.from_item_annotation ia (TypeOrigin.Field (fn, loc)), + TypeAnnotation.from_item_annotation ia (TypeOrigin.Field (exp_origin, fn, loc)), locs' ) | None -> tr_default in @@ -283,7 +286,7 @@ let typecheck_instr let default = exp, typestate in (* If this is an assignment, update the typestate for a field access pvar. *) - let update_typestate_fld pvar fn typ = + let update_typestate_fld pvar origin fn typ = match TypeState.lookup_pvar pvar typestate with | Some _ when not is_assignment -> typestate | _ -> @@ -292,7 +295,8 @@ let typecheck_instr let range = ( t, - TypeAnnotation.from_item_annotation ia (TypeOrigin.Field (fn, loc)), + TypeAnnotation.from_item_annotation + ia (TypeOrigin.Field (origin, fn, loc)), [loc] ) in TypeState.add pvar range typestate @@ -340,8 +344,17 @@ let typecheck_instr | Exp.Lvar _ -> default - | Exp.Lfield (_exp, fn, typ) when ComplexExpressions.parameter_and_static_field () -> - let exp' = Idenv.expand_expr_temps idenv node _exp in + | Exp.Lfield (exp_, fn, typ) when ComplexExpressions.parameter_and_static_field () -> + let inner_origin = + (match exp_ with + | Exp.Lvar pvar -> TypeState.lookup_pvar pvar typestate + | Exp.Var id -> TypeState.lookup_id id typestate + | _ -> None) + |> + Option.value_map + ~f:(fun (_, ta, _) -> TypeAnnotation.get_origin ta) + ~default:TypeOrigin.ONone in + let exp' = Idenv.expand_expr_temps idenv node exp_ in let is_parameter_field pvar = (* parameter.field *) let name = Pvar.get_name pvar in @@ -359,13 +372,13 @@ let typecheck_instr | Exp.Lvar pv when is_parameter_field pv || is_static_field pv -> let fld_name = pvar_to_str pv ^ Ident.fieldname_to_string fn in let pvar = Pvar.mk (Mangled.from_string fld_name) curr_pname in - let typestate' = update_typestate_fld pvar fn typ in + let typestate' = update_typestate_fld pvar inner_origin fn typ in (Exp.Lvar pvar, typestate') | Exp.Lfield (_exp', fn', _) when Ident.java_fieldname_is_outer_instance fn' -> (* handle double dereference when accessing a field from an outer class *) let fld_name = Ident.fieldname_to_string fn' ^ "_" ^ Ident.fieldname_to_string fn in let pvar = Pvar.mk (Mangled.from_string fld_name) curr_pname in - let typestate' = update_typestate_fld pvar fn typ in + let typestate' = update_typestate_fld pvar inner_origin fn typ in (Exp.Lvar pvar, typestate') | Exp.Lvar _ | Exp.Lfield _ when ComplexExpressions.all_nested_fields () -> (* treat var.field1. ... .fieldn as a constant *) @@ -373,7 +386,7 @@ let typecheck_instr match ComplexExpressions.exp_to_string tenv node' exp with | Some exp_str -> let pvar = Pvar.mk (Mangled.from_string exp_str) curr_pname in - let typestate' = update_typestate_fld pvar fn typ in + let typestate' = update_typestate_fld pvar inner_origin fn typ in (Exp.Lvar pvar, typestate') | None -> default diff --git a/infer/src/eradicate/typeOrigin.ml b/infer/src/eradicate/typeOrigin.ml index f8e0bbb4b..15c84fa44 100644 --- a/infer/src/eradicate/typeOrigin.ml +++ b/infer/src/eradicate/typeOrigin.ml @@ -26,7 +26,7 @@ type proc_origin = type t = | Const of Location.t - | Field of Ident.fieldname * Location.t + | Field of t * Ident.fieldname * Location.t | Formal of Mangled.t | Proc of proc_origin | New @@ -36,17 +36,23 @@ type t = let equal = [%compare.equal : t] -let to_string = function - | Const _ -> "Const" - | Field (fn, _) -> "Field " ^ Ident.fieldname_to_simplified_string fn - | Formal s -> "Formal " ^ Mangled.to_string s +let rec to_string = function + | Const _ -> + "Const" + | Field (o, fn, _) -> + "Field " ^ Ident.fieldname_to_simplified_string fn ^ (" (inner: " ^ to_string o ^ ")") + | Formal s -> + "Formal " ^ Mangled.to_string s | Proc po -> Printf.sprintf "Fun %s" (Procname.to_simplified_string po.pname) - | New -> "New" - | ONone -> "ONone" - | Undef -> "Undef" + | New -> + "New" + | ONone -> + "ONone" + | Undef -> + "Undef" let get_description tenv origin = let atline loc = @@ -54,7 +60,7 @@ let get_description tenv origin = match origin with | Const loc -> Some ("null constant" ^ atline loc, Some loc, None) - | Field (fn, loc) -> + | Field (_, fn, loc) -> Some ("field " ^ Ident.fieldname_to_simplified_string fn ^ atline loc, Some loc, None) | Formal s -> Some ("method parameter " ^ Mangled.to_string s, None, None) diff --git a/infer/src/eradicate/typeOrigin.mli b/infer/src/eradicate/typeOrigin.mli index 48cddc1a8..a29cc3ea2 100644 --- a/infer/src/eradicate/typeOrigin.mli +++ b/infer/src/eradicate/typeOrigin.mli @@ -20,7 +20,7 @@ type proc_origin = type t = | Const of Location.t (** A constant in the source *) - | Field of Ident.fieldname * Location.t (** A field access *) + | Field of t * Ident.fieldname * Location.t (** A field access *) | Formal of Mangled.t (** A formal parameter *) | Proc of proc_origin (** A procedure call *) | New (** A new object creation *) diff --git a/infer/tests/codetoanalyze/java/eradicate/FieldNotInitialized.java b/infer/tests/codetoanalyze/java/eradicate/FieldNotInitialized.java index 232ca40c7..28610335b 100644 --- a/infer/tests/codetoanalyze/java/eradicate/FieldNotInitialized.java +++ b/infer/tests/codetoanalyze/java/eradicate/FieldNotInitialized.java @@ -138,4 +138,34 @@ public class FieldNotInitialized { } } + class InitCircular { + String s; + + InitCircular() { + String tmp = s; + s = tmp; // s is not initialized: circular initialization + } + } + + class InitWithOtherClass { + class OtherClass { + String s = ""; + } + + String s; + + InitWithOtherClass(OtherClass x) { + s = x.s; + } + } + + class InitWithThisClass { + + String s; + + InitWithThisClass(InitWithThisClass x) { + s = x.s; + } + } + } diff --git a/infer/tests/codetoanalyze/java/eradicate/issues.exp b/infer/tests/codetoanalyze/java/eradicate/issues.exp index 9fb31f6a7..2cf8e5877 100644 --- a/infer/tests/codetoanalyze/java/eradicate/issues.exp +++ b/infer/tests/codetoanalyze/java/eradicate/issues.exp @@ -1,5 +1,6 @@ codetoanalyze/java/eradicate/ActivityFieldNotInitialized.java, ActivityFieldNotInitialized$BadActivityWithOnCreate.(ActivityFieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, [Field `ActivityFieldNotInitialized$BadActivityWithOnCreate.field` is not initialized in the constructor and is not declared `@Nullable`] codetoanalyze/java/eradicate/FieldNotInitialized.java, FieldNotInitialized$ConditionalFieldInit.(FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, [Field `FieldNotInitialized$ConditionalFieldInit.o1` is not initialized in the constructor and is not declared `@Nullable`] +codetoanalyze/java/eradicate/FieldNotInitialized.java, FieldNotInitialized$InitCircular.(FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, [Field `FieldNotInitialized$InitCircular.s` is not initialized in the constructor and is not declared `@Nullable`] codetoanalyze/java/eradicate/FieldNotInitialized.java, FieldNotInitialized$OnlyRead.(FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, [Field `FieldNotInitialized$OnlyRead.o` is not initialized in the constructor and is not declared `@Nullable`] codetoanalyze/java/eradicate/FieldNotInitialized.java, FieldNotInitialized$OnlyReadIndirect.(FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, [Field `FieldNotInitialized$OnlyReadIndirect.o1` is not initialized in the constructor and is not declared `@Nullable`] codetoanalyze/java/eradicate/FieldNotInitialized.java, FieldNotInitialized$Swap.(FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, [Field `FieldNotInitialized$Swap.o1` is not initialized in the constructor and is not declared `@Nullable`]