From c542b65a42d12993c1f9875a4316e6c931c0a6e6 Mon Sep 17 00:00:00 2001 From: Dominic Cooney Date: Mon, 6 Nov 2017 17:39:31 -0800 Subject: [PATCH] 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 | 17 +++ infer/src/clang/cPredicates.ml | 8 ++ infer/src/clang/cPredicates.mli | 2 + infer/src/clang/cTL.ml | 83 +++++++++++++- infer/src/clang/cTL.mli | 1 + infer/src/clang/ctl_lexer.mll | 1 + infer/src/clang/ctl_parser.mly | 2 + infer/src/clang/objcProperty_decl.ml | 5 + infer/src/clang/objcProperty_decl.mli | 6 +- .../objc/linters/Weak_property_setter.m | 102 ++++++++++++++++++ .../codetoanalyze/objc/linters/issues.exp | 7 ++ 11 files changed, 232 insertions(+), 2 deletions(-) create mode 100644 infer/tests/codetoanalyze/objc/linters/Weak_property_setter.m diff --git a/infer/lib/linter_rules/linters.al b/infer/lib/linter_rules/linters.al index 0eabf35ca..d9a630d9b 100644 --- a/infer/lib/linter_rules/linters.al +++ b/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"; +}; diff --git a/infer/src/clang/cPredicates.ml b/infer/src/clang/cPredicates.ml index 7f086212d..b8a27dc68 100644 --- a/infer/src/clang/cPredicates.ml +++ b/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) -> diff --git a/infer/src/clang/cPredicates.mli b/infer/src/clang/cPredicates.mli index db93dd375..0991a7523 100644 --- a/infer/src/clang/cPredicates.mli +++ b/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 diff --git a/infer/src/clang/cTL.ml b/infer/src/clang/cTL.ml index b0315eaf1..ee2dd87aa 100644 --- a/infer/src/clang/cTL.ml +++ b/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 -> diff --git a/infer/src/clang/cTL.mli b/infer/src/clang/cTL.mli index cf375cd63..ebf264b3f 100644 --- a/infer/src/clang/cTL.mli +++ b/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 *) diff --git a/infer/src/clang/ctl_lexer.mll b/infer/src/clang/ctl_lexer.mll index b4c3d3573..6b6384298 100644 --- a/infer/src/clang/ctl_lexer.mll +++ b/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 } diff --git a/infer/src/clang/ctl_parser.mly b/infer/src/clang/ctl_parser.mly index 921ab7a67..4e5550eca 100644 --- a/infer/src/clang/ctl_parser.mly +++ b/infer/src/clang/ctl_parser.mly @@ -75,6 +75,7 @@ %token 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 } diff --git a/infer/src/clang/objcProperty_decl.ml b/infer/src/clang/objcProperty_decl.ml index 137fc6320..bd5736668 100644 --- a/infer/src/clang/objcProperty_decl.ml +++ b/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 diff --git a/infer/src/clang/objcProperty_decl.mli b/infer/src/clang/objcProperty_decl.mli index 8c79eacd0..b715e4c58 100644 --- a/infer/src/clang/objcProperty_decl.mli +++ b/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 diff --git a/infer/tests/codetoanalyze/objc/linters/Weak_property_setter.m b/infer/tests/codetoanalyze/objc/linters/Weak_property_setter.m new file mode 100644 index 000000000..87567253a --- /dev/null +++ b/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 + +@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 +@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 diff --git a/infer/tests/codetoanalyze/objc/linters/issues.exp b/infer/tests/codetoanalyze/objc/linters/issues.exp index 257b8ea10..7d2a1d7d5 100644 --- a/infer/tests/codetoanalyze/objc/linters/issues.exp +++ b/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, []