From 556b9c121d2b5807fb243f4326611b2b9e24c18a Mon Sep 17 00:00:00 2001
From: Sam Blackshear <shb@fb.com>
Date: Fri, 12 Feb 2016 14:22:45 -0800
Subject: [PATCH] improving error message from fragment retains View checker

Reviewed By: mburman

Differential Revision: D2928721

fb-gh-sync-id: b5a10b4
shipit-source-id: b5a10b4
---
 infer/src/backend/localise.ml                 | 34 +++++++++++++++++++
 infer/src/backend/localise.mli                |  3 ++
 .../checkers/fragmentRetainsViewChecker.ml    | 17 +++-------
 3 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/infer/src/backend/localise.ml b/infer/src/backend/localise.ml
index 0f5c425d4..1bba02f90 100644
--- a/infer/src/backend/localise.ml
+++ b/infer/src/backend/localise.ml
@@ -235,6 +235,26 @@ let by_call_to tags proc_name =
 let by_call_to_ra tags ra =
   "by " ^ call_to_at_line tags ra.Sil.ra_pname ra.Sil.ra_loc
 
+let rec format_typ = function
+  | Sil.Tptr (typ, _) when !Config.curr_language = Config.Java ->
+      format_typ typ
+  | Sil.Tstruct { Sil.struct_name = Some name } ->
+      Mangled.to_string name
+  | Sil.Tvar tname ->
+      Typename.name tname
+  | typ ->
+      Sil.typ_to_string typ
+
+let format_field f =
+  if !Config.curr_language = Config.Java
+  then Ident.java_fieldname_get_field f
+  else Ident.fieldname_to_string f
+
+let format_method m =
+  if !Config.curr_language = Config.Java
+  then Procname.java_get_method m
+  else Procname.to_string m
+
 let mem_dyn_allocated = "memory dynamically allocated"
 let lock_acquired = "lock acquired"
 let released = "released"
@@ -396,6 +416,20 @@ let desc_context_leak pname context_typ fieldname leak_path : error_desc =
     "Context " ^ context_str ^ "may leak during method " ^ pname_str ^ ":\n" in
   { no_desc with descriptions = [preamble; leak_root; path_str] }
 
+let desc_fragment_retains_view fragment_typ fieldname fld_typ pname : error_desc =
+  (* TODO: try advice *)
+  let problem =
+    Printf.sprintf "Fragment %s does not nullify View field %s (type %s) in %s."
+      (format_typ fragment_typ)
+      (format_field fieldname)
+      (format_typ fld_typ)
+      (format_method pname) in
+  let consequences =
+    "If this Fragment is placed on the back stack, a reference to this (probably dead) View will be retained." in
+  let advice =
+    "In general, it is a good idea to initialize View's in onCreateView, then nullify them in onDestroyView." in
+  { no_desc with descriptions = [problem; consequences; advice] }
+
 let desc_custom_error loc : error_desc =
   { no_desc with descriptions = ["detected"; at_line (Tags.create ()) loc] }
 
diff --git a/infer/src/backend/localise.mli b/infer/src/backend/localise.mli
index 17adc3335..becc7c99e 100644
--- a/infer/src/backend/localise.mli
+++ b/infer/src/backend/localise.mli
@@ -207,6 +207,9 @@ val java_unchecked_exn_desc : Procname.t -> Typename.t -> string -> error_desc
 val desc_context_leak :
   Procname.t -> Sil.typ -> Ident.fieldname -> (Ident.fieldname option * Sil.typ) list -> error_desc
 
+val desc_fragment_retains_view :
+  Sil.typ -> Ident.fieldname -> Sil.typ -> Procname.t -> error_desc
+
 (* Create human-readable error description for assertion failures *)
 val desc_custom_error : Location.t -> error_desc
 
diff --git a/infer/src/checkers/fragmentRetainsViewChecker.ml b/infer/src/checkers/fragmentRetainsViewChecker.ml
index 8d43f0f44..9a55509b4 100644
--- a/infer/src/checkers/fragmentRetainsViewChecker.ml
+++ b/infer/src/checkers/fragmentRetainsViewChecker.ml
@@ -12,18 +12,11 @@
 module P = Printf
 open Utils
 
-let report_error fld fld_typ pname pdesc =
+let report_error fragment_typ fld fld_typ pname pdesc =
   let retained_view = "CHECKERS_FRAGMENT_RETAINS_VIEW" in
-  let fld_decl_class, fld_name =
-    Ident.java_fieldname_get_class fld, Ident.java_fieldname_get_field fld in
-  let description =
-    P.sprintf
-      "Fragment %s does not nullify View field %s (type %s) in onDestroyView. If the Fragment is placed on the back stack, a reference to the View may be retained."
-      fld_decl_class
-      fld_name
-      (Sil.typ_to_string (Sil.typ_strip_ptr fld_typ)) in
+  let description = Localise.desc_fragment_retains_view fragment_typ fld fld_typ pname in
+  let exn =  Exceptions.Checkers (retained_view, description) in
   let loc = Cfg.Procdesc.get_loc pdesc in
-  let exn = Exceptions.Checkers (retained_view, Localise.verbatim_desc description) in
   Reporting.log_error pname ~loc:(Some loc) exn
 
 let callback_fragment_retains_view { Callbacks.proc_desc; proc_name; tenv } =
@@ -54,9 +47,9 @@ let callback_fragment_retains_view { Callbacks.proc_desc; proc_name; tenv } =
           let fields_nullified = PatternMatch.get_fields_nullified proc_desc in
           (* report if a field is declared by C, but not nulled out in C.onDestroyView *)
           IList.iter
-            (fun (fname, typ, _) ->
+            (fun (fname, fld_typ, _) ->
                if not (Ident.FieldSet.mem fname fields_nullified) then
-                 report_error fname typ proc_name proc_desc)
+                 report_error typ fname fld_typ proc_name proc_desc)
             declared_view_fields
       | _ -> ()
     end