diff --git a/infer/src/backend/InferPrint.re b/infer/src/backend/InferPrint.re index b60e4de2f..99a4be03e 100644 --- a/infer/src/backend/InferPrint.re +++ b/infer/src/backend/InferPrint.re @@ -491,6 +491,7 @@ let module IssuesTests = { [ Localise.assign_pointer_warning, Localise.bad_pointer_comparison, + Localise.component_factory_function, Localise.cxx_reference_captured_in_objc_block, Localise.direct_atomic_property_access, Localise.field_not_null_checked, diff --git a/infer/src/backend/localise.ml b/infer/src/backend/localise.ml index c3fa8c335..6b5608587 100644 --- a/infer/src/backend/localise.ml +++ b/infer/src/backend/localise.ml @@ -41,6 +41,7 @@ let bad_pointer_comparison = "BAD_POINTER_COMPARISON" 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 condition_always_false = "CONDITION_ALWAYS_FALSE" let condition_always_true = "CONDITION_ALWAYS_TRUE" let context_leak = "CONTEXT_LEAK" diff --git a/infer/src/backend/localise.mli b/infer/src/backend/localise.mli index f192e3d1e..0d05673a3 100644 --- a/infer/src/backend/localise.mli +++ b/infer/src/backend/localise.mli @@ -41,6 +41,7 @@ val condition_is_assignment : t val condition_always_false : t val condition_always_true : t val context_leak : t +val component_factory_function : t val cxx_reference_captured_in_objc_block : t val dangling_pointer_dereference : t val deallocate_stack_variable : t diff --git a/infer/src/clang/ComponentKit.ml b/infer/src/clang/ComponentKit.ml index 2e4296d8f..95f8fc473 100644 --- a/infer/src/clang/ComponentKit.ml +++ b/infer/src/clang/ComponentKit.ml @@ -10,6 +10,27 @@ open CFrontend_utils open !Utils +(** Recursively go up the inheritance hierarchy of a given ObjCInterfaceDecl. + (Returns false on decls other than that one.) *) +let rec is_component_if decl = + match decl with + | Some Clang_ast_t.ObjCInterfaceDecl (_, ndi, _, _, _)-> + let open CFrontend_config in + let whitelist = [ckcomponent_cl] in + let blacklist = [nsobject_cl; nsproxy_cl] in + let in_list some_list = IList.mem string_equal ndi.Clang_ast_t.ni_name some_list in + if in_list whitelist then + true + else if in_list blacklist then + false + else + (match Ast_utils.get_super_if decl with + | Some super_decl -> + is_component_if (Some super_decl) + | None -> false) + | _ -> false + + (** Recursively go up the inheritance hierarchy of a given ObjCInterfaceDecl. (Returns false on decls other than that one.) *) let rec is_component_or_controller_if decl = @@ -117,3 +138,41 @@ let mutable_local_vars_advice context decl = } else None | _ -> assert false (* Should only be called with a VarDecl *) + + +(** Catches functions that should be composite components. + http://componentkit.org/docs/break-out-composites.html + + Any static function that returns a subclass of CKComponent will be flagged. *) +let component_factory_function_advice context decl = + + let rec type_ptr_to_objc_if type_ptr = + let typ_opt = Ast_utils.get_desugared_type type_ptr in + match (typ_opt : Clang_ast_t.c_type option) with + | Some ObjCInterfaceType (_, decl_ptr) -> Ast_utils.get_decl decl_ptr + | Some ObjCObjectPointerType (_, (inner_qual_type: Clang_ast_t.qual_type)) -> + type_ptr_to_objc_if inner_qual_type.qt_type_ptr + | Some FunctionProtoType (_, function_type_info, _) + | Some FunctionNoProtoType (_, function_type_info) -> + type_ptr_to_objc_if function_type_info.Clang_ast_t.fti_return_type + | _ -> None in + + match decl with + | Clang_ast_t.FunctionDecl (decl_info, _, (qual_type: Clang_ast_t.qual_type), _) -> + let condition = context.CLintersContext.is_ck_translation_unit + && Ast_utils.is_in_main_file decl + && General_utils.is_objc_extension + && is_component_if (type_ptr_to_objc_if qual_type.qt_type_ptr) in + if condition then + Some { + CIssue.issue = CIssue.Component_factory_function; + CIssue.description = "Break out composite components"; + CIssue.suggestion = Some ( + "Prefer subclassing CKCompositeComponent to static helper functions \ + that return a CKComponent subclass. \ + http://componentkit.org/docs/break-out-composites.html" + ); + CIssue.loc = CFrontend_checkers.location_from_dinfo decl_info + } + else None + | _ -> assert false (* Should only be called with FunctionDecl *) diff --git a/infer/src/clang/ComponentKit.mli b/infer/src/clang/ComponentKit.mli index 1af27d769..124084d88 100644 --- a/infer/src/clang/ComponentKit.mli +++ b/infer/src/clang/ComponentKit.mli @@ -16,3 +16,6 @@ val contains_ck_impl : Clang_ast_t.decl list -> bool val mutable_local_vars_advice : CLintersContext.context -> Clang_ast_t.decl -> CIssue.issue_desc option + +val component_factory_function_advice : + CLintersContext.context -> Clang_ast_t.decl -> CIssue.issue_desc option diff --git a/infer/src/clang/cFrontend_errors.ml b/infer/src/clang/cFrontend_errors.ml index 8f8b4e6e0..5e8aec10e 100644 --- a/infer/src/clang/cFrontend_errors.ml +++ b/infer/src/clang/cFrontend_errors.ml @@ -76,6 +76,11 @@ let while_stmt_checker_list = [CFrontend_checkers.bad_pointer_comparison_warning let checker_for_while_stmt cond checker context = checker context cond +let function_decl_checker_list = [ComponentKit.component_factory_function_advice] + +let checker_for_function_decl decl checker context = + checker context decl + let get_err_log method_decl_opt = let procname = match method_decl_opt with | Some method_decl -> General_utils.procname_of_decl method_decl @@ -166,6 +171,11 @@ let run_frontend_checkers_on_decl context dec = let key = Ast_utils.generate_key_decl dec in invoke_set_of_checkers call_var_checker context key var_checker_list; context + | FunctionDecl _ -> + let call_function_decl_checker = checker_for_function_decl dec in + let key = Ast_utils.generate_key_decl dec in + invoke_set_of_checkers call_function_decl_checker context key function_decl_checker_list; + context | ObjCPropertyDecl _ -> let call_property_checker = checkers_for_property dec in let key = Ast_utils.generate_key_decl dec in diff --git a/infer/src/clang/cIssue.ml b/infer/src/clang/cIssue.ml index a01709ddd..acf774efb 100644 --- a/infer/src/clang/cIssue.ml +++ b/infer/src/clang/cIssue.ml @@ -10,6 +10,7 @@ type issue = | Assign_pointer_warning | Bad_pointer_comparison + | Component_factory_function | Cxx_reference_captured_in_objc_block | Direct_atomic_property_access | Global_variable_initialized_with_function_or_method_call @@ -24,6 +25,8 @@ let to_string issue = Localise.assign_pointer_warning | Bad_pointer_comparison -> Localise.bad_pointer_comparison + | Component_factory_function -> + Localise.component_factory_function | Cxx_reference_captured_in_objc_block -> Localise.cxx_reference_captured_in_objc_block | Direct_atomic_property_access -> @@ -47,6 +50,7 @@ let severity_of_issue issue = | Global_variable_initialized_with_function_or_method_call | Registered_observer_being_deallocated | Strong_delegate_warning -> Exceptions.Kwarning + | Component_factory_function | Mutable_local_variable_in_component_file -> Exceptions.Kadvice diff --git a/infer/src/clang/cIssue.mli b/infer/src/clang/cIssue.mli index b00559396..3971440fb 100644 --- a/infer/src/clang/cIssue.mli +++ b/infer/src/clang/cIssue.mli @@ -10,6 +10,7 @@ type issue = | Assign_pointer_warning | Bad_pointer_comparison + | Component_factory_function | Cxx_reference_captured_in_objc_block | Direct_atomic_property_access | Global_variable_initialized_with_function_or_method_call diff --git a/infer/tests/codetoanalyze/objcpp/linters/componentkit/FactoryFunctionTest.mm b/infer/tests/codetoanalyze/objcpp/linters/componentkit/FactoryFunctionTest.mm new file mode 100644 index 000000000..2013fdcf8 --- /dev/null +++ b/infer/tests/codetoanalyze/objcpp/linters/componentkit/FactoryFunctionTest.mm @@ -0,0 +1,37 @@ +/* + * 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" + +// Fake "component" (i.e. shouldn't fire for functions that return these) +@interface BarComponent : NSObject +@end + +@interface FooComponent : CKCompositeComponent +@end + +static FooComponent* ExampleFunctionBeforeImpl() { return nil; } + +static BarComponent* ExampleSkipFunctionBeforeImpl() { return nil; } + +@implementation FooComponent + ++ (instancetype)newWithObject:(NSObject*)obj { +} +static FooComponent* ExampleFunctionInsideImpl() { return nil; } + +static BarComponent* ExampleSkipFunctionInsideImpl() { return nil; } + +@end + +static FooComponent* ExampleFunctionAfterImpl() { return nil; } + +static BarComponent* ExampleSkipFunctionAfterImpl() { return nil; } diff --git a/infer/tests/codetoanalyze/objcpp/linters/issues.exp b/infer/tests/codetoanalyze/objcpp/linters/issues.exp index ad48e61cc..149d2ac0e 100644 --- a/infer/tests/codetoanalyze/objcpp/linters/issues.exp +++ b/infer/tests/codetoanalyze/objcpp/linters/issues.exp @@ -1,3 +1,6 @@ +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/MutableLocalVariablesTest.mm, BarComponent_new, 85, MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE componentkit/MutableLocalVariablesTest.mm, BarComponent_new, 87, MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE componentkit/MutableLocalVariablesTest.mm, FooComponent_newWithString:, 48, MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE