diff --git a/infer/src/IR/Localise.ml b/infer/src/IR/Localise.ml index 8262a36e8..b9a0b51dc 100644 --- a/infer/src/IR/Localise.ml +++ b/infer/src/IR/Localise.ml @@ -42,6 +42,7 @@ let class_cast_exception = "CLASS_CAST_EXCEPTION" let comparing_floats_for_equality = "COMPARING_FLOAT_FOR_EQUALITY" let condition_is_assignment = "CONDITION_IS_ASSIGNMENT" let component_factory_function = "COMPONENT_FACTORY_FUNCTION" +let component_initializer_with_side_effects = "COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS" let component_with_multiple_factory_methods = "COMPONENT_WITH_MULTIPLE_FACTORY_METHODS" let component_with_unconventional_superclass = "COMPONENT_WITH_UNCONVENTIONAL_SUPERCLASS" let condition_always_false = "CONDITION_ALWAYS_FALSE" diff --git a/infer/src/IR/Localise.mli b/infer/src/IR/Localise.mli index 02688c7ac..8b5538cac 100644 --- a/infer/src/IR/Localise.mli +++ b/infer/src/IR/Localise.mli @@ -42,6 +42,7 @@ val condition_always_false : t val condition_always_true : t val context_leak : t val component_factory_function : t +val component_initializer_with_side_effects : t val component_with_multiple_factory_methods : t val component_with_unconventional_superclass : t val cxx_reference_captured_in_objc_block : t diff --git a/infer/src/backend/InferPrint.re b/infer/src/backend/InferPrint.re index dec31d9f0..a38226b9b 100644 --- a/infer/src/backend/InferPrint.re +++ b/infer/src/backend/InferPrint.re @@ -505,6 +505,7 @@ let module IssuesTests = { Localise.assign_pointer_warning, Localise.bad_pointer_comparison, Localise.component_factory_function, + Localise.component_initializer_with_side_effects, Localise.component_with_multiple_factory_methods, Localise.component_with_unconventional_superclass, Localise.cxx_reference_captured_in_objc_block, diff --git a/infer/src/clang/CLintersContext.ml b/infer/src/clang/CLintersContext.ml index 447915565..84367c269 100644 --- a/infer/src/clang/CLintersContext.ml +++ b/infer/src/clang/CLintersContext.ml @@ -11,9 +11,13 @@ type context = { translation_unit_context : CFrontend_config.translation_unit_context; current_method : Clang_ast_t.decl option; in_synchronized_block: bool; - is_ck_translation_unit: bool; (** True if the translation unit contains an ObjC class impl that's a subclass of CKComponent or CKComponentController. *) + is_ck_translation_unit: bool; + (** If inside an objc class impl, contains the objc class impl decl. *) + current_objc_impl : Clang_ast_t.decl option; + (** True if inside an objc static factory method (a class-level initializer, like +new) *) + in_objc_static_factory_method : bool; } let empty translation_unit_context = { @@ -21,4 +25,6 @@ let empty translation_unit_context = { translation_unit_context; in_synchronized_block = false; is_ck_translation_unit = false; + current_objc_impl = None; + in_objc_static_factory_method = false; } diff --git a/infer/src/clang/ComponentKit.ml b/infer/src/clang/ComponentKit.ml index fb8f7afe0..192961968 100644 --- a/infer/src/clang/ComponentKit.ml +++ b/infer/src/clang/ComponentKit.ml @@ -219,3 +219,48 @@ let component_with_multiple_factory_methods_advice context decl = | Some d when is_ck_context context decl -> check_interface d | _ -> None) | _ -> assert false + +let in_ck_class (context: CLintersContext.context) = + Option.map_default is_component_or_controller_descendant_impl false context.current_objc_impl + && General_utils.is_objc_extension context.translation_unit_context + +(** Components shouldn't have side-effects in its initializer. + + http://componentkit.org/docs/no-side-effects.html + + The only current way we look for side-effects is by looking for + asynchronous execution (dispatch_async, dispatch_after) and execution that + relies on other threads (dispatch_sync). Other side-effects, like reading + of global variables, is not checked by this analyzer, although still an + infraction of the rule. *) +let rec component_initializer_with_side_effects_advice + (context: CLintersContext.context) call_stmt = + let condition = + in_ck_class context + && context.in_objc_static_factory_method + && (match context.current_objc_impl with + | Some d -> Ast_utils.is_in_main_file context.translation_unit_context d + | None -> false) in + if condition then + match call_stmt with + | Clang_ast_t.ImplicitCastExpr (_, stmt :: _, _, _) -> + component_initializer_with_side_effects_advice context stmt + | Clang_ast_t.DeclRefExpr (_, _, _, decl_ref_expr_info) -> + let refs = [decl_ref_expr_info.drti_decl_ref; + decl_ref_expr_info.drti_found_decl_ref] in + (match IList.find_map_opt Ast_utils.name_of_decl_ref_opt refs with + | Some "dispatch_after" + | Some "dispatch_async" + | Some "dispatch_sync" -> + Some { + CIssue.issue = CIssue.Component_initializer_with_side_effects; + CIssue.description = "No Side-effects"; + CIssue.suggestion = Some "Your +new method should not modify any \ + global variables or global state."; + CIssue.loc = CFrontend_checkers.location_from_stmt context call_stmt + } + | _ -> None) + | _-> + None + else + None diff --git a/infer/src/clang/ComponentKit.mli b/infer/src/clang/ComponentKit.mli index 69f32b1cb..5e2a18602 100644 --- a/infer/src/clang/ComponentKit.mli +++ b/infer/src/clang/ComponentKit.mli @@ -25,3 +25,6 @@ val component_with_unconventional_superclass_advice : val component_with_multiple_factory_methods_advice : CLintersContext.context -> Clang_ast_t.decl -> CIssue.issue_desc option + +val component_initializer_with_side_effects_advice : + CLintersContext.context -> Clang_ast_t.stmt -> CIssue.issue_desc option diff --git a/infer/src/clang/cFrontend_checkers.mli b/infer/src/clang/cFrontend_checkers.mli index c430c6d11..c394d65b2 100644 --- a/infer/src/clang/cFrontend_checkers.mli +++ b/infer/src/clang/cFrontend_checkers.mli @@ -43,6 +43,9 @@ val checker_NSNotificationCenter : val global_var_init_with_calls_warning : CLintersContext.context -> Clang_ast_t.decl -> CIssue.issue_desc option +val location_from_stmt : + CLintersContext.context -> Clang_ast_t.stmt -> Location.t + val location_from_dinfo : CLintersContext.context -> Clang_ast_t.decl_info -> Location.t diff --git a/infer/src/clang/cFrontend_checkers_main.ml b/infer/src/clang/cFrontend_checkers_main.ml index f3567429f..fc03d4e77 100644 --- a/infer/src/clang/cFrontend_checkers_main.ml +++ b/infer/src/clang/cFrontend_checkers_main.ml @@ -61,10 +61,10 @@ let rec do_frontend_checks_stmt (context:CLintersContext.context) stmt = let stmts = Ast_utils.get_stmts_from_stmt stmt in IList.iter (do_all_checks_on_stmts) stmts -and do_frontend_checks_decl context decl = +and do_frontend_checks_decl (context: CLintersContext.context) decl = let open Clang_ast_t in let info = Clang_ast_proj.get_decl_tuple decl in - CLocation.update_curr_file context.CLintersContext.translation_unit_context info; + CLocation.update_curr_file context.translation_unit_context info; let context' = (match decl with | FunctionDecl(_, _, _, fdi) @@ -79,7 +79,18 @@ and do_frontend_checks_decl context decl = | None -> ()); context' | ObjCMethodDecl (_, _, mdi) -> - let context' = {context with CLintersContext.current_method = Some decl } in + let if_decl_opt = + (match context.current_objc_impl with + | Some ObjCImplementationDecl (_, _, _, _, impl_decl_info) -> + Ast_utils.get_decl_opt_with_decl_ref impl_decl_info.oidi_class_interface + | _ -> None) in + let is_factory_method = + (match if_decl_opt with + | Some if_decl -> Ast_utils.is_objc_factory_method if_decl decl + | _ -> false) in + let context' = {context with + CLintersContext.current_method = Some decl; + CLintersContext.in_objc_static_factory_method = is_factory_method} in (match mdi.Clang_ast_t.omdi_body with | Some stmt -> do_frontend_checks_stmt context' stmt @@ -95,7 +106,7 @@ and do_frontend_checks_decl context decl = | _ -> context) in let context'' = CFrontend_errors.run_frontend_checkers_on_decl context' decl in let context_with_orig_current_method = - {context'' with CLintersContext.current_method = context.CLintersContext.current_method } in + {context'' with CLintersContext.current_method = context.current_method } in match Clang_ast_proj.get_decl_context_tuple decl with | Some (decls, _) -> IList.iter (do_frontend_checks_decl context_with_orig_current_method) decls | None -> () diff --git a/infer/src/clang/cFrontend_errors.ml b/infer/src/clang/cFrontend_errors.ml index ccd854ce0..6ea87a097 100644 --- a/infer/src/clang/cFrontend_errors.ml +++ b/infer/src/clang/cFrontend_errors.ml @@ -49,6 +49,12 @@ let objc_impl_checker_list = [CFrontend_checkers.checker_NSNotificationCenter; let checkers_for_objc_impl decl checker context = checker context decl +let call_expr_checker_list = [ComponentKit.component_initializer_with_side_effects_advice] + +(* Invocation of checker belonging to call_expr_checker_list *) +let checkers_for_call_expr stmt checker context = + checker context stmt + (* List of checkers on variables *) let var_checker_list = [CFrontend_checkers.global_var_init_with_calls_warning; ComponentKit.mutable_local_vars_advice] @@ -164,18 +170,24 @@ let run_frontend_checkers_on_stmt context instr = let key = Ast_utils.generate_key_stmt cond in invoke_set_of_checkers call_checker context key while_stmt_checker_list; context + | CallExpr (_, called_func_stmt :: _, _) -> + let call_checker = checkers_for_call_expr called_func_stmt in + let key = Ast_utils.generate_key_stmt called_func_stmt in + invoke_set_of_checkers call_checker context key call_expr_checker_list; + context | ObjCAtSynchronizedStmt _ -> { context with CLintersContext.in_synchronized_block = true } | _ -> context -let run_frontend_checkers_on_decl context dec = +let run_frontend_checkers_on_decl (context: CLintersContext.context) dec = let open Clang_ast_t in match dec with | ObjCImplementationDecl _ -> let call_objc_impl_checker = checkers_for_objc_impl dec in let key = Ast_utils.generate_key_decl dec in - invoke_set_of_checkers call_objc_impl_checker context key objc_impl_checker_list; - context + let context' = {context with current_objc_impl = Some dec} in + invoke_set_of_checkers call_objc_impl_checker context' key objc_impl_checker_list; + context' | ObjCProtocolDecl _ -> let call_objc_protocol_checker = checkers_for_objc_protocol dec in let key = Ast_utils.generate_key_decl dec in diff --git a/infer/src/clang/cFrontend_utils.ml b/infer/src/clang/cFrontend_utils.ml index d4680bd1f..ffe38514a 100644 --- a/infer/src/clang/cFrontend_utils.ml +++ b/infer/src/clang/cFrontend_utils.ml @@ -505,6 +505,14 @@ struct (return_type_matches_class_type omdi.omdi_result_type if_type_decl_pointer) | _ -> false + let name_of_decl_ref_opt (decl_ref_opt: Clang_ast_t.decl_ref option) = + match decl_ref_opt with + | Some decl_ref -> + (match decl_ref.dr_name with + | Some named_decl_info -> Some named_decl_info.ni_name + | _ -> None) + | _ -> None + (* let rec getter_attribute_opt attributes = match attributes with diff --git a/infer/src/clang/cFrontend_utils.mli b/infer/src/clang/cFrontend_utils.mli index d4e2e200e..3a6ec6ccd 100644 --- a/infer/src/clang/cFrontend_utils.mli +++ b/infer/src/clang/cFrontend_utils.mli @@ -175,6 +175,8 @@ sig (** A class method that returns an instance of the class is a factory method. *) val is_objc_factory_method : Clang_ast_t.decl -> Clang_ast_t.decl -> bool + + val name_of_decl_ref_opt : Clang_ast_t.decl_ref option -> string option end module General_utils : diff --git a/infer/src/clang/cIssue.ml b/infer/src/clang/cIssue.ml index 87d0885ea..1968ec1de 100644 --- a/infer/src/clang/cIssue.ml +++ b/infer/src/clang/cIssue.ml @@ -11,6 +11,7 @@ type issue = | Assign_pointer_warning | Bad_pointer_comparison | Component_factory_function + | Component_initializer_with_side_effects | Component_with_multiple_factory_methods | Component_with_unconventional_superclass | Cxx_reference_captured_in_objc_block @@ -29,6 +30,8 @@ let to_string issue = Localise.bad_pointer_comparison | Component_factory_function -> Localise.component_factory_function + | Component_initializer_with_side_effects -> + Localise.component_initializer_with_side_effects | Component_with_multiple_factory_methods -> Localise.component_with_multiple_factory_methods | Component_with_unconventional_superclass -> @@ -57,6 +60,7 @@ let severity_of_issue issue = | Registered_observer_being_deallocated | Strong_delegate_warning -> Exceptions.Kwarning | Component_factory_function + | Component_initializer_with_side_effects | Component_with_multiple_factory_methods | Component_with_unconventional_superclass | Mutable_local_variable_in_component_file -> Exceptions.Kadvice diff --git a/infer/src/clang/cIssue.mli b/infer/src/clang/cIssue.mli index 2335bca8f..2fc28ca90 100644 --- a/infer/src/clang/cIssue.mli +++ b/infer/src/clang/cIssue.mli @@ -11,6 +11,7 @@ type issue = | Assign_pointer_warning | Bad_pointer_comparison | Component_factory_function + | Component_initializer_with_side_effects | Component_with_multiple_factory_methods | Component_with_unconventional_superclass | Cxx_reference_captured_in_objc_block diff --git a/infer/tests/codetoanalyze/objcpp/linters/componentkit/InitializerWithSideEffectTest.mm b/infer/tests/codetoanalyze/objcpp/linters/componentkit/InitializerWithSideEffectTest.mm new file mode 100644 index 000000000..af281dc00 --- /dev/null +++ b/infer/tests/codetoanalyze/objcpp/linters/componentkit/InitializerWithSideEffectTest.mm @@ -0,0 +1,73 @@ +/* + * Copyright (c) 2016 - 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 + +#import "FakeComponentKitHeader.h" + +@interface FooComponent : CKCompositeComponent +@end + +@implementation FooComponent ++ (instancetype)newDerp { + dispatch_async( // error + dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), + ^{ + // do something + }); + dispatch_sync( // error + dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), + ^{ + // do something + }); + dispatch_after( // error + DISPATCH_TIME_NOW, + dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), + ^{ + // do something + }); + // make sure nested things are caught + ^{ + dispatch_sync( // error + dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), + ^{ + ^{ + dispatch_async( // error + dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), + ^{ + // do something + }); + }(); + }); + }(); + return nil; +} + +- (void)derp { + // not an error (only run on factory functions) + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), + ^{ + // do something + }); +} +@end + +@interface BarComponent : NSObject // not a real component +@end + +@implementation BarComponent ++ (instancetype)newDerp { + // not an error (only run on actual components) + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), + ^{ + // do something + }); + return nil; +} +@end diff --git a/infer/tests/codetoanalyze/objcpp/linters/issues.exp b/infer/tests/codetoanalyze/objcpp/linters/issues.exp index 9cfe2f301..b6e90f22a 100644 --- a/infer/tests/codetoanalyze/objcpp/linters/issues.exp +++ b/infer/tests/codetoanalyze/objcpp/linters/issues.exp @@ -1,6 +1,11 @@ componentkit/FactoryFunctionTest.mm, ExampleFunctionAfterImpl, 35, COMPONENT_FACTORY_FUNCTION componentkit/FactoryFunctionTest.mm, ExampleFunctionBeforeImpl, 21, COMPONENT_FACTORY_FUNCTION componentkit/FactoryFunctionTest.mm, ExampleFunctionInsideImpl, 29, COMPONENT_FACTORY_FUNCTION +componentkit/InitializerWithSideEffectTest.mm, FooComponent_newDerp, 19, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS +componentkit/InitializerWithSideEffectTest.mm, FooComponent_newDerp, 24, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS +componentkit/InitializerWithSideEffectTest.mm, FooComponent_newDerp, 29, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS +componentkit/InitializerWithSideEffectTest.mm, __objc_anonymous_block_______1, 37, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS +componentkit/InitializerWithSideEffectTest.mm, __objc_anonymous_block_______2, 41, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS componentkit/MultipleFactoryMethodsTest.h, Linters_dummy_method, 18, COMPONENT_WITH_MULTIPLE_FACTORY_METHODS componentkit/MultipleFactoryMethodsTest.h, Linters_dummy_method, 27, COMPONENT_WITH_MULTIPLE_FACTORY_METHODS componentkit/MutableLocalVariablesTest.mm, BarComponent_new, 85, MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE