diff --git a/infer/src/IR/Localise.ml b/infer/src/IR/Localise.ml index 48e1d458c..64a5f18d5 100644 --- a/infer/src/IR/Localise.ml +++ b/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" diff --git a/infer/src/IR/Localise.mli b/infer/src/IR/Localise.mli index 168515eec..02688c7ac 100644 --- a/infer/src/IR/Localise.mli +++ b/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 diff --git a/infer/src/backend/InferPrint.re b/infer/src/backend/InferPrint.re index 347b19e9d..dec31d9f0 100644 --- a/infer/src/backend/InferPrint.re +++ b/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, diff --git a/infer/src/clang/ComponentKit.ml b/infer/src/clang/ComponentKit.ml index ee065ed64..4bc4fe97c 100644 --- a/infer/src/clang/ComponentKit.ml +++ b/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 diff --git a/infer/src/clang/ComponentKit.mli b/infer/src/clang/ComponentKit.mli index 89119092c..69f32b1cb 100644 --- a/infer/src/clang/ComponentKit.mli +++ b/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 diff --git a/infer/src/clang/cFrontend_errors.ml b/infer/src/clang/cFrontend_errors.ml index 5cf0a1253..ccd854ce0 100644 --- a/infer/src/clang/cFrontend_errors.ml +++ b/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 *) diff --git a/infer/src/clang/cIssue.ml b/infer/src/clang/cIssue.ml index 13eb9b157..87d0885ea 100644 --- a/infer/src/clang/cIssue.ml +++ b/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 diff --git a/infer/src/clang/cIssue.mli b/infer/src/clang/cIssue.mli index 76cdf4620..2335bca8f 100644 --- a/infer/src/clang/cIssue.mli +++ b/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 diff --git a/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h b/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.h new file mode 100644 index 000000000..749be8150 --- /dev/null +++ b/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 + +#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 diff --git a/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.mm b/infer/tests/codetoanalyze/objcpp/linters/componentkit/MultipleFactoryMethodsTest.mm new file mode 100644 index 000000000..4e51690bb --- /dev/null +++ b/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 + +#import "MultipleFactoryMethodsTest.h" + +@implementation FooComponent +@end + +@implementation BarComponent +@end + +@implementation BazComponent +@end + +@implementation BadComponent +@end + +@implementation OKComponent +@end diff --git a/infer/tests/codetoanalyze/objcpp/linters/componentkit/UnconventionalSuperclassTest.h b/infer/tests/codetoanalyze/objcpp/linters/componentkit/UnconventionalSuperclassTest.h new file mode 100644 index 000000000..d4fcb9168 --- /dev/null +++ b/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 + +#import "FakeComponentKitHeader.h" + +@interface FooComponent : CKCompositeComponent // good +@end + +@interface BarComponent : FooComponent // bad +@end diff --git a/infer/tests/codetoanalyze/objcpp/linters/componentkit/UnconventionalSuperclassTest.mm b/infer/tests/codetoanalyze/objcpp/linters/componentkit/UnconventionalSuperclassTest.mm new file mode 100644 index 000000000..140455815 --- /dev/null +++ b/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 + +#import "UnconventionalSuperclassTest.h" + +@implementation FooComponent +@end + +@implementation BarComponent +@end diff --git a/infer/tests/codetoanalyze/objcpp/linters/issues.exp b/infer/tests/codetoanalyze/objcpp/linters/issues.exp index 72ca565bd..9cfe2f301 100644 --- a/infer/tests/codetoanalyze/objcpp/linters/issues.exp +++ b/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