From 7964dd76ca15e66b095dcea485507b1eec95af2a Mon Sep 17 00:00:00 2001 From: Mehdi Bouaziz Date: Mon, 10 Sep 2018 05:19:12 -0700 Subject: [PATCH] Reporting cleanup 19: use IssueType for CIssue.issue_desc Summary: - Let's call `IssueType.from_string` once only - Use properly defined issue types for builtin linters Reviewed By: martinoluca Differential Revision: D9654105 fbshipit-source-id: 947b50a51 --- infer/src/IR/Exceptions.ml | 8 ++-- infer/src/IR/Exceptions.mli | 6 +-- infer/src/base/IssueType.ml | 18 ++++++++ infer/src/base/IssueType.mli | 14 ++++++ infer/src/clang/ComponentKit.ml | 28 +++--------- infer/src/clang/cFrontend_errors.ml | 66 +++++++++++++++------------- infer/src/clang/cFrontend_errors.mli | 1 - infer/src/clang/cIssue.ml | 23 +++++----- infer/src/clang/cIssue.mli | 16 +++---- infer/src/clang/tableaux.ml | 11 +++-- 10 files changed, 104 insertions(+), 87 deletions(-) diff --git a/infer/src/IR/Exceptions.ml b/infer/src/IR/Exceptions.ml index b25801cd0..be242beb1 100644 --- a/infer/src/IR/Exceptions.ml +++ b/infer/src/IR/Exceptions.ml @@ -78,9 +78,7 @@ exception Eradicate of IssueType.t * Localise.error_desc exception Field_not_null_checked of Localise.error_desc * L.ocaml_pos -exception - Frontend_warning of - (string * string option * string option * string option) * Localise.error_desc * L.ocaml_pos +exception Frontend_warning of IssueType.t * Localise.error_desc * L.ocaml_pos exception Checkers of IssueType.t * Localise.error_desc @@ -322,8 +320,8 @@ let recognize_exception exn = ; visibility= Exn_user ; severity= Some Warning ; category= Nocat } - | Frontend_warning ((name, hum, doc_url, linters_def_file), desc, ocaml_pos) -> - { name= IssueType.from_string name ?hum ?doc_url ?linters_def_file + | Frontend_warning (issue_type, desc, ocaml_pos) -> + { name= issue_type ; description= desc ; ocaml_pos= Some ocaml_pos ; visibility= Exn_user diff --git a/infer/src/IR/Exceptions.mli b/infer/src/IR/Exceptions.mli index ec9b9311c..dc180bc83 100644 --- a/infer/src/IR/Exceptions.mli +++ b/infer/src/IR/Exceptions.mli @@ -79,11 +79,7 @@ exception Eradicate of IssueType.t * Localise.error_desc exception Checkers of IssueType.t * Localise.error_desc -exception - Frontend_warning of - (string * string option * string option * string option) - * Localise.error_desc - * Logging.ocaml_pos +exception Frontend_warning of IssueType.t * Localise.error_desc * Logging.ocaml_pos exception Inherently_dangerous_function of Localise.error_desc diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index aae7792a8..705f33b0a 100644 --- a/infer/src/base/IssueType.ml +++ b/infer/src/base/IssueType.ml @@ -140,6 +140,20 @@ let codequery = from_string "Codequery" let comparing_floats_for_equality = from_string "COMPARING_FLOAT_FOR_EQUALITY" +let component_factory_function = from_string "COMPONENT_FACTORY_FUNCTION" + +let component_file_cyclomatic_complexity = from_string "COMPONENT_FILE_CYCLOMATIC_COMPLEXITY" + +let component_file_line_count = from_string "COMPONENT_FILE_LINE_COUNT" + +let component_initializer_with_side_effects = from_string "COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS" + +let component_with_multiple_factory_methods = from_string "COMPONENT_WITH_MULTIPLE_FACTORY_METHODS" + +let component_with_unconventional_superclass = + from_string "COMPONENT_WITH_UNCONVENTIONAL_SUPERCLASS" + + let condition_always_false = from_string ~enabled:false "CONDITION_ALWAYS_FALSE" let condition_always_true = from_string ~enabled:false "CONDITION_ALWAYS_TRUE" @@ -285,6 +299,10 @@ let missing_fld = from_string "Missing_fld" ~hum:"Missing Field" let missing_required_prop = from_string "MISSING_REQUIRED_PROP" +let mutable_local_variable_in_component_file = + from_string "MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE" + + let null_dereference = from_string "NULL_DEREFERENCE" let null_test_after_dereference = from_string ~enabled:false "NULL_TEST_AFTER_DEREFERENCE" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index 37e62ce5a..2645f7307 100644 --- a/infer/src/base/IssueType.mli +++ b/infer/src/base/IssueType.mli @@ -88,6 +88,18 @@ val codequery : t val comparing_floats_for_equality : t +val component_factory_function : t + +val component_file_cyclomatic_complexity : t + +val component_file_line_count : t + +val component_initializer_with_side_effects : t + +val component_with_multiple_factory_methods : t + +val component_with_unconventional_superclass : t + val condition_always_false : t val condition_always_true : t @@ -195,6 +207,8 @@ val missing_fld : t val missing_required_prop : t +val mutable_local_variable_in_component_file : t + val null_dereference : t val null_test_after_dereference : t diff --git a/infer/src/clang/ComponentKit.ml b/infer/src/clang/ComponentKit.ml index 17429229d..295eb1b03 100644 --- a/infer/src/clang/ComponentKit.ml +++ b/infer/src/clang/ComponentKit.ml @@ -150,8 +150,7 @@ let mutable_local_vars_advice context an = if should_not_report_mutable_local then None else Some - { CIssue.id= "MUTABLE_LOCAL_VARIABLE_IN_COMPONENT_FILE" - ; name= None + { CIssue.issue_type= IssueType.mutable_local_variable_in_component_file ; severity= Exceptions.Advice ; mode= CIssue.On ; description= @@ -159,7 +158,6 @@ let mutable_local_vars_advice context an = ^ MF.monospaced_to_string named_decl_info.ni_name ^ " should be const to avoid reassignment" ; suggestion= Some "Add a const (after the asterisk for pointer types)." - ; doc_url= None ; loc= CFrontend_checkers.location_from_dinfo context decl_info } | _ -> None @@ -188,8 +186,7 @@ let component_factory_function_advice context an = let objc_interface = CAst_utils.qual_type_to_objc_interface qual_type in if is_component_if objc_interface then Some - { CIssue.id= "COMPONENT_FACTORY_FUNCTION" - ; name= None + { CIssue.issue_type= IssueType.component_factory_function ; severity= Exceptions.Advice ; mode= CIssue.Off ; description= "Break out composite components" @@ -197,7 +194,6 @@ let component_factory_function_advice context an = Some "Prefer subclassing CKCompositeComponent to static helper functions that return \ a CKComponent subclass." - ; doc_url= None ; loc= CFrontend_checkers.location_from_dinfo context decl_info } else None | _ -> @@ -243,13 +239,11 @@ let component_with_unconventional_superclass_advice context an = in if condition then Some - { CIssue.id= "COMPONENT_WITH_UNCONVENTIONAL_SUPERCLASS" - ; name= None + { CIssue.issue_type= IssueType.component_with_unconventional_superclass ; severity= Exceptions.Advice ; mode= CIssue.On ; description= "Never Subclass Components" ; suggestion= Some "Instead, create a new subclass of CKCompositeComponent." - ; doc_url= None ; loc= CFrontend_checkers.location_from_decl context if_decl } else None else None @@ -303,8 +297,7 @@ let component_with_multiple_factory_methods_advice context an = let factory_methods = List.filter ~f:(is_available_factory_method (Some if_decl)) decls in List.map ~f:(fun meth_decl -> - { CIssue.id= "COMPONENT_WITH_MULTIPLE_FACTORY_METHODS" - ; name= None + { CIssue.issue_type= IssueType.component_with_multiple_factory_methods ; severity= Exceptions.Advice ; mode= CIssue.On ; description= "Avoid Overrides" @@ -312,7 +305,6 @@ let component_with_multiple_factory_methods_advice context an = Some "Instead, always expose all parameters in a single designated initializer and \ document which are optional." - ; doc_url= None ; loc= CFrontend_checkers.location_from_decl context meth_decl } ) (IList.drop factory_methods 1) | _ -> @@ -383,14 +375,12 @@ let rec component_initializer_with_side_effects_advice_ (context : CLintersConte match List.find_map ~f:CAst_utils.name_of_decl_ref_opt refs with | Some "dispatch_after" | Some "dispatch_async" | Some "dispatch_sync" -> Some - { CIssue.id= "COMPONENT_INITIALIZER_WITH_SIDE_EFFECTS" - ; name= None + { CIssue.issue_type= IssueType.component_initializer_with_side_effects ; severity= Exceptions.Advice ; mode= CIssue.On ; description= "No Side-effects" ; suggestion= Some "Your +new method should not modify any global variables or global state." - ; doc_url= None ; loc= CFrontend_checkers.location_from_stmt context call_stmt } | _ -> None ) @@ -421,13 +411,11 @@ let component_file_line_count_info (context : CLintersContext.context) dec = let line_count = SourceFile.line_count source_file in List.map ~f:(fun i -> - { CIssue.id= "COMPONENT_FILE_LINE_COUNT" - ; name= None + { CIssue.issue_type= IssueType.component_file_line_count ; severity= Exceptions.Info ; mode= CIssue.Off ; description= "Line count analytics" ; suggestion= None - ; doc_url= None ; loc= {Location.line= i; Location.col= 0; Location.file= source_file} } ) (List.range 1 line_count ~start:`inclusive ~stop:`inclusive) | _ -> @@ -472,13 +460,11 @@ let component_file_cyclomatic_complexity_info (context : CLintersContext.context match cyclo_loc_opt an with | Some loc -> Some - { CIssue.id= "COMPONENT_FILE_CYCLOMATIC_COMPLEXITY" - ; name= None + { CIssue.issue_type= IssueType.component_file_cyclomatic_complexity ; severity= Exceptions.Info ; mode= CIssue.Off ; description= "Cyclomatic Complexity Incremental Marker" ; suggestion= None - ; doc_url= None ; loc } | _ -> None diff --git a/infer/src/clang/cFrontend_errors.ml b/infer/src/clang/cFrontend_errors.ml index 13923d466..e56620eca 100644 --- a/infer/src/clang/cFrontend_errors.ml +++ b/infer/src/clang/cFrontend_errors.ml @@ -27,7 +27,10 @@ let filter_parsed_linters_developer parsed_linters = important for debugging the rule. Pass the flag --linter to specify the linter \ you want to debug." | Some lint -> - List.filter ~f:(fun (rule : linter) -> String.equal rule.issue_desc.id lint) parsed_linters + List.filter + ~f:(fun (rule : linter) -> + String.equal rule.issue_desc.issue_type.IssueType.unique_id lint ) + parsed_linters else parsed_linters @@ -57,7 +60,9 @@ let filter_parsed_linters parsed_linters source_file = let pp_linters fmt linters = - let pp_linter fmt {issue_desc= {id}} = F.fprintf fmt "%s@\n" id in + let pp_linter fmt {issue_desc= {issue_type= {IssueType.unique_id}}} = + F.fprintf fmt "%s@\n" unique_id + in List.iter ~f:(pp_linter fmt) linters @@ -179,14 +184,10 @@ let string_to_issue_mode m = L.die InternalError "Mode %s does not exist. Please specify ON/OFF" s -let post_process_linter_definition (linter : linter) = - match Config.get_linter_doc_url ~linter_id:linter.issue_desc.id with - | Some doc_url -> - let issue_desc = {linter.issue_desc with doc_url= Some doc_url} in - {linter with issue_desc} - | None -> - linter - +type parsed_issue_type = + { name: string option + (** issue name, if no name is given name will be a readable version of id, by removing underscores and capitalizing first letters of words *) + ; doc_url: string option } (** Convert a parsed checker in list of linters *) let create_parsed_linters linters_def_file checkers : linter list = @@ -195,13 +196,11 @@ let create_parsed_linters linters_def_file checkers : linter list = L.(debug Linters Medium) "@\nConverting checkers in (condition, issue) pairs@\n" ; let do_one_checker checker : linter = let dummy_issue = - { id= checker.id - ; name= None + { issue_type= {name= None; doc_url= None} ; description= "" ; suggestion= None ; loc= Location.dummy ; severity= Exceptions.Warning - ; doc_url= None ; mode= CIssue.On } in let issue_desc, condition, whitelist_paths, blacklist_paths = @@ -218,9 +217,15 @@ let create_parsed_linters linters_def_file checkers : linter list = | CDesc (av, m) when ALVar.is_mode_keyword av -> ({issue with mode= string_to_issue_mode m}, cond, wl_paths, bl_paths) | CDesc (av, doc) when ALVar.is_doc_url_keyword av -> - ({issue with doc_url= Some doc}, cond, wl_paths, bl_paths) + ( {issue with issue_type= {issue.issue_type with doc_url= Some doc}} + , cond + , wl_paths + , bl_paths ) | CDesc (av, name) when ALVar.is_name_keyword av -> - ({issue with name= Some name}, cond, wl_paths, bl_paths) + ( {issue with issue_type= {issue.issue_type with name= Some name}} + , cond + , wl_paths + , bl_paths ) | CPath (`WhitelistPath, paths) -> (issue, cond, paths, bl_paths) | CPath (`BlacklistPath, paths) -> @@ -233,11 +238,17 @@ let create_parsed_linters linters_def_file checkers : linter list = in L.(debug Linters Medium) "@\nMaking condition and issue desc for checker '%s'@\n" checker.id ; L.(debug Linters Medium) "@\nCondition =@\n %a@\n" CTL.Debug.pp_formula condition ; - L.(debug Linters Medium) "@\nIssue_desc = %a@\n" CIssue.pp_issue issue_desc ; - let linter = - {condition; issue_desc; def_file= Some linters_def_file; whitelist_paths; blacklist_paths} + let issue_type = + let doc_url = + Option.first_some + (Config.get_linter_doc_url ~linter_id:checker.id) + issue_desc.issue_type.doc_url + in + IssueType.from_string checker.id ?hum:issue_desc.issue_type.name ?doc_url ~linters_def_file in - post_process_linter_definition linter + let issue_desc = {issue_desc with issue_type} in + L.(debug Linters Medium) "@\nIssue_desc = %a@\n" CIssue.pp_issue issue_desc ; + {condition; issue_desc; def_file= Some linters_def_file; whitelist_paths; blacklist_paths} in List.map ~f:do_one_checker checkers @@ -428,7 +439,7 @@ let expand_checkers macro_map path_map checkers = (** Add a frontend warning with a description desc at location loc to the errlog of a proc desc *) let log_frontend_issue method_decl_opt (node : Ctl_parser_types.ast_node) - (issue_desc : CIssue.issue_desc) linters_def_file = + (issue_desc : CIssue.issue_desc) = let procname = match method_decl_opt with | Some method_decl -> @@ -440,10 +451,7 @@ let log_frontend_issue method_decl_opt (node : Ctl_parser_types.ast_node) let err_desc = Errdesc.explain_frontend_warning issue_desc.description issue_desc.suggestion issue_desc.loc in - let exn = - Exceptions.Frontend_warning - ((issue_desc.id, issue_desc.name, issue_desc.doc_url, linters_def_file), err_desc, __POS__) - in + let exn = Exceptions.Frontend_warning (issue_desc.issue_type, err_desc, __POS__) in let trace = [Errlog.make_trace_element 0 issue_desc.loc "" []] in let key_str = match node with @@ -458,15 +466,14 @@ let log_frontend_issue method_decl_opt (node : Ctl_parser_types.ast_node) let fill_issue_desc_info_and_log context ~witness ~current_node (issue_desc : CIssue.issue_desc) - linters_def_file loc = + loc = let process_message message = remove_new_lines_and_whitespace (expand_message_string context message current_node) in let description = process_message issue_desc.description in let suggestion = Option.map ~f:process_message issue_desc.suggestion in let issue_desc' = {issue_desc with description; loc; suggestion} in - try - log_frontend_issue context.CLintersContext.current_method witness issue_desc' linters_def_file + try log_frontend_issue context.CLintersContext.current_method witness issue_desc' with CFrontend_config.IncorrectAssumption e -> let trans_unit_ctx = context.CLintersContext.translation_unit_context in ClangLogging.log_caught_exception trans_unit_ctx "IncorrectAssumption" e.position @@ -482,7 +489,7 @@ let invoke_set_of_hard_coded_checkers_an context (an : Ctl_parser_types.ast_node List.iter ~f:(fun issue_desc -> if CIssue.should_run_check issue_desc.CIssue.mode then - fill_issue_desc_info_and_log context ~witness:an ~current_node:an issue_desc None + fill_issue_desc_info_and_log context ~witness:an ~current_node:an issue_desc issue_desc.CIssue.loc ) issue_desc_list ) checkers @@ -498,8 +505,7 @@ let invoke_set_of_parsed_checkers_an parsed_linters context (an : Ctl_parser_typ () | Some witness -> let loc = CFrontend_checkers.location_from_an context witness in - fill_issue_desc_info_and_log context ~witness ~current_node:an linter.issue_desc - linter.def_file loc ) + fill_issue_desc_info_and_log context ~witness ~current_node:an linter.issue_desc loc ) parsed_linters diff --git a/infer/src/clang/cFrontend_errors.mli b/infer/src/clang/cFrontend_errors.mli index 9446743ba..7d8a39021 100644 --- a/infer/src/clang/cFrontend_errors.mli +++ b/infer/src/clang/cFrontend_errors.mli @@ -49,6 +49,5 @@ val fill_issue_desc_info_and_log : -> witness:Ctl_parser_types.ast_node -> current_node:Ctl_parser_types.ast_node -> CIssue.issue_desc - -> string option -> Location.t -> unit diff --git a/infer/src/clang/cIssue.ml b/infer/src/clang/cIssue.ml index 2dfe8d803..eef652c10 100644 --- a/infer/src/clang/cIssue.ml +++ b/infer/src/clang/cIssue.ml @@ -9,32 +9,31 @@ open! IStd type mode = On | Off -type issue_desc = - { id: string - ; (* issue id *) +type 'issue_type issue_desc0 = + { issue_type: 'issue_type + ; (* issue type *) description: string ; (* Description in the error message *) - doc_url: string option - ; mode: mode - ; name: string option - ; (* issue name, if no name is given name will be a readable version of id, - by removing underscores and capitalizing first letters of words *) - loc: Location.t + mode: mode + ; loc: Location.t ; (* location in the code *) severity: Exceptions.severity ; suggestion: string option (* an optional suggestion or correction *) } +type issue_desc = IssueType.t issue_desc0 + let string_of_mode m = match m with On -> "On" | Off -> "Off" let pp_issue fmt issue = - Format.fprintf fmt "{@\n Id = %s@\n" issue.id ; - Format.fprintf fmt "{ Name = %s@\n" (Option.value ~default:"" issue.name) ; + Format.fprintf fmt "{@\n Id = %s@\n" issue.issue_type.IssueType.unique_id ; + Format.fprintf fmt "{ Name = %s@\n" issue.issue_type.IssueType.hum ; Format.fprintf fmt " Severity = %s@\n" (Exceptions.severity_string issue.severity) ; Format.fprintf fmt " Mode = %s@\n" (string_of_mode issue.mode) ; Format.fprintf fmt " Description = %s@\n" issue.description ; Format.fprintf fmt " Suggestion = %s@\n" (Option.value ~default:"" issue.suggestion) ; - Format.fprintf fmt " Docs URL = %s@\n" (Option.value ~default:"" issue.doc_url) ; + Format.fprintf fmt " Docs URL = %s@\n" + (Option.value ~default:"" issue.issue_type.IssueType.doc_url) ; Format.fprintf fmt " Loc = %s@\n" (Location.to_string issue.loc) ; Format.fprintf fmt "}@\n" diff --git a/infer/src/clang/cIssue.mli b/infer/src/clang/cIssue.mli index 2364a2572..608729c42 100644 --- a/infer/src/clang/cIssue.mli +++ b/infer/src/clang/cIssue.mli @@ -9,22 +9,20 @@ open! IStd type mode = On | Off -type issue_desc = - { id: string - ; (* issue id *) +type 'issue_type issue_desc0 = + { issue_type: 'issue_type + ; (* issue type *) description: string ; (* Description in the error message *) - doc_url: string option - ; mode: mode - ; name: string option - ; (* issue name, if no name is given name will be a readable version of id, - by removing underscores and capitalizing first letters of words *) - loc: Location.t + mode: mode + ; loc: Location.t ; (* location in the code *) severity: Exceptions.severity ; suggestion: string option (* an optional suggestion or correction *) } +type issue_desc = IssueType.t issue_desc0 + val pp_issue : Format.formatter -> issue_desc -> unit val should_run_check : mode -> bool diff --git a/infer/src/clang/tableaux.ml b/infer/src/clang/tableaux.ml index 37fa8c7bd..be62ba470 100644 --- a/infer/src/clang/tableaux.ml +++ b/infer/src/clang/tableaux.ml @@ -295,8 +295,7 @@ let report_issue an lcxt linter (*npo_condition*) = let loc = CFrontend_checkers.location_from_an lcxt an in let should_report = match an with Decl dec -> is_decl_allowed lcxt dec | Stmt _ -> true in if should_report then - fill_issue_desc_info_and_log lcxt ~witness:an ~current_node:an linter.issue_desc - linter.def_file loc + fill_issue_desc_info_and_log lcxt ~witness:an ~current_node:an linter.issue_desc loc let check_linter_map linter_map_contex phi = @@ -332,10 +331,14 @@ let build_valuation parsed_linters an lcxt linter_map_context = (is_state_only, cl') in if not (is_state_only && skip_evaluation_InNode_formula an normalized_condition) then ( - let sat_set = add_valid_formulae an linter.issue_desc.id lcxt cl in + let sat_set = + add_valid_formulae an linter.issue_desc.issue_type.IssueType.unique_id lcxt cl + in (*L.progress " [Set Size: %i] @\n" (CTLFormulaSet.cardinal sat_set);*) if CTLFormulaSet.mem normalized_condition sat_set then report_issue an lcxt linter ; - add_formula_to_valuation (node_pointer, linter.issue_desc.id) sat_set ) + add_formula_to_valuation + (node_pointer, linter.issue_desc.issue_type.IssueType.unique_id) + sat_set ) in List.iter ~f:(fun (linter : linter) ->