Add a lint rule warning about ObjC custom setters for weak properties

Reviewed By: dulmarod

Differential Revision: D6219684

fbshipit-source-id: 586add0

infer/lib/linter_rules/linters.al

@@ -256,3 +256,20 @@ DEFINE-CHECKER POINTER_TO_CONST_OBJC_CLASS = {
   SET severity = "WARNING";
   SET mode = "ON";
 };
+
+
+DEFINE-CHECKER DISCOURAGED_WEAK_PROPERTY_CUSTOM_SETTER = {
+  LET has_body = HOLDS-NEXT WITH-TRANSITION Body (TRUE);
+  LET is_weak_property_setter =
+    HOLDS-NEXT WITH-TRANSITION AccessorForProperty "setter" (
+      is_weak_property()
+    );
+  SET report_when =
+    WHEN
+      has_body AND
+      is_weak_property_setter
+    HOLDS-IN-NODE ObjCMethodDecl;
+  SET message = "Custom setters are not called when ARC sets weak properties to nil.";
+  SET severity = "WARNING";
+  SET mode = "ON";
+};

infer/src/clang/cPredicates.ml

@@ -308,6 +308,14 @@ let is_strong_property an =
       false
 
 
+let is_weak_property an =
+  match an with
+  | Ctl_parser_types.Decl Clang_ast_t.ObjCPropertyDecl (_, _, pdi) ->
+      ObjcProperty_decl.is_weak_property pdi
+  | _ ->
+      false
+
+
 let is_assign_property an =
   match an with
   | Ctl_parser_types.Decl Clang_ast_t.ObjCPropertyDecl (_, _, pdi) ->

infer/src/clang/cPredicates.mli

@@ -39,6 +39,8 @@ val call_function : Ctl_parser_types.ast_node -> ALVar.alexp -> bool
 
 val is_strong_property : Ctl_parser_types.ast_node -> bool
 
+val is_weak_property : Ctl_parser_types.ast_node -> bool
+
 val is_assign_property : Ctl_parser_types.ast_node -> bool
 
 val is_property_pointer_type : Ctl_parser_types.ast_node -> bool

infer/src/clang/cTL.ml

@@ -18,6 +18,7 @@ module L = Logging
 
 (** Transition labels used for example to switch from decl to stmt *)
 type transitions =
+  | AccessorForProperty of ALVar.alexp  (** decl to decl *)
   | Body  (** decl to stmt *)
   | FieldName of ALVar.alexp  (** stmt to stmt, decl to decl *)
   | Fields  (** stmt to stmt, decl to decl *)
@@ -35,7 +36,7 @@ let is_transition_to_successor trans =
   match trans with
   | Body | InitExpr | FieldName _ | Fields | ParameterName _ | ParameterPos _ | Parameters | Cond ->
       true
-  | Super | PointerToDecl | Protocol ->
+  | Super | PointerToDecl | Protocol | AccessorForProperty _ ->
       false
 
 
@@ -134,6 +135,8 @@ module Debug = struct
   let pp_transition fmt trans_opt =
     let pp_aux fmt trans =
       match trans with
+      | AccessorForProperty kind ->
+          Format.pp_print_string fmt ("AccessorForProperty " ^ ALVar.alexp_to_string kind)
       | Body ->
           Format.pp_print_string fmt "Body"
       | FieldName name ->
@@ -648,6 +651,80 @@ let transition_decl_to_stmt d trs =
   List.fold ~f:(fun l e -> match e with Some st -> Stmt st :: l | _ -> l) temp_res ~init:[]
 
 
+let transition_decl_to_decl_via_accessor_for_property d desired_kind =
+  let open Clang_ast_t in
+  let find_property_for_accessor decl_opt predicate =
+    let decl_matches decl =
+      match decl with ObjCPropertyDecl (_, _, opdi) -> predicate opdi | _ -> false
+    in
+    match decl_opt with
+    | Some ObjCCategoryImplDecl (_, _, _, _, ocidi) ->
+        let category_decls =
+          match CAst_utils.get_decl_opt_with_decl_ref ocidi.ocidi_category_decl with
+          | Some ObjCCategoryDecl (_, _, decls, _, _) ->
+              List.filter ~f:decl_matches decls
+          | _ ->
+              []
+        in
+        let class_decls =
+          match CAst_utils.get_decl_opt_with_decl_ref ocidi.ocidi_class_interface with
+          | Some ObjCInterfaceDecl (_, _, decls, _, _) ->
+              List.filter ~f:decl_matches decls
+          | _ ->
+              []
+        in
+        category_decls @ class_decls
+    | Some ObjCImplementationDecl (_, _, _, _, oidi) -> (
+      match CAst_utils.get_decl_opt_with_decl_ref oidi.oidi_class_interface with
+      | Some ObjCInterfaceDecl (_, _, decls, _, _) ->
+          List.filter ~f:decl_matches decls
+      | _ ->
+          [] )
+    | _ ->
+        []
+  in
+  match d with
+  | ObjCMethodDecl (di, method_decl_name, mdi)
+    -> (
+      (* infer whether this method may be a getter or setter (or
+         neither) from its argument list *)
+      let num_params = List.length mdi.omdi_parameters in
+      let actual_kind, accessor_decl_ref_of_property_decl_info =
+        match num_params with
+        | 0 ->
+            ("getter", fun opdi -> opdi.opdi_getter_method)
+        | 1 ->
+            ("setter", fun opdi -> opdi.opdi_setter_method)
+        | _ ->
+            ("", fun _ -> None)
+      in
+      if not (ALVar.compare_str_with_alexp actual_kind desired_kind) then []
+      else
+        match CAst_utils.get_decl_opt_with_decl_ref mdi.omdi_property_decl with
+        | Some property_decl ->
+            (* clang handles most cases: property declarations with
+               accessor method declarations in the inferface; property
+               declarations in base classes; etc. *)
+            [Decl property_decl]
+        | None ->
+            (* search the interface for a matching property *)
+            let name_check opdi =
+              match accessor_decl_ref_of_property_decl_info opdi with
+              | None ->
+                  false
+              | Some dr ->
+                match dr.dr_name with
+                | Some ni ->
+                    String.equal method_decl_name.ni_name ni.ni_name
+                | _ ->
+                    false
+            in
+            let impl_decl_opt = CAst_utils.get_decl_opt di.di_parent_pointer in
+            List.map ~f:(fun x -> Decl x) (find_property_for_accessor impl_decl_opt name_check) )
+  | _ ->
+      []
+
+
 let transition_decl_to_decl_via_super d =
   let decl_opt_to_ast_node_opt d_opt = match d_opt with Some d' -> [Decl d'] | None -> [] in
   let do_ObjCImplementationDecl d =
@@ -865,6 +942,8 @@ let next_state_via_transition an trans =
       transition_via_parameter_name an name
   | an, ParameterPos pos ->
       transition_via_parameter_pos an pos
+  | Decl d, AccessorForProperty name ->
+      transition_decl_to_decl_via_accessor_for_property d name
   | _, _ ->
       []
 
@@ -967,6 +1046,8 @@ let rec eval_Atomic _pred_name args an lcxt =
       CPredicates.is_strong_property an
   | "is_unop_with_kind", [kind], an ->
       CPredicates.is_unop_with_kind an kind
+  | "is_weak_property", [], an ->
+      CPredicates.is_weak_property an
   | "iphoneos_target_sdk_version_greater_or_equal", [version], _ ->
       CPredicates.iphoneos_target_sdk_version_greater_or_equal lcxt (ALVar.alexp_to_string version)
   | "method_return_type", [typ], an ->

infer/src/clang/cTL.mli

@@ -16,6 +16,7 @@ open Ctl_parser_types
 
 (** Transition labels used for example to switch from decl to stmt *)
 type transitions =
+  | AccessorForProperty of ALVar.alexp  (** decl to decl *)
   | Body  (** decl to stmt *)
   | FieldName of ALVar.alexp  (** stmt to stmt, decl to decl *)
   | Fields  (** stmt to stmt, decl to decl *)

infer/src/clang/ctl_lexer.mll

@@ -70,6 +70,7 @@ rule token = parse
   | "NOT" { NOT }
   | "IMPLIES" { IMPLIES }
   | "REGEXP" { REGEXP }
+  | "AccessorForProperty" { ACCESSOR_FOR_PROPERTY }
   | "Any" {ANY}
   | "Fields" { FIELDS }
   | "FieldName" { FIELD_NAME }

infer/src/clang/ctl_parser.mly

@@ -75,6 +75,7 @@
 %token <string> STRING
 %token WHITELIST_PATH
 %token BLACKLIST_PATH
+%token ACCESSOR_FOR_PROPERTY
 %token ANY
 %token BODY
 %token COND
@@ -237,6 +238,7 @@ actual_params:
   ;
 
 transition_label:
+  | ACCESSOR_FOR_PROPERTY alexp { Some (CTL.AccessorForProperty $2) }
   | ANY { None }
   | BODY { Some CTL.Body }
   | COND { Some CTL.Cond }

infer/src/clang/objcProperty_decl.ml

@@ -24,6 +24,11 @@ let is_strong_property obj_c_property_decl_info =
   List.exists ~f:(fun a -> match a with `Strong -> true | _ -> false) attrs
 
 
+let is_weak_property obj_c_property_decl_info =
+  let attrs = obj_c_property_decl_info.Clang_ast_t.opdi_property_attributes in
+  List.exists ~f:(fun a -> match a with `Weak -> true | _ -> false) attrs
+
+
 let is_assign_property obj_c_property_decl_info =
   let attrs = obj_c_property_decl_info.Clang_ast_t.opdi_property_attributes in
   List.exists ~f:(fun a -> match a with `Assign -> true | _ -> false) attrs

infer/src/clang/objcProperty_decl.mli

@@ -9,7 +9,7 @@
 
 open! IStd
 
-(** Process properties by creating their getters and setters in the case that they need to be syntethized *)
+(** Process properties by creating their getters and setters in the case that they need to be synthesized *)
 
 (** or in the case of dynamic. *)
 
@@ -17,6 +17,10 @@ open! IStd
 
 val is_strong_property : Clang_ast_t.obj_c_property_decl_info -> bool
 
+(* Given a property type returns whether the property is weak *)
+
+val is_weak_property : Clang_ast_t.obj_c_property_decl_info -> bool
+
 (* Returns true if a property has the `assign` attribute *)
 
 val is_assign_property : Clang_ast_t.obj_c_property_decl_info -> bool

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

@@ -0,0 +1,102 @@
+/*
+ * Copyright (c) 2017 - present Facebook, Inc.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD style license found in the
+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ */
+
+#import <Foundation/NSObject.h>
+
+@interface Base : NSObject
+@property(atomic, weak) id baseProp;
+@property(atomic, weak, setter=setBaseAlternate:) Base* baseExplicitSetter;
+@property(atomic, weak) id baseWillBeImplementedInCategory;
+@end
+
+@protocol Protocol
+@property(atomic, weak) id protocolProp;
+@end
+
+@interface Derived : Base<Protocol>
+@property(atomic, strong) id strongProp;
+@property(atomic, weak) id derivedProp;
+@property(atomic, weak) id willBeImplementedInCategory;
+
+// No warnings for this; synthesized setters are OK.
+@property(atomic, weak) NSObject* synthesized;
+
+@property(atomic, weak) id unimplemented;
+- (id)unimplemented;
+// No warnings for this; this is not implemented here.
+- (void)setUnimplemented:(id)value;
+@end
+
+@implementation Derived
+
+// These should not generate warnings about custom weak setters:
+
+// Not setters:
+
+- (id)baseProp {
+  return nil;
+}
+
+- (id)strongProp {
+  return nil;
+}
+
+- (id)derivedProp {
+  return nil;
+}
+
+- (id)protocolProp {
+  return nil;
+}
+
+// A setter, but not a weak property.
+- (void)setStrongProp:(id)value {
+}
+
+// The following should generate warnings:
+
+- (void)setBaseProp:(id)value {
+}
+
+- (void)setBaseAlternate:(Base*)value {
+}
+
+- (void)setDerivedProp:(id)value {
+}
+
+- (void)setProtocolProp:(id)value {
+}
+
+@end
+
+@interface Derived (DerivedCategory)
+@property(atomic, weak) NSObject* derivedCategoryProp;
+@end
+
+@implementation Derived (DerivedCategory)
+
+- (NSObject*)derivedCategoryProp {
+  return nil;
+}
+
+// The following should generate warnings:
+
+// Custom setter for a weak property in a category
+- (void)setDerivedCategoryProp:(NSObject*)value {
+}
+
+// Custom setter for a weak property defined in a base class
+- (void)setBaseWillBeImplementedInCategory:(id)value {
+}
+
+// Custom setter for a weak property defined in the class interface
+- (void)setWillBeImplementedInCategory:(id)value {
+}
+
+@end

infer/tests/codetoanalyze/objc/linters/issues.exp

@@ -1,4 +1,11 @@
 codetoanalyze/objc/linters/Pointer_to_const_objc_class.m, Linters_dummy_method, 13, POINTER_TO_CONST_OBJC_CLASS, []
+codetoanalyze/objc/linters/Weak_property_setter.m, Derived_setBaseAlternate, 67, DISCOURAGED_WEAK_PROPERTY_CUSTOM_SETTER, []
+codetoanalyze/objc/linters/Weak_property_setter.m, Derived_setBaseProp, 64, DISCOURAGED_WEAK_PROPERTY_CUSTOM_SETTER, []
+codetoanalyze/objc/linters/Weak_property_setter.m, Derived_setBaseWillBeImplementedInCategory, 95, DISCOURAGED_WEAK_PROPERTY_CUSTOM_SETTER, []
+codetoanalyze/objc/linters/Weak_property_setter.m, Derived_setDerivedCategoryProp, 91, DISCOURAGED_WEAK_PROPERTY_CUSTOM_SETTER, []
+codetoanalyze/objc/linters/Weak_property_setter.m, Derived_setDerivedProp, 70, DISCOURAGED_WEAK_PROPERTY_CUSTOM_SETTER, []
+codetoanalyze/objc/linters/Weak_property_setter.m, Derived_setProtocolProp, 73, DISCOURAGED_WEAK_PROPERTY_CUSTOM_SETTER, []
+codetoanalyze/objc/linters/Weak_property_setter.m, Derived_setWillBeImplementedInCategory, 99, DISCOURAGED_WEAK_PROPERTY_CUSTOM_SETTER, []
 codetoanalyze/objc/linters/assign_pointer.m, Linters_dummy_method, 18, ASSIGN_POINTER_WARNING, []
 codetoanalyze/objc/linters/assign_pointer.m, Linters_dummy_method, 20, ASSIGN_POINTER_WARNING, []
 codetoanalyze/objc/linters/assign_pointer.m, Linters_dummy_method, 22, ASSIGN_POINTER_WARNING, []