[componentkit] Initializer rule should return multiple issues

Reviewed By: jvillard

Differential Revision: D4359088

fbshipit-source-id: cd6d075

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

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

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;

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

infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.mm

@@ -31,3 +31,6 @@
 
 @implementation UnavailableInitializer2
 @end
+
+@implementation LotsOfInitializers
+@end

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, []