[componentkit] Initializer With Side-Effect Analyzer

Reviewed By: dulmarod

Differential Revision: D4063217

fbshipit-source-id: 7196a64

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_initializer_with_side_effects = "COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS"
 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"

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_initializer_with_side_effects : t
 val component_with_multiple_factory_methods : t
 val component_with_unconventional_superclass : t
 val cxx_reference_captured_in_objc_block : 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_initializer_with_side_effects,
             Localise.component_with_multiple_factory_methods,
             Localise.component_with_unconventional_superclass,
             Localise.cxx_reference_captured_in_objc_block,

infer/src/clang/CLintersContext.ml

@@ -11,9 +11,13 @@ type context = {
   translation_unit_context : CFrontend_config.translation_unit_context;
   current_method : Clang_ast_t.decl option;
   in_synchronized_block: bool;
-  is_ck_translation_unit: bool;
   (** True if the translation unit contains an ObjC class impl that's a subclass
       of CKComponent or CKComponentController. *)
+  is_ck_translation_unit: bool;
+  (** If inside an objc class impl, contains the objc class impl decl. *)
+  current_objc_impl : Clang_ast_t.decl option;
+  (** True if inside an objc static factory method (a class-level initializer, like +new) *)
+  in_objc_static_factory_method : bool;
 }
 
 let empty translation_unit_context = {
@@ -21,4 +25,6 @@ let empty translation_unit_context = {
   translation_unit_context;
   in_synchronized_block = false;
   is_ck_translation_unit = false;
+  current_objc_impl = None;
+  in_objc_static_factory_method = false;
 }

infer/src/clang/ComponentKit.ml

@@ -219,3 +219,48 @@ let component_with_multiple_factory_methods_advice context decl =
        | Some d when is_ck_context context decl -> check_interface d
        | _ -> None)
   | _ -> assert false
+
+let in_ck_class (context: CLintersContext.context) =
+  Option.map_default is_component_or_controller_descendant_impl false context.current_objc_impl
+  && General_utils.is_objc_extension context.translation_unit_context
+
+(** Components shouldn't have side-effects in its initializer.
+
+    http://componentkit.org/docs/no-side-effects.html
+
+    The only current way we look for side-effects is by looking for
+    asynchronous execution (dispatch_async, dispatch_after) and execution that
+    relies on other threads (dispatch_sync). Other side-effects, like reading
+    of global variables, is not checked by this analyzer, although still an
+    infraction of the rule. *)
+let rec component_initializer_with_side_effects_advice
+    (context: CLintersContext.context) call_stmt =
+  let condition =
+    in_ck_class context
+    && context.in_objc_static_factory_method
+    && (match context.current_objc_impl with
+        | Some d -> Ast_utils.is_in_main_file context.translation_unit_context d
+        | None -> false) in
+  if condition then
+    match call_stmt with
+    | Clang_ast_t.ImplicitCastExpr (_, stmt :: _, _, _) ->
+        component_initializer_with_side_effects_advice context stmt
+    | Clang_ast_t.DeclRefExpr (_, _, _, decl_ref_expr_info) ->
+        let refs = [decl_ref_expr_info.drti_decl_ref;
+                    decl_ref_expr_info.drti_found_decl_ref] in
+        (match IList.find_map_opt Ast_utils.name_of_decl_ref_opt refs with
+         | Some "dispatch_after"
+         | Some "dispatch_async"
+         | Some "dispatch_sync" ->
+             Some {
+               CIssue.issue = CIssue.Component_initializer_with_side_effects;
+               CIssue.description = "No Side-effects";
+               CIssue.suggestion = Some "Your +new method should not modify any \
+                                         global variables or global state.";
+               CIssue.loc = CFrontend_checkers.location_from_stmt context call_stmt
+             }
+         | _ -> None)
+    | _->
+        None
+  else
+    None

infer/src/clang/ComponentKit.mli

@@ -25,3 +25,6 @@ val component_with_unconventional_superclass_advice :
 
 val component_with_multiple_factory_methods_advice :
   CLintersContext.context -> Clang_ast_t.decl -> CIssue.issue_desc option
+
+val component_initializer_with_side_effects_advice :
+  CLintersContext.context -> Clang_ast_t.stmt -> CIssue.issue_desc option

infer/src/clang/cFrontend_checkers.mli

@@ -43,6 +43,9 @@ val checker_NSNotificationCenter :
 val global_var_init_with_calls_warning :
   CLintersContext.context -> Clang_ast_t.decl -> CIssue.issue_desc option
 
+val location_from_stmt :
+  CLintersContext.context -> Clang_ast_t.stmt -> Location.t
+
 val location_from_dinfo :
   CLintersContext.context -> Clang_ast_t.decl_info -> Location.t
 

infer/src/clang/cFrontend_checkers_main.ml

@@ -61,10 +61,10 @@ let rec do_frontend_checks_stmt (context:CLintersContext.context) stmt =
   let stmts = Ast_utils.get_stmts_from_stmt stmt in
   IList.iter (do_all_checks_on_stmts) stmts
 
-and do_frontend_checks_decl context decl =
+and do_frontend_checks_decl (context: CLintersContext.context) decl =
   let open Clang_ast_t in
   let info = Clang_ast_proj.get_decl_tuple decl in
-  CLocation.update_curr_file context.CLintersContext.translation_unit_context info;
+  CLocation.update_curr_file context.translation_unit_context info;
   let context' =
     (match decl with
      | FunctionDecl(_, _, _, fdi)
@@ -79,7 +79,18 @@ and do_frontend_checks_decl context decl =
           | None -> ());
          context'
      | ObjCMethodDecl (_, _, mdi) ->
-         let context' = {context with CLintersContext.current_method = Some decl } in
+         let if_decl_opt =
+           (match context.current_objc_impl with
+            | Some ObjCImplementationDecl (_, _, _, _, impl_decl_info) ->
+                Ast_utils.get_decl_opt_with_decl_ref impl_decl_info.oidi_class_interface
+            | _ -> None) in
+         let is_factory_method =
+           (match if_decl_opt with
+            | Some if_decl -> Ast_utils.is_objc_factory_method if_decl decl
+            | _ -> false) in
+         let context' = {context with
+                         CLintersContext.current_method = Some decl;
+                         CLintersContext.in_objc_static_factory_method = is_factory_method} in
          (match mdi.Clang_ast_t.omdi_body with
           | Some stmt ->
               do_frontend_checks_stmt context' stmt
@@ -95,7 +106,7 @@ and do_frontend_checks_decl context decl =
      | _ -> context) in
   let context'' = CFrontend_errors.run_frontend_checkers_on_decl context' decl in
   let context_with_orig_current_method =
-    {context'' with CLintersContext.current_method = context.CLintersContext.current_method } in
+    {context'' with CLintersContext.current_method = context.current_method } in
   match Clang_ast_proj.get_decl_context_tuple decl with
   | Some (decls, _) -> IList.iter (do_frontend_checks_decl context_with_orig_current_method) decls
   | None -> ()

infer/src/clang/cFrontend_errors.ml

@@ -49,6 +49,12 @@ let objc_impl_checker_list = [CFrontend_checkers.checker_NSNotificationCenter;
 let checkers_for_objc_impl decl checker context =
   checker context decl
 
+let call_expr_checker_list = [ComponentKit.component_initializer_with_side_effects_advice]
+
+(* Invocation of checker belonging to call_expr_checker_list *)
+let checkers_for_call_expr stmt checker context =
+  checker context stmt
+
 (* List of checkers on variables *)
 let var_checker_list = [CFrontend_checkers.global_var_init_with_calls_warning;
                         ComponentKit.mutable_local_vars_advice]
@@ -164,18 +170,24 @@ let run_frontend_checkers_on_stmt context instr =
       let key = Ast_utils.generate_key_stmt cond in
       invoke_set_of_checkers call_checker context key while_stmt_checker_list;
       context
+  | CallExpr (_, called_func_stmt :: _, _) ->
+      let call_checker = checkers_for_call_expr called_func_stmt in
+      let key = Ast_utils.generate_key_stmt called_func_stmt in
+      invoke_set_of_checkers call_checker context key call_expr_checker_list;
+      context
   | ObjCAtSynchronizedStmt _ ->
       { context with CLintersContext.in_synchronized_block = true }
   | _ -> context
 
-let run_frontend_checkers_on_decl context dec =
+let run_frontend_checkers_on_decl (context: CLintersContext.context) dec =
   let open Clang_ast_t in
   match dec with
   | ObjCImplementationDecl _ ->
       let call_objc_impl_checker = checkers_for_objc_impl dec in
       let key = Ast_utils.generate_key_decl dec in
-      invoke_set_of_checkers call_objc_impl_checker context key objc_impl_checker_list;
-      context
+      let context' = {context with current_objc_impl = Some dec} in
+      invoke_set_of_checkers call_objc_impl_checker context' key objc_impl_checker_list;
+      context'
   | ObjCProtocolDecl _ ->
       let call_objc_protocol_checker = checkers_for_objc_protocol dec in
       let key = Ast_utils.generate_key_decl dec in

infer/src/clang/cFrontend_utils.ml

@@ -505,6 +505,14 @@ struct
         (return_type_matches_class_type omdi.omdi_result_type if_type_decl_pointer)
     | _ -> false
 
+  let name_of_decl_ref_opt (decl_ref_opt: Clang_ast_t.decl_ref option) =
+    match decl_ref_opt with
+    | Some decl_ref ->
+        (match decl_ref.dr_name with
+         | Some named_decl_info -> Some named_decl_info.ni_name
+         | _ -> None)
+    | _ -> None
+
 (*
   let rec getter_attribute_opt attributes =
     match attributes with

infer/src/clang/cFrontend_utils.mli

@@ -175,6 +175,8 @@ sig
 
   (** A class method that returns an instance of the class is a factory method. *)
   val is_objc_factory_method : Clang_ast_t.decl -> Clang_ast_t.decl -> bool
+
+  val name_of_decl_ref_opt : Clang_ast_t.decl_ref option -> string option
 end
 
 module General_utils :

infer/src/clang/cIssue.ml

@@ -11,6 +11,7 @@ type issue =
   | Assign_pointer_warning
   | Bad_pointer_comparison
   | Component_factory_function
+  | Component_initializer_with_side_effects
   | Component_with_multiple_factory_methods
   | Component_with_unconventional_superclass
   | Cxx_reference_captured_in_objc_block
@@ -29,6 +30,8 @@ let to_string issue =
          Localise.bad_pointer_comparison
      | Component_factory_function ->
          Localise.component_factory_function
+     | Component_initializer_with_side_effects ->
+         Localise.component_initializer_with_side_effects
      | Component_with_multiple_factory_methods ->
          Localise.component_with_multiple_factory_methods
      | Component_with_unconventional_superclass ->
@@ -57,6 +60,7 @@ let severity_of_issue issue =
   | Registered_observer_being_deallocated
   | Strong_delegate_warning -> Exceptions.Kwarning
   | Component_factory_function
+  | Component_initializer_with_side_effects
   | 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_initializer_with_side_effects
   | Component_with_multiple_factory_methods
   | Component_with_unconventional_superclass
   | Cxx_reference_captured_in_objc_block

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

@@ -0,0 +1,73 @@
+/*
+ * 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
+@end
+
+@implementation FooComponent
++ (instancetype)newDerp {
+  dispatch_async( // error
+      dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
+      ^{
+          // do something
+      });
+  dispatch_sync( // error
+      dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
+      ^{
+          // do something
+      });
+  dispatch_after( // error
+      DISPATCH_TIME_NOW,
+      dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
+      ^{
+          // do something
+      });
+  // make sure nested things are caught
+  ^{
+    dispatch_sync( // error
+        dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
+        ^{
+          ^{
+            dispatch_async( // error
+                dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
+                ^{
+                    // do something
+                });
+          }();
+        });
+  }();
+  return nil;
+}
+
+- (void)derp {
+  // not an error (only run on factory functions)
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
+                 ^{
+                     // do something
+                 });
+}
+@end
+
+@interface BarComponent : NSObject // not a real component
+@end
+
+@implementation BarComponent
++ (instancetype)newDerp {
+  // not an error (only run on actual components)
+  dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
+                 ^{
+                     // do something
+                 });
+  return nil;
+}
+@end

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

@@ -1,6 +1,11 @@
 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/InitializerWithSideEffectTest.mm, FooComponent_newDerp, 19, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS
+componentkit/InitializerWithSideEffectTest.mm, FooComponent_newDerp, 24, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS
+componentkit/InitializerWithSideEffectTest.mm, FooComponent_newDerp, 29, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS
+componentkit/InitializerWithSideEffectTest.mm, __objc_anonymous_block_______1, 37, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS
+componentkit/InitializerWithSideEffectTest.mm, __objc_anonymous_block_______2, 41, COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS
 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