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

@@ -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
+exception Frontend_warning of IssueType.t * Localise.error_desc * L.ocaml_pos
-  Frontend_warning of
-    (string * string option * string option * string option) * 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) ->
+  | Frontend_warning (issue_type, desc, ocaml_pos) ->
-      { name= IssueType.from_string name ?hum ?doc_url ?linters_def_file
+      { name= issue_type
       ; description= desc
       ; ocaml_pos= Some ocaml_pos
       ; visibility= Exn_user

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
+exception Frontend_warning of IssueType.t * Localise.error_desc * Logging.ocaml_pos
-  Frontend_warning of
-    (string * string option * string option * string option)
-    * Localise.error_desc
-    * Logging.ocaml_pos
 
 exception Inherently_dangerous_function of Localise.error_desc
 

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"

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

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"
+              { CIssue.issue_type= IssueType.mutable_local_variable_in_component_file
-              ; name= None
               ; 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"
+            { CIssue.issue_type= IssueType.component_factory_function
-            ; name= None
             ; 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"
+              { CIssue.issue_type= IssueType.component_with_unconventional_superclass
-              ; name= None
               ; 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"
+            { CIssue.issue_type= IssueType.component_with_multiple_factory_methods
-            ; name= None
             ; 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"
+              { CIssue.issue_type= IssueType.component_initializer_with_side_effects
-              ; name= None
               ; 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"
+          { CIssue.issue_type= IssueType.component_file_line_count
-          ; name= None
           ; 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"
+        { CIssue.issue_type= IssueType.component_file_cyclomatic_complexity
-        ; name= None
         ; severity= Exceptions.Info
         ; mode= CIssue.Off
         ; description= "Cyclomatic Complexity Incremental Marker"
         ; suggestion= None
-        ; doc_url= None
         ; loc }
   | _ ->
       None

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 <name> 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) =
+type parsed_issue_type =
-  match Config.get_linter_doc_url ~linter_id:linter.issue_desc.id with
+  { name: string option
-  | Some doc_url ->
+        (** issue name, if no name is given name will be a readable version of id, by removing underscores and capitalizing first letters of words *)
-      let issue_desc = {linter.issue_desc with doc_url= Some doc_url} in
+  ; doc_url: string option }
-      {linter with issue_desc}
-  | None ->
-      linter
-
 
 (** 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
+      { issue_type= {name= None; doc_url= None}
-      ; name= 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 issue_type =
-    let linter =
+      let doc_url =
-      {condition; issue_desc; def_file= Some linters_def_file; whitelist_paths; blacklist_paths}
+        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 =
+  let exn = Exceptions.Frontend_warning (issue_desc.issue_type, err_desc, __POS__) in
-    Exceptions.Frontend_warning
-      ((issue_desc.id, issue_desc.name, issue_desc.doc_url, linters_def_file), 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
+  try log_frontend_issue context.CLintersContext.current_method witness issue_desc'
-    log_frontend_issue context.CLintersContext.current_method witness issue_desc' linters_def_file
   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
+            fill_issue_desc_info_and_log context ~witness ~current_node:an linter.issue_desc loc )
-              linter.def_file loc )
     parsed_linters
 
 

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

infer/src/clang/cIssue.ml

@@ -9,32 +9,31 @@ open! IStd
 
 type mode = On | Off
 
-type issue_desc =
+type 'issue_type issue_desc0 =
-  { id: string
+  { issue_type: 'issue_type
-  ; (* issue id *)
+  ; (* issue type *)
     description: string
   ; (* Description in the error message *)
-    doc_url: string option
+    mode: mode
-  ; mode: mode
+  ; loc: Location.t
-  ; 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
   ; (* 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 "{@\n   Id = %s@\n" issue.issue_type.IssueType.unique_id ;
-  Format.fprintf fmt "{  Name = %s@\n" (Option.value ~default:"" issue.name) ;
+  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"
 

infer/src/clang/cIssue.mli

@@ -9,22 +9,20 @@ open! IStd
 
 type mode = On | Off
 
-type issue_desc =
+type 'issue_type issue_desc0 =
-  { id: string
+  { issue_type: 'issue_type
-  ; (* issue id *)
+  ; (* issue type *)
     description: string
   ; (* Description in the error message *)
-    doc_url: string option
+    mode: mode
-  ; mode: mode
+  ; loc: Location.t
-  ; 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
   ; (* 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

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
+    fill_issue_desc_info_and_log lcxt ~witness:an ~current_node:an linter.issue_desc loc
-      linter.def_file 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) ->