From b557b4992198a445b49912cbe7f731a1b10984fb Mon Sep 17 00:00:00 2001 From: Ryan Rhee Date: Thu, 22 Dec 2016 10:41:26 -0800 Subject: [PATCH] [componentkit] Initializer rule should return multiple issues Reviewed By: jvillard Differential Revision: D4359088 fbshipit-source-id: cd6d075 --- infer/src/clang/ComponentKit.ml | 34 +++++++++---------- infer/src/clang/ComponentKit.mli | 2 +- infer/src/clang/cFrontend_errors.ml | 7 ++-- .../componentkit/MultipleFactoryMethodsTest.h | 8 +++++ .../MultipleFactoryMethodsTest.mm | 3 ++ .../codetoanalyze/objcpp/linters/issues.exp | 9 +++-- 6 files changed, 39 insertions(+), 24 deletions(-) diff --git a/infer/src/clang/ComponentKit.ml b/infer/src/clang/ComponentKit.ml index 3cc2cb7e7..8ef7b3dfe 100644 --- a/infer/src/clang/ComponentKit.ml +++ b/infer/src/clang/ComponentKit.ml @@ -219,7 +219,10 @@ let component_with_unconventional_superclass_advice context an = additional factory methods are implementation-only, the rule doesn't catch it. While its existence is probably not good, I can't think of any reason there would be factory methods that aren't exposed outside of a class is - not useful if there's only one public factory method. *) + not useful if there's only one public factory method. + + Given n factory methods, the rule should emit n-1 issues. Each issue's + location should point to the method declaration. *) let component_with_multiple_factory_methods_advice context an = let is_unavailable_attr attr = match attr with | Clang_ast_t.UnavailableAttr _ -> true @@ -234,21 +237,18 @@ let component_with_multiple_factory_methods_advice context an = let check_interface if_decl = match if_decl with - | Clang_ast_t.ObjCInterfaceDecl (decl_info, _, decls, _, _) -> + | Clang_ast_t.ObjCInterfaceDecl (_, _, decls, _, _) -> let factory_methods = IList.filter (is_available_factory_method if_decl) decls in - if (IList.length factory_methods) > 1 then - CTL.True, Some { - CIssue.name = "COMPONENT_WITH_MULTIPLE_FACTORY_METHODS"; - severity = Exceptions.Kadvice; - mode = CIssue.On; - description = "Avoid Overrides"; - suggestion = - Some "Instead, always expose all parameters in a single \ - designated initializer and document which are optional."; - loc = CFrontend_checkers.location_from_dinfo context decl_info - } - else - CTL.False, None + CTL.True, IList.map (fun meth_decl -> { + CIssue.name = "COMPONENT_WITH_MULTIPLE_FACTORY_METHODS"; + severity = Exceptions.Kadvice; + mode = CIssue.On; + description = "Avoid Overrides"; + suggestion = + Some "Instead, always expose all parameters in a single \ + designated initializer and document which are optional."; + loc = CFrontend_checkers.location_from_decl context meth_decl + }) (IList.drop_first 1 factory_methods) | _ -> assert false in match an with | CTL.Decl (Clang_ast_t.ObjCImplementationDecl (_, _, _, _, impl_decl_info)) -> @@ -256,8 +256,8 @@ let component_with_multiple_factory_methods_advice context an = Ast_utils.get_decl_opt_with_decl_ref impl_decl_info.oidi_class_interface in (match if_decl_opt with | Some d when is_ck_context context an -> check_interface d - | _ -> CTL.False, None) - | _ -> CTL.False, None + | _ -> CTL.False, []) + | _ -> CTL.False, [] let in_ck_class (context: CLintersContext.context) = Option.value_map ~f:is_component_or_controller_descendant_impl ~default:false diff --git a/infer/src/clang/ComponentKit.mli b/infer/src/clang/ComponentKit.mli index 3b261ddde..d98cfc3d6 100644 --- a/infer/src/clang/ComponentKit.mli +++ b/infer/src/clang/ComponentKit.mli @@ -26,7 +26,7 @@ val component_with_unconventional_superclass_advice : CLintersContext.context -> CTL.ast_node -> CTL.t * CIssue.issue_desc option val component_with_multiple_factory_methods_advice : - CLintersContext.context -> CTL.ast_node -> CTL.t * CIssue.issue_desc option + CLintersContext.context -> CTL.ast_node -> CTL.t * CIssue.issue_desc list val component_initializer_with_side_effects_advice : CLintersContext.context -> CTL.ast_node -> CTL.t * CIssue.issue_desc option diff --git a/infer/src/clang/cFrontend_errors.ml b/infer/src/clang/cFrontend_errors.ml index fae30e18a..c7853f185 100644 --- a/infer/src/clang/cFrontend_errors.ml +++ b/infer/src/clang/cFrontend_errors.ml @@ -24,11 +24,12 @@ let decl_single_checkers_list = [CFrontend_checkers.ctl_strong_delegate_warning; ComponentKit.component_with_unconventional_superclass_advice; ComponentKit.mutable_local_vars_advice; ComponentKit.component_factory_function_advice; - ComponentKit.component_file_cyclomatic_complexity_info; - ComponentKit.component_with_multiple_factory_methods_advice;] + ComponentKit.component_file_cyclomatic_complexity_info;] (* List of checkers on decls *) -let decl_checkers_list = IList.map single_to_multi decl_single_checkers_list +let decl_checkers_list = + ComponentKit.component_with_multiple_factory_methods_advice:: + (IList.map single_to_multi decl_single_checkers_list) (* List of checkers on stmts *that return 0 or 1 issue* *) let stmt_single_checkers_list = [CFrontend_checkers.ctl_direct_atomic_property_access_warning; diff --git a/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h b/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h index bde7b5641..6381ac99a 100644 --- a/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h +++ b/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h @@ -51,3 +51,11 @@ + (instancetype)newWithObject1:(NSObject*)obj; + (instancetype)newWithObject2:(NSObject*)obj; @end + +// 4 initializers -> should output 3 issues +@interface LotsOfInitializers : CKCompositeComponent ++ (instancetype)newWithObject1:(NSObject*)obj; ++ (instancetype)newWithObject2:(NSObject*)obj; ++ (instancetype)newWithObject3:(NSObject*)obj; ++ (instancetype)newWithObject4:(NSObject*)obj; +@end diff --git a/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.mm b/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.mm index c4deff165..dffafa8e6 100644 --- a/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.mm +++ b/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.mm @@ -31,3 +31,6 @@ @implementation UnavailableInitializer2 @end + +@implementation LotsOfInitializers +@end diff --git a/infer/tests/codetoanalyze/objcpp/linters/issues.exp b/infer/tests/codetoanalyze/objcpp/linters/issues.exp index f253268a1..a6f779383 100644 --- a/infer/tests/codetoanalyze/objcpp/linters/issues.exp +++ b/infer/tests/codetoanalyze/objcpp/linters/issues.exp @@ -6,9 +6,12 @@ codetoanalyze/objcpp/linters/componentkit/InitializerWithSideEffectTest.mm, FooC codetoanalyze/objcpp/linters/componentkit/InitializerWithSideEffectTest.mm, FooComponent_newDerp, 29, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS, [] codetoanalyze/objcpp/linters/componentkit/InitializerWithSideEffectTest.mm, __objc_anonymous_block_______1, 37, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS, [] codetoanalyze/objcpp/linters/componentkit/InitializerWithSideEffectTest.mm, __objc_anonymous_block_______2, 41, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS, [] -codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h, Linters_dummy_method, 18, COMPONENT_WITH_MULTIPLE_FACTORY_METHODS, [] -codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h, Linters_dummy_method, 27, COMPONENT_WITH_MULTIPLE_FACTORY_METHODS, [] -codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h, Linters_dummy_method, 48, COMPONENT_WITH_MULTIPLE_FACTORY_METHODS, [] +codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h, Linters_dummy_method, 20, COMPONENT_WITH_MULTIPLE_FACTORY_METHODS, [] +codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h, Linters_dummy_method, 29, COMPONENT_WITH_MULTIPLE_FACTORY_METHODS, [] +codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h, Linters_dummy_method, 52, COMPONENT_WITH_MULTIPLE_FACTORY_METHODS, [] +codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h, Linters_dummy_method, 58, COMPONENT_WITH_MULTIPLE_FACTORY_METHODS, [] +codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h, Linters_dummy_method, 59, COMPONENT_WITH_MULTIPLE_FACTORY_METHODS, [] +codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h, Linters_dummy_method, 60, COMPONENT_WITH_MULTIPLE_FACTORY_METHODS, [] codetoanalyze/objcpp/linters/componentkit/MutableLocalVariablesTest.mm, BarComponent_new, 89, MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE, [] codetoanalyze/objcpp/linters/componentkit/MutableLocalVariablesTest.mm, BarComponent_new, 91, MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE, [] codetoanalyze/objcpp/linters/componentkit/MutableLocalVariablesTest.mm, BarComponent_new, 110, MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE, []