[DRAFT] [backend] analyze procedures defined in changed headers

.gitignore

@@ -36,6 +36,7 @@ duplicates.txt
 /infer/tests/build_systems/buck_clang_test_determinator/diff.mod.test
 /infer/tests/build_systems/buck_export_changed_functions/diff.mod.test
 /infer/tests/build_systems/clang_compilation_db_escaped/compile_commands.json
+/infer/tests/build_systems/clang_compilation_db_headers/compile_commands.json
 /infer/tests/build_systems/clang_compilation_db_relpath/compile_commands.json
 /infer/tests/build_systems/clang_skip_analysis_in_path/compile_commands.json
 /infer/tests/build_systems/clang_test_determinator/*.test

Makefile

@@ -14,7 +14,7 @@ MAKE_SOURCE = $(MAKE) -C $(SRC_DIR)
 ifeq ($(BUILD_C_ANALYZERS),yes)
 BUILD_SYSTEMS_TESTS += \
   backtrack_level \
-  clang_compilation_db_escaped clang_compilation_db_relpath \
+  clang_compilation_db_escaped clang_compilation_db_headers clang_compilation_db_relpath \
   clang_multiple_files \
   clang_skip_analysis_in_path \
   clang_translation \
@@ -25,6 +25,7 @@ BUILD_SYSTEMS_TESTS += \
   clang_with_MD_flag \
   deduplicate_template_warnings \
   delete_results_dir \
+  differential_changed_header \
   disjunctive_domain \
   duplicate_symbols \
   extract_capture \
@@ -45,6 +46,7 @@ BUILD_SYSTEMS_TESTS += \
   tracebugs \
   utf8_in_procname \
   incremental_analysis_add_procedure \
+  incremental_analysis_change_header \
   incremental_analysis_change_procedure \
   incremental_analysis_change_tenv \
   incremental_analysis_file_level_change \

infer/man/man1/infer-analyze.txt

@@ -41,7 +41,16 @@ OPTIONS
        --changed-files-index file
            Specify the file containing the list of source files from which
            reactive analysis should start. Source files should be specified
-           relative to project root or be absolute
+           relative to project root or be absolute. For a header file in the
+           list, the source file with the same name in the same directory is
+           analyzed too, if it exists, and, for each procedure defined in the
+           header, so is a captured source file that contains it, unless the
+           list already selects one. These source files are analyzed in full
+           and all their issues are reported. Changes to headers that define
+           no procedures (types, declarations, macros) select no other source
+           file. With --compilation-database, only the source files selected
+           by the list are captured, so the list should also contain source
+           files that include the changed headers.
 
        --config-gating
            Activates: config-gating checker: Determines which config flags

infer/man/man1/infer-full.txt

@@ -343,7 +343,16 @@ OPTIONS
        --changed-files-index file
            Specify the file containing the list of source files from which
            reactive analysis should start. Source files should be specified
-           relative to project root or be absolute
+           relative to project root or be absolute. For a header file in the
+           list, the source file with the same name in the same directory is
+           analyzed too, if it exists, and, for each procedure defined in the
+           header, so is a captured source file that contains it, unless the
+           list already selects one. These source files are analyzed in full
+           and all their issues are reported. Changes to headers that define
+           no procedures (types, declarations, macros) select no other source
+           file. With --compilation-database, only the source files selected
+           by the list are captured, so the list should also contain source
+           files that include the changed headers.
            See also infer-analyze(1).
 
        --clang-biniou-file file

infer/man/man1/infer.txt

@@ -343,7 +343,16 @@ OPTIONS
        --changed-files-index file
            Specify the file containing the list of source files from which
            reactive analysis should start. Source files should be specified
-           relative to project root or be absolute
+           relative to project root or be absolute. For a header file in the
+           list, the source file with the same name in the same directory is
+           analyzed too, if it exists, and, for each procedure defined in the
+           header, so is a captured source file that contains it, unless the
+           list already selects one. These source files are analyzed in full
+           and all their issues are reported. Changes to headers that define
+           no procedures (types, declarations, macros) select no other source
+           file. With --compilation-database, only the source files selected
+           by the list are captured, so the list should also contain source
+           files that include the changed headers.
            See also infer-analyze(1).
 
        --clang-biniou-file file

infer/src/IR/SourceFiles.ml

@@ -166,6 +166,19 @@ let is_freshly_captured source =
       |> Option.exists ~f:deserialize_freshly_captured )
 
 
+let mem_statement =
+  Database.register_statement CaptureDatabase "SELECT 1 FROM source_files WHERE source_file = :k"
+
+
+let mem source =
+  Database.with_registered_statement mem_statement ~f:(fun db stmt ->
+      SourceFile.SQLite.serialize source
+      |> Sqlite3.bind stmt 1
+      |> SqliteUtils.check_result_code db ~log:"mem bind source file" ;
+      SqliteUtils.result_single_column_option ~finalize:false ~log:"SourceFiles.mem" db stmt
+      |> Option.is_some )
+
+
 let mark_all_stale () = DBWriter.mark_all_source_files_stale ()
 
 let select_all_source_files_statement =

infer/src/IR/SourceFiles.mli

@@ -30,6 +30,9 @@ val is_empty : unit -> bool
 val is_freshly_captured : SourceFile.t -> bool
 (** whether the source file was captured in the last capture phase *)
 
+val mem : SourceFile.t -> bool
+(** whether the source file has been captured *)
+
 val mark_all_stale : unit -> unit
 (** mark all source files as stale; do be called at the start of a new capture phase *)
 

infer/src/backend/AnalysisDependencyGraph.ml

@@ -51,7 +51,8 @@ let build ~changed_files =
       (* Ensure proc_name is part of the graph if it has not been referenced yet *)
       if not (CallGraph.mem_procname graph proc_name) then CallGraph.create_node graph proc_name [] ) ;
   (* Then, flag in [graph] any procedure with a summary depending (transitively) on either (1) a
-     deleted procedure, (2) the tenv of a changed file or (3) the summary of a changed procedure. *)
+     deleted procedure, (2) the tenv of a changed file, (3) the summary of a changed procedure or (4)
+     the summary of a procedure defined in a changed header. *)
   List.iter !deleted_procs ~f:(CallGraph.flag_reachable graph) ;
   SourceFile.Set.iter
     (fun sf ->
@@ -66,6 +67,11 @@ let build ~changed_files =
           | Some attrs ->
               if attrs.changed then CallGraph.flag_reachable graph pname ) )
     changed_files ;
+  (* do not rely on [changed] for procedures defined in headers: each translation unit including the
+     header compares its copy with the stored one, which may come from a translation unit captured
+     earlier in the same capture, and then marks its copy unchanged *)
+  Procedures.get_procs_defined_in_changed_headers changed_files
+  |> Procname.Map.iter (fun pname _ -> CallGraph.flag_reachable graph pname) ;
   graph
 
 

infer/src/backend/InferAnalyze.ml

@@ -68,41 +68,64 @@ let analyze_target :
     result
 
 
-let source_file_should_be_analyzed ?(no_file_means_all = true) ~changed_files source_file =
-  (* whether [fname] is one of the [changed_files] *)
-  let is_changed_file =
-    if Config.suffix_match_changed_files then
-      let path_ends_with file =
-        String.is_suffix ~suffix:(SourceFile.to_rel_path file) (SourceFile.to_rel_path source_file)
-      in
-      Option.map changed_files ~f:(SourceFile.Set.exists path_ends_with)
-    else Option.map changed_files ~f:(SourceFile.Set.mem source_file)
-  in
+let source_file_should_be_analyzed ?(no_file_means_all = true) ~changed_files
+    ~translation_units_of_changed_headers source_file =
   let check_modified () =
     let modified = SourceFiles.is_freshly_captured source_file in
     if modified then L.debug Analysis Medium "Modified: %a@\n" SourceFile.pp source_file ;
     modified
   in
-  match is_changed_file with
-  | Some b ->
-      b
+  match changed_files with
+  | Some changed_files ->
+      SourceFile.is_changed ~changed_files source_file
+      || SourceFile.Set.mem source_file translation_units_of_changed_headers
   | None when Config.reactive_mode ->
       check_modified ()
   | None ->
       no_file_means_all
 
 
+(** the translation units to analyze, besides those selected by [changed_files], so that the
+    procedures defined in changed headers are analyzed *)
+let get_translation_units_of_changed_headers changed_files =
+  let header_procs = Procedures.get_procs_defined_in_changed_headers changed_files in
+  if Procname.Map.is_empty header_procs then SourceFile.Set.empty
+  else
+    let header_procs_not_in_changed_files =
+      SourceFiles.get_all
+        ~filter:(fun source_file ->
+          (Lazy.force Filtering.source_files_filter) source_file
+          && SourceFile.is_changed ~changed_files source_file )
+        ()
+      |> List.fold ~init:header_procs ~f:(fun header_procs source_file ->
+          List.fold (SourceFiles.proc_names_of_source source_file) ~init:header_procs
+            ~f:(fun header_procs proc_name -> Procname.Map.remove proc_name header_procs ) )
+    in
+    let translation_units =
+      Procname.Map.fold
+        (fun _ -> SourceFile.Set.add)
+        header_procs_not_in_changed_files SourceFile.Set.empty
+    in
+    L.debug Analysis Quiet
+      "Translation units analyzed for procedures defined in changed headers: %a@\n"
+      (Pp.seq ~sep:", " SourceFile.pp)
+      (SourceFile.Set.elements translation_units) ;
+    translation_units
+
+
 let register_active_checkers () =
   RegisterCheckers.get_active_checkers () |> RegisterCheckers.register
 
 
-let get_source_files_to_analyze ~no_file_means_all ~changed_files =
+let get_source_files_to_analyze ~no_file_means_all ~changed_files
+    ~translation_units_of_changed_headers =
   let n_all_source_files = ref 0 in
   let n_source_files_to_analyze = ref 0 in
   let filter sourcefile =
     let result =
       (Lazy.force Filtering.source_files_filter) sourcefile
-      && source_file_should_be_analyzed ~no_file_means_all ~changed_files sourcefile
+      && source_file_should_be_analyzed ~no_file_means_all ~changed_files
+           ~translation_units_of_changed_headers sourcefile
     in
     incr n_all_source_files ;
     if result then incr n_source_files_to_analyze ;
@@ -268,8 +291,16 @@ let main ~changed_files =
       IssueLog.invalidate_all ~procedures ;
       L.progress "Done@." )
     else if not Config.incremental_analysis then DBWriter.delete_all_specs () ;
+  let translation_units_of_changed_headers =
+    Option.value_map changed_files ~default:SourceFile.Set.empty
+      ~f:get_translation_units_of_changed_headers
+  in
   let no_file_means_all = Option.is_none Config.procs_to_analyze_index in
-  let source_files = lazy (get_source_files_to_analyze ~no_file_means_all ~changed_files) in
+  let source_files =
+    lazy
+      (get_source_files_to_analyze ~no_file_means_all ~changed_files
+         ~translation_units_of_changed_headers )
+  in
   (* empty all caches to minimize the process heap to have less work to do when forking *)
   clear_caches () ;
   let initial_spec_count =
@@ -292,7 +323,8 @@ let main ~changed_files =
     (ExecutionDuration.wall_time analysis_duration) ;
   if Config.reactive_capture then
     ReactiveCapture.store_missed_captures
-      ~source_files_filter:(source_file_should_be_analyzed ~changed_files)
+      ~source_files_filter:
+        (source_file_should_be_analyzed ~changed_files ~translation_units_of_changed_headers)
       () ;
   ExecutionDuration.log ~prefix:"backend_stats.scheduler_process_analysis_time" Analysis
     analysis_duration ;

infer/src/backend/Procedures.ml

@@ -26,6 +26,67 @@ let get_all ~filter () =
   |> run_query_fold adb "reading all procedure names analysisdb" adb_stmt
 
 
+let compute_procs_defined_in_changed_headers changed_files =
+  let headers =
+    (* a deleted header defines no procedure, but with [--suffix-match-changed-files] the entries
+       can be partial paths *)
+    SourceFile.Set.filter
+      (fun file ->
+        SourceFile.is_header file
+        && (Config.suffix_match_changed_files || ISys.file_exists (SourceFile.to_abs_path file))
+        && not (SourceFiles.mem file) )
+      changed_files
+  in
+  if SourceFile.Set.is_empty headers then Procname.Map.empty
+  else
+    let db = Database.get_database CaptureDatabase in
+    let stmt = Sqlite3.prepare db "SELECT proc_attributes FROM procedures WHERE cfg IS NOT NULL" in
+    let header_procs =
+      SqliteUtils.result_fold_single_column_rows db ~log:"reading procedure locations" stmt ~init:[]
+        ~f:(fun procs data ->
+          let attrs = ProcAttributes.SQLite.deserialize data in
+          if SourceFile.is_changed ~changed_files:headers attrs.loc.file then attrs :: procs
+          else procs )
+    in
+    let files_with_procs =
+      List.map header_procs ~f:(fun {ProcAttributes.loc} -> loc.file) |> SourceFile.Set.of_list
+    in
+    let headers_without_procs =
+      SourceFile.Set.filter
+        (fun header ->
+          not
+            (SourceFile.Set.exists
+               (SourceFile.is_changed ~changed_files:(SourceFile.Set.singleton header))
+               files_with_procs ) )
+        headers
+      |> SourceFile.Set.elements
+    in
+    if not (List.is_empty headers_without_procs) then
+      if List.is_empty Config.clang_compilation_dbs then
+        L.debug Analysis Quiet "Changed headers that define no captured procedure: %a@\n"
+          (Pp.seq ~sep:", " SourceFile.pp) headers_without_procs
+      else
+        L.user_warning
+          "Changed headers that define no captured procedure: %a. With --compilation-database, \
+           only the files in the changed files index are captured: to analyze the procedures \
+           defined in a header, add a source file that includes it to the index.@."
+          (Pp.seq ~sep:", " SourceFile.pp) headers_without_procs ;
+    List.fold header_procs ~init:Procname.Map.empty
+      ~f:(fun procs {ProcAttributes.proc_name; translation_unit} ->
+        Procname.Map.add proc_name translation_unit procs )
+
+
+let get_procs_defined_in_changed_headers =
+  let cache = ref (SourceFile.Set.empty, Procname.Map.empty) in
+  fun changed_files ->
+    let cached_changed_files, procs = !cache in
+    if SourceFile.Set.equal cached_changed_files changed_files then procs
+    else
+      let procs = compute_procs_defined_in_changed_headers changed_files in
+      cache := (changed_files, procs) ;
+      procs
+
+
 let select_proc_names_interactive ~filter =
   let proc_names = get_all ~filter () |> List.rev in
   let proc_names_len = List.length proc_names in

infer/src/backend/Procedures.mli

@@ -9,6 +9,10 @@ open! IStd
 
 val get_all : filter:Filtering.procedures_filter -> unit -> Procname.t list
 
+val get_procs_defined_in_changed_headers : SourceFile.Set.t -> SourceFile.t Procname.Map.t
+(** the captured procedures defined in the changed headers, mapped to the translation units of their
+    captured copies; memoized, and warns about changed headers that define no captured procedure *)
+
 val pp_all :
      filter:Filtering.procedures_filter
   -> proc_name:bool

infer/src/base/Config.ml

@@ -1064,7 +1064,14 @@ and changed_files_index =
     ~in_help:InferCommand.[(Analyze, manual_generic)]
     ~meta:"file"
     "Specify the file containing the list of source files from which reactive analysis should \
-     start. Source files should be specified relative to project root or be absolute"
+     start. Source files should be specified relative to project root or be absolute. For a header \
+     file in the list, the source file with the same name in the same directory is analyzed too, \
+     if it exists, and, for each procedure defined in the header, so is a captured source file \
+     that contains it, unless the list already selects one. These source files are analyzed in \
+     full and all their issues are reported. Changes to headers that define no procedures (types, \
+     declarations, macros) select no other source file. With $(b,--compilation-database), only the \
+     source files selected by the list are captured, so the list should also contain source files \
+     that include the changed headers."
 
 
 and check_version =

infer/src/base/SourceFile.ml

@@ -254,10 +254,19 @@ let is_under_project_root = function
       false
 
 
+let header_exts = ["h"; "hh"; "hpp"; "hxx"]
+
+let is_header source_file =
+  snd (Filename.split_extension (to_string source_file))
+  |> Option.exists ~f:(fun ext ->
+      List.mem ~equal:String.equal
+        ("h++" :: "inc" :: "inl" :: "ipp" :: "tcc" :: "tpp" :: header_exts)
+        (String.lowercase ext) )
+
+
 let of_header ?(warn_on_error = true) header_file =
   let abs_path = to_abs_path header_file in
   let source_exts = ["c"; "cc"; "cpp"; "cxx"; "m"; "mm"] in
-  let header_exts = ["h"; "hh"; "hpp"; "hxx"] in
   match Filename.split_extension abs_path with
   | file_no_ext, Some ext when List.mem ~equal:String.equal header_exts ext ->
       List.find_map source_exts ~f:(fun ext ->
@@ -343,6 +352,13 @@ let read_config_files_to_analyze =
   fun () -> Lazy.force result
 
 
+let is_changed ~changed_files source_file =
+  if Config.suffix_match_changed_files then
+    let path = to_rel_path source_file in
+    Set.exists (fun file -> String.is_suffix ~suffix:(to_rel_path file) path) changed_files
+  else Set.mem source_file changed_files
+
+
 let is_matching patterns source_file =
   let path = to_rel_path source_file in
   List.exists

infer/src/base/SourceFile.mli

@@ -43,6 +43,10 @@ val read_config_files_to_analyze : unit -> Set.t option
 (** return the list of files as read from Config.files_to_analyze_index. NOTE: it may include extra
     source_files if --changed-files-index contains paths to header files. *)
 
+val is_changed : changed_files:Set.t -> t -> bool
+(** whether the file is one of [changed_files] or, with [--suffix-match-changed-files], whether the
+    path of one of them is a suffix of its path *)
+
 val invalid : string -> t
 (** Invalid source file *)
 
@@ -62,6 +66,10 @@ val create : ?check_abs_path:bool -> ?check_rel_path:bool -> string -> t
 val is_under_project_root : t -> bool
 (** Returns true if the file is under the project root or the workspace directory if it exists *)
 
+val is_header : t -> bool
+(** whether the file has the extension of a C/C++/Objective-C header, including files of inline or
+    template definitions such as [.inl] or [.ipp] *)
+
 val of_header : ?warn_on_error:bool -> t -> t option
 (** Return approximate source file corresponding to the parameter if it's header file and file
     exists. returns None otherwise. WARNING: If warn_on_error is false, no warning will be shown