[componentkit] Multiple factory methods

Reviewed By: dulmarod

Differential Revision: D4033747

fbshipit-source-id: cf2801b

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_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"
 let condition_always_true = "CONDITION_ALWAYS_TRUE"

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_with_multiple_factory_methods : t
 val component_with_unconventional_superclass : t
 val cxx_reference_captured_in_objc_block : t
 val dangling_pointer_dereference : t

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_with_multiple_factory_methods,
             Localise.component_with_unconventional_superclass,
             Localise.cxx_reference_captured_in_objc_block,
             Localise.direct_atomic_property_access,

infer/src/clang/ComponentKit.ml

@@ -183,3 +183,68 @@ let component_with_unconventional_superclass_advice context decl =
       else
         None
   | _ -> assert false
+
+let if_decl_to_di_pointer_opt if_decl =
+  match if_decl with
+  | Clang_ast_t.ObjCInterfaceDecl (if_decl_info, _, _, _, _) ->
+      Some if_decl_info.di_pointer
+  | _ -> None
+
+let is_instance_type type_ptr =
+  match Ast_utils.name_opt_of_typedef_type_ptr type_ptr with
+  | Some name -> name = "instancetype"
+  | None -> false
+
+let return_type_matches_class_type rtp type_decl_pointer =
+  if is_instance_type rtp then
+    true
+  else
+    let return_type_decl_opt = Ast_utils.type_ptr_to_objc_interface rtp in
+    let return_type_decl_pointer_opt =
+      Option.map if_decl_to_di_pointer_opt return_type_decl_opt in
+    (Some type_decl_pointer) = return_type_decl_pointer_opt
+
+(** A class method that returns an instance of the class is a factory method. *)
+let is_factory_method if_decl meth_decl =
+  let if_type_decl_pointer = if_decl_to_di_pointer_opt if_decl in
+  match meth_decl with
+  | Clang_ast_t.ObjCMethodDecl (_, _, omdi) ->
+      (not omdi.omdi_is_instance_method) &&
+      (return_type_matches_class_type omdi.omdi_result_type if_type_decl_pointer)
+  | _ -> false
+
+(** Components should only have one factory method.
+
+    (They could technically have none if they re-use the parent class's factory
+    method.)
+
+    We care about ones that are declared in the interface. In other words, if
+    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. *)
+let component_with_multiple_factory_methods_advice context decl =
+  let check_interface if_decl =
+    match if_decl with
+    | Clang_ast_t.ObjCInterfaceDecl (decl_info, _, decls, _, _) ->
+        let factory_methods = IList.filter (is_factory_method if_decl) decls in
+        if (IList.length factory_methods) > 1 then
+          Some {
+            CIssue.issue = CIssue.Component_with_multiple_factory_methods;
+            CIssue.description = "Avoid Overrides";
+            CIssue.suggestion =
+              Some "Instead, always expose all parameters in a single \
+                    designated initializer and document which are optional.";
+            CIssue.loc = CFrontend_checkers.location_from_dinfo context decl_info
+          }
+        else
+          None
+    | _ -> assert false in
+  match decl with
+  | Clang_ast_t.ObjCImplementationDecl (_, _, _, _, impl_decl_info) ->
+      let if_decl_opt =
+        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 decl -> check_interface d
+       | _ -> None)
+  | _ -> assert false

infer/src/clang/ComponentKit.mli

@@ -22,3 +22,6 @@ val component_factory_function_advice :
 
 val component_with_unconventional_superclass_advice :
   CLintersContext.context -> Clang_ast_t.decl -> CIssue.issue_desc option
+
+val component_with_multiple_factory_methods_advice :
+  CLintersContext.context -> Clang_ast_t.decl -> CIssue.issue_desc option

infer/src/clang/cFrontend_errors.ml

@@ -42,6 +42,7 @@ let checkers_for_objc_protocol decl checker context =
 
 (* List of checkers running on ObjCImpl decls *)
 let objc_impl_checker_list = [CFrontend_checkers.checker_NSNotificationCenter;
+                              ComponentKit.component_with_multiple_factory_methods_advice;
                               ComponentKit.component_with_unconventional_superclass_advice]
 
 (* Invocation of checker belonging to objc_impl_checker_list *)

infer/src/clang/cIssue.ml

@@ -11,6 +11,7 @@ type issue =
   | Assign_pointer_warning
   | Bad_pointer_comparison
   | Component_factory_function
+  | Component_with_multiple_factory_methods
   | Component_with_unconventional_superclass
   | Cxx_reference_captured_in_objc_block
   | Direct_atomic_property_access
@@ -28,6 +29,8 @@ let to_string issue =
          Localise.bad_pointer_comparison
      | Component_factory_function ->
          Localise.component_factory_function
+     | Component_with_multiple_factory_methods ->
+         Localise.component_with_multiple_factory_methods
      | Component_with_unconventional_superclass ->
          Localise.component_with_unconventional_superclass
      | Cxx_reference_captured_in_objc_block ->
@@ -54,6 +57,7 @@ let severity_of_issue issue =
   | Registered_observer_being_deallocated
   | Strong_delegate_warning -> Exceptions.Kwarning
   | Component_factory_function
+  | Component_with_multiple_factory_methods
   | Component_with_unconventional_superclass
   | Mutable_local_variable_in_component_file -> Exceptions.Kadvice
 

infer/src/clang/cIssue.mli

@@ -11,6 +11,7 @@ type issue =
   | Assign_pointer_warning
   | Bad_pointer_comparison
   | Component_factory_function
+  | Component_with_multiple_factory_methods
   | Component_with_unconventional_superclass
   | Cxx_reference_captured_in_objc_block
   | Direct_atomic_property_access

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

@@ -0,0 +1,38 @@
+/*
+ * 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 <Foundation/Foundation.h>
+
+#import "FakeComponentKitHeader.h"
+
+@interface FooComponent : CKCompositeComponent // OK - only one factory method
++ (instancetype)newWithObject:(NSObject*)obj;
+@end
+
+@interface BarComponent : CKCompositeComponent // Not OK - two factory methods
++ (instancetype)newWithObject1:(NSObject*)obj;
++ (instancetype)newWithObject2:(NSObject*)obj;
+@end
+
+@interface BazComponent : CKCompositeComponent // OK - no factory methods (?)
+@end
+
+// Not OK - Using the class name instaed of `instancename` doesn't make this OK
+@interface BadComponent : CKCompositeComponent
++ (instancetype)newWithObject1:(NSObject*)obj;
++ (BadComponent*)newWithObject2:(NSObject*)obj;
+@end
+
+// OK - don't count non-factory methods
+@interface OKComponent : CKCompositeComponent
++ (instancetype)newWithObject:(NSObject*)obj;
++ (FooComponent*)newOtherWithObject:(NSObject*)obj; // different class
++ (int)somethingElse;
+- (NSString*)blah:(int)lol;
+@end

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

@@ -0,0 +1,27 @@
+/*
+ * 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 <Foundation/Foundation.h>
+
+#import "MultipleFactoryMethodsTest.h"
+
+@implementation FooComponent
+@end
+
+@implementation BarComponent
+@end
+
+@implementation BazComponent
+@end
+
+@implementation BadComponent
+@end
+
+@implementation OKComponent
+@end

infer/tests/codetoanalyze/objcpp/linters/componentkit/UnconventionalSuperclassTest.h

@@ -0,0 +1,18 @@
+/*
+ * 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 <Foundation/Foundation.h>
+
+#import "FakeComponentKitHeader.h"
+
+@interface FooComponent : CKCompositeComponent // good
+@end
+
+@interface BarComponent : FooComponent // bad
+@end

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

@@ -0,0 +1,18 @@
+/*
+ * 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 <Foundation/Foundation.h>
+
+#import "UnconventionalSuperclassTest.h"
+
+@implementation FooComponent
+@end
+
+@implementation BarComponent
+@end

infer/tests/codetoanalyze/objcpp/linters/issues.exp

@@ -1,6 +1,8 @@
 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/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
 componentkit/MutableLocalVariablesTest.mm, BarComponent_new, 87, MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE
 componentkit/MutableLocalVariablesTest.mm, FooComponent_newWithString:, 48, MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE