Migrate BAD_POINTER_COMPARISON check from backend to linters infra

Reviewed By: dulmarod

Differential Revision: D3614726

fbshipit-source-id: bdc7651

infer/src/backend/errdesc.ml

@@ -440,11 +440,6 @@ let explain_allocation_mismatch ra_alloc ra_dealloc =
   Localise.desc_allocation_mismatch
     (get_primitive_called true ra_alloc) (get_primitive_called false ra_dealloc)
 
-(** Produce a description of a pointer dangerously coerced to a boolean in a comparison *)
-let explain_bad_pointer_comparison exp node loc =
-  let dexp_opt = exp_rv_dexp node exp in
-  Localise.desc_bad_pointer_comparison dexp_opt loc
-
 (** check whether the type of leaked [hpred] appears as a predicate
     in an inductive predicate in [prop] *)
 let leak_from_list_abstraction hpred prop =
@@ -517,7 +512,7 @@ let explain_leak tenv hpred prop alloc_att_opt bucket =
     | _ -> (None, None, None) in
   let is_file = match resource_opt with
     | Some Sil.Rfile -> true
-    | _ -> false in 
+    | _ -> false in
   let check_pvar pvar =
     (* check that pvar is local or global and has the same type as the leaked hpred *)
     (Pvar.is_local pvar || Pvar.is_global pvar) &&

infer/src/backend/errdesc.mli

@@ -44,9 +44,6 @@ val exp_rv_dexp : Cfg.Node.t -> Sil.exp -> DecompiledExp.t option
 val explain_context_leak : Procname.t -> Typ.t -> Ident.fieldname ->
   (Ident.fieldname option * Typ.t) list -> Localise.error_desc
 
-(** Produce a description of a pointer dangerously coerced to a boolean in a comparison *)
-val explain_bad_pointer_comparison : Sil.exp -> Cfg.Node.t -> Location.t -> Localise.error_desc
-
 (** Produce a description of a mismatch between an allocation function and a deallocation function *)
 val explain_allocation_mismatch : Sil.res_action -> Sil.res_action -> Localise.error_desc
 

infer/src/backend/exceptions.ml

@@ -37,7 +37,6 @@ exception Array_out_of_bounds_l2 of Localise.error_desc * L.ml_loc
 exception Array_out_of_bounds_l3 of Localise.error_desc * L.ml_loc
 exception Array_of_pointsto of L.ml_loc
 exception Bad_footprint of L.ml_loc
-exception Bad_pointer_comparison of Localise.error_desc * L.ml_loc
 exception Class_cast_exception of Localise.error_desc * L.ml_loc
 exception Codequery of Localise.error_desc
 exception Comparing_floats_for_equality of Localise.error_desc * L.ml_loc
@@ -121,9 +120,6 @@ let recognize_exception exn =
         let ml_loc = (f, l, c, c) in
         (Localise.from_string "Assert_failure",
          Localise.no_desc, Some ml_loc, Exn_developer, High, None, Nocat)
-    | Bad_pointer_comparison (desc, ml_loc) ->
-        (Localise.bad_pointer_comparison,
-         desc, Some ml_loc, Exn_user, High, Some Kerror, Prover)
     | Bad_footprint ml_loc ->
         (Localise.from_string "Bad_footprint",
          Localise.no_desc, Some ml_loc, Exn_developer, Low, None, Nocat)
@@ -351,4 +347,3 @@ let pp_err (_, node_key) loc ekind ex_name desc ml_loc_opt fmt () =
 let handle_exception exn =
   let _, _, _, visibility, _, _, _ = recognize_exception exn in
   visibility == Exn_user || visibility == Exn_developer
-

infer/src/backend/exceptions.mli

@@ -36,7 +36,6 @@ exception Array_out_of_bounds_l1 of Localise.error_desc * Logging.ml_loc
 exception Array_out_of_bounds_l2 of Localise.error_desc * Logging.ml_loc
 exception Array_out_of_bounds_l3 of Localise.error_desc * Logging.ml_loc
 exception Bad_footprint of Logging.ml_loc
-exception Bad_pointer_comparison of Localise.error_desc * Logging.ml_loc
 exception Class_cast_exception of Localise.error_desc * Logging.ml_loc
 exception Codequery of Localise.error_desc
 exception Comparing_floats_for_equality of Localise.error_desc * Logging.ml_loc

infer/src/backend/localise.ml

@@ -475,18 +475,6 @@ let desc_fragment_retains_view fragment_typ fieldname fld_typ pname : error_desc
 let desc_custom_error loc : error_desc =
   { no_desc with descriptions = ["detected"; at_line (Tags.create ()) loc] }
 
-let desc_bad_pointer_comparison dexp_opt loc : error_desc =
-  let dexp_str = match dexp_opt with
-    | Some dexp -> (DecompiledExp.to_string dexp) ^ " "
-    | None -> "" in
-  let line_info = at_line (Tags.create ()) loc in
-  let check_msg =
-    "Implicitly checking whether NSNumber pointer " ^ dexp_str ^ "is nil " ^ line_info ^ "." in
-  let concern_msg = "Did you mean to compare against the unboxed value instead?" in
-  let fix_msg_rec1 = "Please either explicitly compare " ^ dexp_str ^ "to nil" in
-  let fix_msg_rec2 = "or use one of the NSNumber accessors before the comparison." in
-  { no_desc with descriptions = [check_msg; concern_msg; fix_msg_rec1; fix_msg_rec2] }
-
 (** type of access *)
 type access =
   | Last_assigned of int * bool (* line, null_case_flag *)

infer/src/backend/localise.mli

@@ -230,7 +230,6 @@ val desc_fragment_retains_view :
 (* Create human-readable error description for assertion failures *)
 val desc_custom_error : Location.t -> error_desc
 
-val desc_bad_pointer_comparison : DecompiledExp.t option -> Location.t -> error_desc
 (** kind of precondition not met *)
 type pnm_kind =
   | Pnm_bounds

infer/src/backend/symExec.ml

@@ -1058,11 +1058,6 @@ let rec sym_exec tenv current_pdesc _instr (prop_: Prop.normal Prop.t) path
                 true (* skip subpart of a condition obtained from compilation of && and || *)
             | _ -> false in
           true_branch && not skip_loop in
-        (* in comparisons, nil is translated as (void * ) 0 rather than 0 *)
-        let is_comparison_to_nil = function
-          | Sil.Cast ((Typ.Tptr (Typ.Tvoid, _)), exp) ->
-              !Config.curr_language = Config.Clang && Sil.exp_is_zero exp
-          | _ -> false in
         match Prop.exp_normalize_prop Prop.prop_emp cond with
         | Sil.Const (Const.Cint i) when report_condition_always_true_false i ->
             let node = State.get_node () in
@@ -1071,26 +1066,6 @@ let rec sym_exec tenv current_pdesc _instr (prop_: Prop.normal Prop.t) path
               Exceptions.Condition_always_true_false (desc, not (IntLit.iszero i), __POS__) in
             let pre_opt = State.get_normalized_pre (Abs.abstract_no_symop current_pname) in
             Reporting.log_warning current_pname ~pre: pre_opt exn
-        | Sil.BinOp ((Binop.Eq | Binop.Ne), lhs, rhs)
-          when true_branch && !Config.footprint && not (is_comparison_to_nil rhs) ->
-            (* iOS: check that NSNumber *'s are not used in conditionals without comparing to nil *)
-            let lhs_normal = Prop.exp_normalize_prop prop__ lhs in
-            let is_nsnumber = function
-              | Typ.Tvar (Typename.TN_csu (Csu.Class _, name)) ->
-                  Mangled.to_string name = "NSNumber"
-              | _ -> false in
-            let lhs_is_ns_ptr () =
-              IList.exists
-                (function
-                  | Sil.Hpointsto (_, Sil.Eexp (exp, _), Sil.Sizeof (Typ.Tptr (typ, _), _, _)) ->
-                      Sil.exp_equal exp lhs_normal && is_nsnumber typ
-                  | _ -> false)
-                (Prop.get_sigma prop__) in
-            if not (Sil.exp_is_zero lhs_normal) && lhs_is_ns_ptr () then
-              let node = State.get_node () in
-              let desc = Errdesc.explain_bad_pointer_comparison lhs node loc in
-              let exn = Exceptions.Bad_pointer_comparison (desc, __POS__) in
-              Reporting.log_warning current_pname exn
         | _ -> () in
       if not Config.report_runtime_exceptions then
         check_already_dereferenced current_pname cond prop__;

infer/src/clang/cFrontend_checkers.ml

@@ -270,6 +270,44 @@ let captured_cxx_ref_in_objc_block_warning stmt_info captured_vars =
   else None
 
 
+(* BAD_POINTER_COMPARISON: Fires whenever a NSNumber is dangerously coerced to
+    a boolean in a comparison *)
+let bad_pointer_comparison_warning stmt_info stmts =
+  let rec condition stmts =
+    let condition_aux stmt =
+      match stmt with
+      | Clang_ast_t.CallExpr _
+      | Clang_ast_t.CXXMemberCallExpr _
+      | Clang_ast_t.CXXOperatorCallExpr _
+      | Clang_ast_t.ObjCMessageExpr _ -> false
+      | Clang_ast_t.BinaryOperator (_, _, _, boi)
+        when (boi.boi_kind = `NE) || (boi.boi_kind = `EQ) -> false
+      | Clang_ast_t.UnaryOperator (_, stmts, _, uoi) when uoi.uoi_kind = `LNot ->
+          condition stmts
+      | stmt ->
+          match Clang_ast_proj.get_expr_tuple stmt with
+          | Some (_, stmts, expr_info) ->
+              let typ = CFrontend_utils.Ast_utils.get_desugared_type expr_info.ei_type_ptr in
+              if CFrontend_utils.Ast_utils.is_ptr_to_objc_class typ "NSNumber" then
+                true
+              else
+                condition stmts
+          | _ -> false in
+    IList.exists condition_aux stmts in
+  if condition stmts then
+    Some { CIssue.
+           issue = CIssue.Bad_pointer_comparison;
+           description = "Implicitly checking whether NSNumber pointer is nil";
+           suggestion =
+             Some ("Did you mean to compare against the unboxed value instead? " ^
+                   "Please either explicitly compare the NSNumber instance to nil, " ^
+                   "or use one of the NSNumber accessors before the comparison.");
+           loc = location_from_sinfo stmt_info
+         }
+  else
+    None
+
+
 (* exist m1:  m1.body|- EF call_method(addObserver:) => exists m2 : m2.body |- EF call_method(removeObserver:) *)
 let checker_NSNotificationCenter decl_info decls =
   let exists_method_calling_addObserver =

infer/src/clang/cFrontend_checkers.mli

@@ -29,6 +29,9 @@ val direct_atomic_property_access_warning : Clang_ast_t.decl -> Clang_ast_t.stmt
 val captured_cxx_ref_in_objc_block_warning : Clang_ast_t.stmt_info ->
   Clang_ast_t.block_captured_variable list -> CIssue.issue_desc option
 
+val bad_pointer_comparison_warning :
+  Clang_ast_t.stmt_info -> Clang_ast_t.stmt list -> CIssue.issue_desc option
+
 (* REGISTERED_OBSERVER_BEING_DEALLOCATED: an object is registered in a notification center
    but not removed before deallocation *)
 val checker_NSNotificationCenter : Clang_ast_t.decl_info -> Clang_ast_t.decl list ->

infer/src/clang/cFrontend_checkers_main.ml

@@ -9,7 +9,7 @@
 
 let rec do_frontend_checks_stmt cfg cg method_decl stmt =
   CFrontend_errors.run_frontend_checkers_on_stmt cfg cg method_decl stmt;
-  let _, stmts = Clang_ast_proj.get_stmt_tuple stmt in
+  let stmts = CFrontend_utils.Ast_utils.get_stmts_from_stmt stmt in
   IList.iter (do_frontend_checks_stmt cfg cg method_decl) stmts
 
 let rec do_frontend_checks_decl cfg cg decl =

infer/src/clang/cFrontend_errors.ml

@@ -47,6 +47,36 @@ let global_var_checker_list = [CFrontend_checkers.global_var_init_with_calls_war
 let checker_for_global_var dec checker =
   checker dec
 
+(* List of checkers on conditional operator *)
+let conditional_op_checker_list = [CFrontend_checkers.bad_pointer_comparison_warning]
+
+(* Invocation of checker belonging to conditional_op_checker_list *)
+let checker_for_conditional_op stmt_info first_stmt checker =
+  checker stmt_info first_stmt
+
+(* List of checkers on if-statement *)
+let if_stmt_checker_list = [CFrontend_checkers.bad_pointer_comparison_warning]
+
+(* Invocation of checker belonging to if_stmt_checker_list *)
+let checker_for_if_stmt stmt_info cond checker =
+  checker stmt_info cond
+
+(* List of checkers on for statement *)
+let for_stmt_checker_list = [CFrontend_checkers.bad_pointer_comparison_warning]
+
+(* Invocation of checker belonging to for_stmt_checker_list *)
+let checker_for_for_stmt stmt_info cond checker =
+  checker stmt_info cond
+
+(* List of checkers on while statement *)
+let while_stmt_checker_list = [CFrontend_checkers.bad_pointer_comparison_warning]
+
+(* Invocation of checker belonging to while_stmt_checker_list *)
+let checker_for_while_stmt stmt_info cond checker =
+  checker stmt_info cond
+
+
+
 let errLogMap = ref Procname.Map.empty
 
 let get_err_log cfg cg method_decl_opt loc =
@@ -103,6 +133,18 @@ let run_frontend_checkers_on_stmt cfg cg method_decl instr =
       let call_captured_vars_checker = checkers_for_capture_vars stmt_info captured_block_vars in
       let decl_opt = Some method_decl in
       invoke_set_of_checkers call_captured_vars_checker cfg cg decl_opt captured_vars_checker_list
+  | IfStmt (stmt_info, _ :: cond :: _) ->
+      let call_checker = checker_for_if_stmt stmt_info [cond] in
+      invoke_set_of_checkers call_checker cfg cg None if_stmt_checker_list
+  | ConditionalOperator (stmt_info, first_stmt :: _, _) ->
+      let call_checker = checker_for_conditional_op stmt_info [first_stmt] in
+      invoke_set_of_checkers call_checker cfg cg None conditional_op_checker_list
+  | ForStmt (stmt_info, [_; _; cond; _; _]) ->
+      let call_checker = checker_for_for_stmt stmt_info [cond] in
+      invoke_set_of_checkers call_checker cfg cg None for_stmt_checker_list
+  | WhileStmt (stmt_info, [_; cond; _]) ->
+      let call_checker = checker_for_while_stmt stmt_info [cond] in
+      invoke_set_of_checkers call_checker cfg cg None while_stmt_checker_list
   | _ -> ()
 
 let run_frontend_checkers_on_decl cfg cg dec =

infer/src/clang/cFrontend_utils.ml

@@ -424,6 +424,18 @@ struct
     | Config.OBJCPP -> true
     | _ -> false
 
+  let is_ptr_to_objc_class typ class_name =
+    match typ with
+    | Some Clang_ast_t.ObjCObjectPointerType (_, typ_ptr) ->
+        (match get_desugared_type typ_ptr with
+         | Some ObjCInterfaceType (_, ptr) ->
+             (match get_decl ptr with
+              | Some ObjCInterfaceDecl (_, ndi, _, _, _) ->
+                  String.compare ndi.ni_name class_name = 0
+              | _ -> false)
+         | _ -> false)
+    | _ -> false
+
 (*
   let rec getter_attribute_opt attributes =
     match attributes with

infer/src/clang/cFrontend_utils.mli

@@ -149,6 +149,8 @@ sig
   (* true if CFrontend_config.language is set ot ObjC *)
   val is_objcpp : unit -> bool
 
+  val is_ptr_to_objc_class : Clang_ast_t.c_type option -> string -> bool
+
 end
 
 module General_utils :

infer/src/clang/cIssue.ml

@@ -9,6 +9,7 @@
 
 type issue =
   | Assign_pointer_warning
+  | Bad_pointer_comparison
   | Cxx_reference_captured_in_objc_block
   | Direct_atomic_property_access
   | Global_variable_initialized_with_function_or_method_call
@@ -18,6 +19,7 @@ type issue =
 let to_string issue =
   match issue with
   | Assign_pointer_warning -> "ASSIGN_POINTER_WARNING"
+  | Bad_pointer_comparison -> "BAD_POINTER_COMPARISON"
   | Cxx_reference_captured_in_objc_block ->
       "CXX_REFERENCE_CAPTURED_IN_OBJC_BLOCK"
   | Direct_atomic_property_access -> "DIRECT_ATOMIC_PROPERTY_ACCESS"
@@ -30,6 +32,7 @@ let to_string issue =
 let severity_of_issue issue =
   match issue with
   | Assign_pointer_warning
+  | Bad_pointer_comparison
   | Cxx_reference_captured_in_objc_block
   | Direct_atomic_property_access
   | Global_variable_initialized_with_function_or_method_call

infer/src/clang/cIssue.mli

@@ -9,6 +9,7 @@
 
 type issue =
   | Assign_pointer_warning
+  | Bad_pointer_comparison
   | Cxx_reference_captured_in_objc_block
   | Direct_atomic_property_access
   | Global_variable_initialized_with_function_or_method_call

infer/tests/codetoanalyze/objc/linters/nsnumber.m

@@ -19,22 +19,52 @@ void bad2(NSNumber* isNum) {
   }
 }
 
-void ok1(NSNumber* isNum) {
+void bad3(NSNumber* number1, NSNumber* number2) {
+  if (number1 == nil || number2) {
+  }
+}
+
+@interface TestClass : NSObject
+
+@property(atomic) int value;
+
+- (BOOL)doStuff:(NSNumber*)number;
+
+@end
+
+@implementation TestClass
+
+- (BOOL)doStuff:(NSNumber*)number {
+  return YES;
+}
+
+@end
+
+void bad4(NSNumber* number, TestClass* t) { t.value = number ? 1 : 0; }
+
+void ok1(NSNumber* number, TestClass* t) { t.value = number == nil ? 1 : 0; }
+
+void ok2(NSNumber* number, TestClass* t) {
+  if ([t doStuff:number]) {
+  }
+}
+
+void ok3(NSNumber* isNum) {
   if (isNum != nil) {
   }
 }
 
-void ok2(NSNumber* isNum) {
+void ok4(NSNumber* isNum) {
   if (nil != isNum) {
   }
 }
 
-void ok3(NSNumber* isNum) {
+void ok5(NSNumber* isNum) {
   if (nil == isNum) {
   }
 }
 
-void ok4(NSNumber* isNum) {
+void ok6(NSNumber* isNum) {
   if (isNum == nil) {
   }
 }

infer/tests/endtoend/objc/linters/NSNumber2Test.java

@@ -7,10 +7,10 @@
  * of patent rights can be found in the PATENTS file in the same directory.
  */
 
-package endtoend.objc.infer;
+package endtoend.objc.linters;
 
 import static org.hamcrest.MatcherAssert.assertThat;
-import static utils.matchers.ResultContainsExactly.containsExactly;
+import static utils.matchers.ResultContainsLineNumbers.containsLines;
 
 import com.google.common.collect.ImmutableList;
 
@@ -28,7 +28,7 @@ import utils.InferRunner;
 public class NSNumber2Test {
 
   public static final String NSNUMBER_FILE =
-      "infer/tests/codetoanalyze/objc/errors/bad_ptr_comparisons/badpointer.m";
+      "infer/tests/codetoanalyze/objc/linters/badpointer.m";
 
   private static ImmutableList<String> inferCmd;
 
@@ -48,20 +48,10 @@ public class NSNumber2Test {
   public void badNSNumberPointerComparisonShouldBeFound()
       throws InterruptedException, IOException, InferException {
     InferResults inferResults = InferRunner.runInferObjC(inferCmd);
-    String[] methods = {
-      "bad1",
-      "bad2",
-      "bad3"
-    };
     assertThat(
         "Results should contain " + BAD_POINTER_COMPARISON,
         inferResults,
-        containsExactly(
-            BAD_POINTER_COMPARISON,
-            NSNUMBER_FILE,
-            methods
-        )
-    );
+        containsLines(new int[] {17, 26, 33}));
   }
 
 }

infer/tests/endtoend/objc/linters/NSNumberTest.java

@@ -7,10 +7,10 @@
  * of patent rights can be found in the PATENTS file in the same directory.
  */
 
-package endtoend.objc.infer;
+package endtoend.objc.linters;
 
 import static org.hamcrest.MatcherAssert.assertThat;
-import static utils.matchers.ResultContainsExactly.containsExactly;
+import static utils.matchers.ResultContainsLineNumbers.containsLines;
 
 import com.google.common.collect.ImmutableList;
 
@@ -28,7 +28,7 @@ import utils.InferRunner;
 public class NSNumberTest {
 
   public static final String NSNUMBER_FILE =
-      "infer/tests/codetoanalyze/objc/errors/bad_ptr_comparisons/nsnumber.m";
+      "infer/tests/codetoanalyze/objc/linters/nsnumber.m";
 
   private static ImmutableList<String> inferCmd;
 
@@ -48,18 +48,10 @@ public class NSNumberTest {
   public void badNSNumberPointerComparisonShouldBeFound()
       throws InterruptedException, IOException, InferException {
     InferResults inferResults = InferRunner.runInferObjC(inferCmd);
-    String[] methods = {
-      "bad1",
-      "bad2"
-    };
     assertThat(
         "Results should contain " + BAD_POINTER_COMPARISON,
         inferResults,
-        containsExactly(
-            BAD_POINTER_COMPARISON,
-            NSNUMBER_FILE,
-            methods
-        )
+        containsLines(new int[]{13, 18, 23, 43})
     );
   }
 

infer/tests/utils/InferResults.java

@@ -67,6 +67,7 @@ public class InferResults {
             errorType.equals("PARAMETER_NOT_NULL_CHECKED") ||
             errorType.equals("DANGLING_POINTER_DEREFERENCE") ||
             errorType.equals("IVAR_NOT_NULL_CHECKED") ||
+            errorType.equals("BAD_POINTER_COMPARISON") ||
             errorType.startsWith("ERADICATE")) {
           Integer errorLine = Integer.parseInt(items[5].trim());
           String procedure = items[6];