From ebb69313589ada0a2f894b4c8b2f5ca55ffd8500 Mon Sep 17 00:00:00 2001 From: Martino Luca Date: Thu, 15 Dec 2016 02:03:35 -0800 Subject: [PATCH] [CTL] Let issue name be a string for linters Summary: This change is to support the development of CTL's DSL, where issues can be specified directly from the language, in the form of strings. Severity is specified locally to the place where the check is defined Reviewed By: ddino Differential Revision: D4326594 fbshipit-source-id: 7b146ac --- infer/src/clang/ComponentKit.ml | 67 +++++++++++++------------ infer/src/clang/cFrontend_checkers.ml | 60 +++++++++++++---------- infer/src/clang/cFrontend_errors.ml | 5 +- infer/src/clang/cIssue.ml | 70 +-------------------------- infer/src/clang/cIssue.mli | 23 +-------- 5 files changed, 77 insertions(+), 148 deletions(-) diff --git a/infer/src/clang/ComponentKit.ml b/infer/src/clang/ComponentKit.ml index ac358e013..ce1b1ebf4 100644 --- a/infer/src/clang/ComponentKit.ml +++ b/infer/src/clang/ComponentKit.ml @@ -116,11 +116,12 @@ let mutable_local_vars_advice context an = && not decl_info.di_is_implicit in if condition then CTL.True, Some { - CIssue.issue = CIssue.Mutable_local_variable_in_component_file; - CIssue.description = "Local variable '" ^ named_decl_info.ni_name - ^ "' should be const to avoid reassignment"; - CIssue.suggestion = Some "Add a const (after the asterisk for pointer types)."; - CIssue.loc = CFrontend_checkers.location_from_dinfo context decl_info + CIssue.name = Localise.to_string Localise.mutable_local_variable_in_component_file; + severity = Exceptions.Kadvice; + description = "Local variable '" ^ named_decl_info.ni_name + ^ "' should be const to avoid reassignment"; + suggestion = Some "Add a const (after the asterisk for pointer types)."; + loc = CFrontend_checkers.location_from_dinfo context decl_info } else CTL.False, None | _ -> CTL.False, None (* Should only be called with a VarDecl *) @@ -142,13 +143,14 @@ let component_factory_function_advice context an = is_ck_context context an && is_component_if objc_interface in if condition then CTL.True, Some { - CIssue.issue = CIssue.Component_factory_function; - CIssue.description = "Break out composite components"; - CIssue.suggestion = Some ( + CIssue.name = Localise.to_string Localise.component_factory_function; + severity = Exceptions.Kadvice; + description = "Break out composite components"; + suggestion = Some ( "Prefer subclassing CKCompositeComponent to static helper functions \ that return a CKComponent subclass." ); - CIssue.loc = CFrontend_checkers.location_from_dinfo context decl_info + loc = CFrontend_checkers.location_from_dinfo context decl_info } else CTL.False, None | _ -> CTL.False, None (* Should only be called with FunctionDecl *) @@ -182,12 +184,13 @@ let component_with_unconventional_superclass_advice context an = && not has_conventional_superclass in if condition then CTL.True, Some { - CIssue.issue = CIssue.Component_with_unconventional_superclass; - CIssue.description = "Never Subclass Components"; - CIssue.suggestion = Some ( + CIssue.name = Localise.to_string Localise.component_with_unconventional_superclass; + severity = Exceptions.Kadvice; + description = "Never Subclass Components"; + suggestion = Some ( "Instead, create a new subclass of CKCompositeComponent." ); - CIssue.loc = CFrontend_checkers.location_from_decl context if_decl + loc = CFrontend_checkers.location_from_decl context if_decl } else CTL.False, None @@ -232,12 +235,13 @@ let component_with_multiple_factory_methods_advice context an = let factory_methods = IList.filter (is_available_factory_method if_decl) decls in if (IList.length factory_methods) > 1 then CTL.True, Some { - CIssue.issue = CIssue.Component_with_multiple_factory_methods; - CIssue.description = "Avoid Overrides"; - CIssue.suggestion = + CIssue.name = Localise.to_string Localise.component_with_multiple_factory_methods; + severity = Exceptions.Kadvice; + description = "Avoid Overrides"; + 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 + loc = CFrontend_checkers.location_from_dinfo context decl_info } else CTL.False, None @@ -285,11 +289,12 @@ let rec _component_initializer_with_side_effects_advice | Some "dispatch_async" | Some "dispatch_sync" -> CTL.True, 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 + CIssue.name = Localise.to_string Localise.component_initializer_with_side_effects; + severity = Exceptions.Kadvice; + description = "No Side-effects"; + suggestion = Some "Your +new method should not modify any \ + global variables or global state."; + loc = CFrontend_checkers.location_from_stmt context call_stmt } | _ -> CTL.False, None) @@ -317,10 +322,11 @@ let component_file_line_count_info (context: CLintersContext.context) dec = context.translation_unit_context.CFrontend_config.source_file in let line_count = SourceFile.line_count source_file in IList.map (fun i -> { - CIssue.issue = CIssue.Component_file_line_count; - CIssue.description = "Line count analytics"; - CIssue.suggestion = None; - CIssue.loc = { + CIssue.name = Localise.to_string Localise.component_file_line_count; + severity = Exceptions.Kinfo; + description = "Line count analytics"; + suggestion = None; + loc = { Location.line = i; Location.col = 0; Location.file = source_file @@ -361,9 +367,10 @@ let component_file_cyclomatic_complexity_info (context: CLintersContext.context) match cyclo_loc_opt an with | Some loc -> CTL.True, Some { - CIssue.issue = CIssue.Component_file_cyclomatic_complexity; - CIssue.description = "Cyclomatic Complexity Incremental Marker"; - CIssue.suggestion = None; - CIssue.loc = loc + CIssue.name = Localise.to_string Localise.component_file_cyclomatic_complexity; + severity = Exceptions.Kinfo; + description = "Cyclomatic Complexity Incremental Marker"; + suggestion = None; + loc = loc } | _ -> CTL.False, None diff --git a/infer/src/clang/cFrontend_checkers.ml b/infer/src/clang/cFrontend_checkers.ml index 5ae673be0..da3db313c 100644 --- a/infer/src/clang/cFrontend_checkers.ml +++ b/infer/src/clang/cFrontend_checkers.ml @@ -119,12 +119,13 @@ let ctl_ns_notification_warning lctx an = let condition = InNode (["ObjCImplementationDecl"; "ObjCProtocolDecl"], Not (Implies (eventually_addObserver, eventually_removeObserver))) in let issue_desc = { - CIssue.issue = CIssue.Registered_observer_being_deallocated; - CIssue.description = + CIssue.name = Localise.to_string Localise.registered_observer_being_deallocated; + severity = Exceptions.Kwarning; + description = "Object self is registered in a notification center but not being removed before deallocation"; - CIssue.suggestion = + suggestion = Some "Consider removing the object from the notification center before its deallocation."; - CIssue.loc = location_from_an lctx an; + loc = location_from_an lctx an; } in condition, Some issue_desc @@ -152,7 +153,8 @@ let ctl_bad_pointer_comparison_warning lctx an = let condition = InNode (["IfStmt"; "ForStmt"; "WhileStmt"; "ConditionalOperator"], etx) in let issue_desc = { CIssue. - issue = CIssue.Bad_pointer_comparison; + name = Localise.to_string Localise.bad_pointer_comparison; + severity = Exceptions.Kwarning; description = "Implicitly checking whether NSNumber pointer is nil"; suggestion = Some ("Did you mean to compare against the unboxed value instead? " ^ @@ -175,11 +177,12 @@ let ctl_strong_delegate_warning lctx an = And (name_does_not_contains_queue, is_strong_property))) in let issue_desc = { - CIssue.issue = CIssue.Strong_delegate_warning; - CIssue.description = + CIssue.name = Localise.to_string Localise.strong_delegate_warning; + severity = Exceptions.Kwarning; + description = "Property or ivar %decl_name% declared strong"; - CIssue.suggestion = Some "In general delegates should be declared weak or assign"; - CIssue.loc = location_from_an lctx an + suggestion = Some "In general delegates should be declared weak or assign"; + loc = location_from_an lctx an } in condition, Some issue_desc @@ -196,12 +199,14 @@ let ctl_global_var_init_with_calls_warning lctx an = let condition = InNode (["VarDecl"], And (ctl_is_global_var, ctl_is_initialized_with_expensive_call)) in let issue_desc = { - CIssue.issue = CIssue.Global_variable_initialized_with_function_or_method_call; - CIssue.description = + CIssue.name = + Localise.to_string Localise.global_variable_initialized_with_function_or_method_call; + severity = Exceptions.Kwarning; + description = "Global variable %decl_name% is initialized using a function or method call"; - CIssue.suggestion = Some + suggestion = Some "If the function/method call is expensive, it can affect the starting time of the app."; - CIssue.loc = location_from_an lctx an + loc = location_from_an lctx an } in condition, Some issue_desc @@ -212,11 +217,12 @@ let ctl_assign_pointer_warning lctx an = And (Atomic ("is_assign_property", []), Atomic ("is_property_pointer_type", []))) in let issue_desc = - { CIssue.issue = CIssue.Assign_pointer_warning; - CIssue.description = + { CIssue.name = Localise.to_string Localise.assign_pointer_warning; + severity = Exceptions.Kwarning; + description = "Property `%decl_name%` is a pointer type marked with the `assign` attribute"; - CIssue.suggestion = Some "Use a different attribute like `strong` or `weak`."; - CIssue.loc = location_from_an lctx an + suggestion = Some "Use a different attribute like `strong` or `weak`."; + loc = location_from_an lctx an } in condition, Some issue_desc @@ -233,11 +239,12 @@ let ctl_direct_atomic_property_access_warning lctx an = Not (Atomic ("is_objc_constructor", []))), Not (Atomic ("is_objc_dealloc", [])))) in let issue_desc = { - CIssue.issue = CIssue.Direct_atomic_property_access; - CIssue.description = "Direct access to ivar %ivar_name% of an atomic property"; - CIssue.suggestion = + CIssue.name = Localise.to_string Localise.direct_atomic_property_access; + severity = Exceptions.Kwarning; + description = "Direct access to ivar %ivar_name% of an atomic property"; + suggestion = Some "Accessing an ivar of an atomic property makes the property nonatomic"; - CIssue.loc = location_from_an lctx an + loc = location_from_an lctx an } in condition, Some issue_desc @@ -246,12 +253,13 @@ let ctl_captured_cxx_ref_in_objc_block_warning lctx an = let open CTL in let condition = InNode (["BlockDecl"], Atomic ("captures_cxx_references", [])) in let issue_desc = { - CIssue.issue = CIssue.Cxx_reference_captured_in_objc_block; - CIssue.description = + CIssue.name = Localise.to_string Localise.cxx_reference_captured_in_objc_block; + severity = Exceptions.Kwarning; + description = "C++ Reference variable(s) %var_name% captured by Objective-C block"; - CIssue.suggestion = Some ("C++ References are unmanaged and may be invalid " ^ - "by the time the block executes."); - CIssue.loc = match an with + suggestion = Some ("C++ References are unmanaged and may be invalid " ^ + "by the time the block executes."); + loc = match an with | Stmt (Clang_ast_t.BlockExpr (_, _ , _, decl)) -> location_from_an lctx (Decl decl) | _ -> location_from_an lctx an; } in diff --git a/infer/src/clang/cFrontend_errors.ml b/infer/src/clang/cFrontend_errors.ml index b82e26823..aa5a1f917 100644 --- a/infer/src/clang/cFrontend_errors.ml +++ b/infer/src/clang/cFrontend_errors.ml @@ -120,15 +120,14 @@ let get_err_log translation_unit_context method_decl_opt = (* Add a frontend warning with a description desc at location loc to the errlog of a proc desc *) let log_frontend_issue translation_unit_context method_decl_opt key issue_desc = - let issue = issue_desc.CIssue.issue in + let name = issue_desc.CIssue.name in let loc = issue_desc.CIssue.loc in let errlog = get_err_log translation_unit_context method_decl_opt in let err_desc = Errdesc.explain_frontend_warning issue_desc.CIssue.description issue_desc.CIssue.suggestion loc in - let name = CIssue.to_string issue in let exn = Exceptions.Frontend_warning (name, err_desc, __POS__) in let trace = [ Errlog.make_trace_element 0 issue_desc.CIssue.loc "" [] ] in - let err_kind = CIssue.severity_of_issue issue in + let err_kind = issue_desc.CIssue.severity in let method_name = Ast_utils.full_name_of_decl_opt method_decl_opt in let key = Hashtbl.hash (key ^ method_name) in Reporting.log_issue_from_errlog err_kind errlog exn ~loc ~ltr:trace diff --git a/infer/src/clang/cIssue.ml b/infer/src/clang/cIssue.ml index acba40486..1b2db4fc7 100644 --- a/infer/src/clang/cIssue.ml +++ b/infer/src/clang/cIssue.ml @@ -9,75 +9,9 @@ open! IStd -type issue = - | Assign_pointer_warning - | Bad_pointer_comparison - | Component_factory_function - | Component_file_cyclomatic_complexity - | Component_file_line_count - | Component_initializer_with_side_effects - | Component_with_multiple_factory_methods - | Component_with_unconventional_superclass - | Cxx_reference_captured_in_objc_block - | Direct_atomic_property_access - | Global_variable_initialized_with_function_or_method_call - | Mutable_local_variable_in_component_file - | Registered_observer_being_deallocated - | Strong_delegate_warning - -let to_string issue = - Localise.to_string - (match issue with - | Assign_pointer_warning -> - Localise.assign_pointer_warning - | Bad_pointer_comparison -> - Localise.bad_pointer_comparison - | Component_factory_function -> - Localise.component_factory_function - | Component_file_cyclomatic_complexity -> - Localise.component_file_cyclomatic_complexity - | Component_file_line_count -> - Localise.component_file_line_count - | 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 -> - Localise.component_with_unconventional_superclass - | Cxx_reference_captured_in_objc_block -> - Localise.cxx_reference_captured_in_objc_block - | Direct_atomic_property_access -> - Localise.direct_atomic_property_access - | Global_variable_initialized_with_function_or_method_call -> - Localise.global_variable_initialized_with_function_or_method_call - | Mutable_local_variable_in_component_file -> - Localise.mutable_local_variable_in_component_file - | Registered_observer_being_deallocated -> - Localise.registered_observer_being_deallocated - | Strong_delegate_warning -> - Localise.strong_delegate_warning - ) - -let severity_of_issue issue = - match issue with - | Assign_pointer_warning - | Bad_pointer_comparison - | Cxx_reference_captured_in_objc_block - | Direct_atomic_property_access - | Global_variable_initialized_with_function_or_method_call - | 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 - | Component_file_cyclomatic_complexity - | Component_file_line_count -> Exceptions.Kinfo - - type issue_desc = { - issue : issue; (* issue *) + name : string; (* issue name *) + severity: Exceptions.err_kind; description : string; (* Description in the error message *) suggestion : string option; (* an optional suggestion or correction *) loc : Location.t; (* location in the code *) diff --git a/infer/src/clang/cIssue.mli b/infer/src/clang/cIssue.mli index d7bab784e..34dd7d668 100644 --- a/infer/src/clang/cIssue.mli +++ b/infer/src/clang/cIssue.mli @@ -9,28 +9,9 @@ open! IStd -type issue = - | Assign_pointer_warning - | Bad_pointer_comparison - | Component_factory_function - | Component_file_cyclomatic_complexity - | Component_file_line_count - | Component_initializer_with_side_effects - | Component_with_multiple_factory_methods - | Component_with_unconventional_superclass - | Cxx_reference_captured_in_objc_block - | Direct_atomic_property_access - | Global_variable_initialized_with_function_or_method_call - | Mutable_local_variable_in_component_file - | Registered_observer_being_deallocated - | Strong_delegate_warning - -val to_string : issue -> string - -val severity_of_issue : issue -> Exceptions.err_kind - type issue_desc = { - issue : issue; (* issue *) + name : string; (* issue name *) + severity : Exceptions.err_kind; description : string; (* Description in the error message *) suggestion : string option; (* an optional suggestion or correction *) loc : Location.t; (* location in the code *)