[componentkit] Factory functions analyzer

Reviewed By: dulmarod

Differential Revision: D3933693

fbshipit-source-id: 6294fb4

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,

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"

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

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 *)

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

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

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
 
 

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

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 <Foundation/Foundation.h>
+
+#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; }

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