DCE: Make analysis phase return AnalysisResult.t (Task 8)

This completes Task 8 of the DCE refactor plan, making the analysis phase
return an immutable AnalysisResult.t instead of logging directly.

Key changes:
- Add AnalysisResult module with immutable result type and issue constructors
- makeDeadIssue: pure function to create dead code issues
- Decl.report: returns issue list (includes dead module + dead value issues)
- DeadOptionalArgs.check: returns issue list instead of logging
- DeadModules.checkModuleDead: returns issue option instead of logging
- reportDead: returns AnalysisResult.t, caller logs issues

Architecture after this change:
  merged_view (immutable)
      │
      ▼
  reportDead (pure function)
      │
      ▼
  AnalysisResult.t (immutable)
      │
      ▼
  report (side effects here only)

Note: Optional args and incorrect annotation issues are still logged inline
during resolveRecursiveRefs. A future task could collect these as well.

Author: cristianoc

analysis/reanalyze/DEADCODE_REFACTOR_PLAN.md

@@ -410,16 +410,7 @@ They should follow the same pattern as everything else.
 **Value**: Analysis phase works on immutable merged data, returns immutable results.
 Can be parallelized, memoized, reordered.
 
-**Changes**:
-- [ ] `solve_deadness : config -> merged_view -> analysis_result` (pure)
-- [ ] Input `merged_view` is immutable (from Tasks 4-7)
-- [ ] Output `analysis_result` is immutable
-- [ ] `Decl.report`: Return `issue` instead of logging
-- [ ] Remove all `Log_.warning`, `Log_.item` calls from analysis path
-- [ ] Side effects (logging, JSON) only in final reporting phase
-- [ ] Make `DeadModules` state part of `analysis_result` (currently mutated during solver)
-
-**Architecture**:
+**Architecture goal**:
 ```
 merged_view (immutable) 
     │
@@ -433,12 +424,105 @@ analysis_result (immutable)
 report (side effects here only)
 ```
 
-**Key guarantee**: After Tasks 4-7, the analysis phase has **no mutable state**.
-This enables parallelization, caching, and incremental recomputation.
+**Approach**: Break into small, behavior-preserving steps. Each step can be verified
+before moving to the next. The key is: change return type, then immediately log at
+the call site, so behavior stays identical.
+
+---
+
+#### Task 8.1: Create `AnalysisResult` module ✅
+
+**Changes**:
+- [x] Create `AnalysisResult.ml/mli` with `type t = { issues: Common.issue list }`
+- [x] Add constructors: `empty`, `add_issue`, `get_issues`
+- [x] Add issue constructors: `make_dead_issue`, `make_dead_module_issue`
+
+**Verify**: Build succeeds. No behavior change (module not used yet).
+
+---
+
+#### Task 8.2: Make `emitWarning` return issue (behavior preserving) ✅
+
+**Changes**:
+- [x] Add `makeDeadIssue` (pure function)
+- [x] `emitWarning` uses `makeDeadIssue` internally (later removed)
+
+**Verify**: `make test-analysis` passes with identical output.
+
+---
+
+#### Task 8.3: Make `Decl.report` return issue option (behavior preserving) ✅
+
+**Changes**:
+- [x] Change `Decl.report` signature to return `issue option`
+- [x] Use `makeDeadIssue` internally
+- [x] At call site in `reportDead`, log returned issue
+
+**Verify**: `make test-analysis` passes with identical output.
+
+---
+
+#### Task 8.4: Make `DeadOptionalArgs.check` return issues (behavior preserving) ✅
+
+**Changes**:
+- [x] Add `foldUnused`, `foldAlwaysUsed` to `OptionalArgs` module
+- [x] Change `check` signature to return `issue list` instead of `unit`
+- [x] At call site in `resolveRecursiveRefs`, immediately log returned issues
+
+**Verify**: `make test-analysis` passes with identical output.
+
+---
+
+#### Task 8.5: Collect incorrect annotation issues (behavior preserving) ✅
+
+**Changes**:
+- [x] Use `makeDeadIssue` for incorrect `@dead` annotation issues
+- [x] Log immediately at call site
+- [x] Remove `emitWarning` function (no longer needed)
+
+**Verify**: `make test-analysis` passes with identical output.
+
+---
+
+#### Task 8.6: Make `DeadModules.checkModuleDead` return issue (behavior preserving) ✅
+
+**Changes**:
+- [x] Change `DeadModules.checkModuleDead` to return `issue option`
+- [x] At call sites, log returned issue immediately
+
+**Verify**: `make test-analysis` passes with identical output.
+
+---
+
+#### Task 8.7: Collect all issues in `reportDead` (behavior preserving) ✅
+
+**Changes**:
+- [x] Change `Decl.report` to return `issue list` (includes dead module issues)
+- [x] Use `List.concat_map` to collect all issues
+- [x] Log all issues at the end of `reportDead`
+
+**Verify**: `make test-analysis` passes with identical output.
+
+---
+
+#### Task 8.8: Return `AnalysisResult.t` from `reportDead` ✅
+
+**Changes**:
+- [x] Change `reportDead` to return `AnalysisResult.t` instead of `unit`
+- [x] Move logging from `reportDead` to caller in `Reanalyze.ml`
+
+**Verify**: `make test-analysis` passes with identical output.
+
+---
+
+**Status**: Complete ✅
 
-**Test**: Run analysis twice on same input, verify identical results. Verify no side effects.
+**Key guarantee**: The analysis phase (`reportDead`) now returns an immutable
+`AnalysisResult.t` containing all dead code issues. Side effects (logging)
+only happen in the caller (`Reanalyze.runAnalysis`).
 
-**Estimated effort**: Medium (many logging call sites, but mechanical)
+**Note**: Optional args and incorrect annotation issues are still logged inline
+during `resolveRecursiveRefs`. A future task could collect these as well.
 
 ### Task 9: ~~Separate annotation computation from file writing (P5)~~ REMOVED
 

analysis/reanalyze/src/AnalysisResult.ml

@@ -0,0 +1,52 @@
+(** Analysis result - immutable output from the solver.
+    
+    The solver returns this instead of logging directly.
+    All side effects (logging, JSON output) happen in the reporting phase. *)
+
+open Common
+
+type t = {issues: issue list}
+(** Immutable analysis result *)
+
+let empty = {issues = []}
+
+let add_issue result issue = {issues = issue :: result.issues}
+
+let add_issues result new_issues =
+  {issues = List.rev_append new_issues result.issues}
+
+let get_issues result = result.issues |> List.rev
+
+let issue_count result = List.length result.issues
+
+(** Create a dead code issue *)
+let make_dead_issue ~loc ~deadWarning ~path ~message =
+  {
+    name =
+      (match deadWarning with
+      | WarningDeadException -> "Warning Dead Exception"
+      | WarningDeadType -> "Warning Dead Type"
+      | WarningDeadValue -> "Warning Dead Value"
+      | WarningDeadValueWithSideEffects ->
+        "Warning Dead Value With Side Effects"
+      | IncorrectDeadAnnotation -> "Incorrect Dead Annotation");
+    severity = Warning;
+    loc;
+    description = DeadWarning {deadWarning; path; message};
+  }
+
+(** Create a dead module issue *)
+let make_dead_module_issue ~loc ~moduleName =
+  {
+    name = "Warning Dead Module";
+    severity = Warning;
+    loc;
+    description =
+      DeadModule
+        {
+          message =
+            Format.asprintf "@{<info>%s@} %s"
+              (moduleName |> Name.toInterface |> Name.toString)
+              "is a dead module as all its items are dead.";
+        };
+  }

analysis/reanalyze/src/AnalysisResult.mli

@@ -0,0 +1,37 @@
+(** Analysis result - immutable output from the solver.
+    
+    The solver returns this instead of logging directly.
+    All side effects (logging, JSON output) happen in the reporting phase. *)
+
+open Common
+
+type t
+(** Immutable analysis result *)
+
+val empty : t
+(** Empty result with no issues *)
+
+val add_issue : t -> issue -> t
+(** Add a single issue to the result *)
+
+val add_issues : t -> issue list -> t
+(** Add multiple issues to the result *)
+
+val get_issues : t -> issue list
+(** Get all issues in order they were added *)
+
+val issue_count : t -> int
+(** Count of issues *)
+
+(** {2 Issue constructors} *)
+
+val make_dead_issue :
+  loc:Location.t ->
+  deadWarning:deadWarning ->
+  path:string ->
+  message:string ->
+  issue
+(** Create a dead code warning issue *)
+
+val make_dead_module_issue : loc:Location.t -> moduleName:Name.t -> issue
+(** Create a dead module warning issue *)

analysis/reanalyze/src/Common.ml

@@ -133,6 +133,11 @@ module OptionalArgs = struct
 
   let iterUnused f x = StringSet.iter f x.unused
   let iterAlwaysUsed f x = StringSet.iter (fun s -> f s x.count) x.alwaysUsed
+
+  let foldUnused f x init = StringSet.fold f x.unused init
+
+  let foldAlwaysUsed f x init =
+    StringSet.fold (fun s acc -> f s x.count acc) x.alwaysUsed init
 end
 
 module DeclKind = struct

analysis/reanalyze/src/DeadCommon.ml

@@ -159,13 +159,12 @@ let addValueDeclaration ~config ~decls ~file ?(isToplevel = true)
        ~declKind:(Value {isToplevel; optionalArgs; sideEffects})
        ~loc ~moduleLoc ~path
 
-let emitWarning ~config ~decl ~message deadWarning =
+(** Create a dead code issue. Pure - no side effects. *)
+let makeDeadIssue ~decl ~message deadWarning : Common.issue =
   let loc = decl |> declGetLoc in
-  decl.path
-  |> Path.toModuleName ~isType:(decl.declKind |> DeclKind.isType)
-  |> DeadModules.checkModuleDead ~config ~fileName:decl.pos.pos_fname;
-  Log_.warning ~loc
-    (DeadWarning {deadWarning; path = Path.withoutHead decl.path; message})
+  AnalysisResult.make_dead_issue ~loc ~deadWarning
+    ~path:(Path.withoutHead decl.path)
+    ~message
 
 module Decl = struct
   let isValue decl =
@@ -250,10 +249,13 @@ module Decl = struct
           ReportingContext.set_max_end ctx decl.posEnd;
     insideReportedValue
 
-  let report ~config ~refs (ctx : ReportingContext.t) decl =
+  (** Report a dead declaration. Returns list of issues (dead module first, then dead value).
+      Caller is responsible for logging. *)
+  let report ~config ~refs (ctx : ReportingContext.t) decl : Common.issue list =
     let insideReportedValue = decl |> isInsideReportedValue ctx in
-    if decl.report then
-      let name, message =
+    if not decl.report then []
+    else
+      let deadWarning, message =
         match decl.declKind with
         | Exception ->
           (WarningDeadException, "is never raised or passed as value")
@@ -300,11 +302,18 @@ module Decl = struct
            | _ -> true)
         && (config.DceConfig.run.transitive || not (hasRefBelow ()))
       in
-      if shouldEmitWarning then (
-        decl.path
-        |> Path.toModuleName ~isType:(decl.declKind |> DeclKind.isType)
-        |> DeadModules.checkModuleDead ~config ~fileName:decl.pos.pos_fname;
-        emitWarning ~config ~decl ~message name)
+      if shouldEmitWarning then
+        let dead_module_issue =
+          decl.path
+          |> Path.toModuleName ~isType:(decl.declKind |> DeclKind.isType)
+          |> DeadModules.checkModuleDead ~config ~fileName:decl.pos.pos_fname
+        in
+        let dead_value_issue = makeDeadIssue ~decl ~message deadWarning in
+        (* Return in order: dead module first (if any), then dead value *)
+        match dead_module_issue with
+        | Some mi -> [mi; dead_value_issue]
+        | None -> [dead_value_issue]
+      else []
 end
 
 let declIsDead ~annotations ~refs decl =
@@ -320,7 +329,8 @@ let doReportDead ~annotations pos =
   not (FileAnnotations.is_annotated_gentype_or_dead annotations pos)
 
 let rec resolveRecursiveRefs ~all_refs ~annotations ~config ~decls
-    ~checkOptionalArg:(checkOptionalArgFn : config:DceConfig.t -> decl -> unit)
+    ~checkOptionalArg:
+      (checkOptionalArgFn : config:DceConfig.t -> decl -> Common.issue list)
     ~deadDeclarations ~level ~orderedFiles ~refs ~refsBeingResolved decl : bool
     =
   match decl.pos with
@@ -387,14 +397,24 @@ let rec resolveRecursiveRefs ~all_refs ~annotations ~config ~decls
         if not (doReportDead ~annotations decl.pos) then decl.report <- false;
         deadDeclarations := decl :: !deadDeclarations)
       else (
-        checkOptionalArgFn ~config decl;
+        checkOptionalArgFn ~config decl
+        |> List.iter (fun (issue : Common.issue) ->
+               Log_.warning ~loc:issue.loc issue.description);
         decl.path
         |> DeadModules.markLive ~config
              ~isType:(decl.declKind |> DeclKind.isType)
              ~loc:decl.moduleLoc;
-        if FileAnnotations.is_annotated_dead annotations decl.pos then
-          emitWarning ~config ~decl ~message:" is annotated @dead but is live"
-            IncorrectDeadAnnotation);
+        if FileAnnotations.is_annotated_dead annotations decl.pos then (
+          let issue =
+            makeDeadIssue ~decl ~message:" is annotated @dead but is live"
+              IncorrectDeadAnnotation
+          in
+          decl.path
+          |> Path.toModuleName ~isType:(decl.declKind |> DeclKind.isType)
+          |> DeadModules.checkModuleDead ~config ~fileName:decl.pos.pos_fname
+          |> Option.iter (fun (mod_issue : Common.issue) ->
+                 Log_.warning ~loc:mod_issue.loc mod_issue.description);
+          Log_.warning ~loc:issue.loc issue.description));
       if config.DceConfig.cli.debug then
         let refsString =
           newRefs |> References.PosSet.elements |> List.map posToString
@@ -413,7 +433,10 @@ let rec resolveRecursiveRefs ~all_refs ~annotations ~config ~decls
 let reportDead ~annotations ~config ~decls ~refs ~file_deps
     ~checkOptionalArg:
       (checkOptionalArgFn :
-        annotations:FileAnnotations.t -> config:DceConfig.t -> decl -> unit) =
+        annotations:FileAnnotations.t ->
+        config:DceConfig.t ->
+        decl ->
+        Common.issue list) : AnalysisResult.t =
   let iterDeclInOrder ~deadDeclarations ~orderedFiles decl =
     let decl_refs =
       match decl |> Decl.isValue with
@@ -459,5 +482,12 @@ let reportDead ~annotations ~config ~decls ~refs ~file_deps
   let sortedDeadDeclarations =
     !deadDeclarations |> List.fast_sort Decl.compareForReporting
   in
+  (* Collect issues from dead declarations *)
   let reporting_ctx = ReportingContext.create () in
-  sortedDeadDeclarations |> List.iter (Decl.report ~config ~refs reporting_ctx)
+  let issues =
+    sortedDeadDeclarations
+    |> List.concat_map (fun decl ->
+           Decl.report ~config ~refs reporting_ctx decl)
+  in
+  (* Return result - caller is responsible for logging *)
+  AnalysisResult.add_issues AnalysisResult.empty issues

analysis/reanalyze/src/DeadModules.ml

@@ -19,8 +19,11 @@ let markLive ~config ~isType ~(loc : Location.t) path =
     | Some (false, loc) -> Hashtbl.replace table moduleName (true, loc)
     | Some (true, _) -> ()
 
-let checkModuleDead ~config ~fileName:pos_fname moduleName =
-  if active ~config then
+(** Check if a module is dead and return issue if so. Pure - no logging. *)
+let checkModuleDead ~config ~fileName:pos_fname moduleName : Common.issue option
+    =
+  if not (active ~config) then None
+  else
     match Hashtbl.find_opt table moduleName with
     | Some (false, loc) ->
       Hashtbl.remove table moduleName;
@@ -33,12 +36,5 @@ let checkModuleDead ~config ~fileName:pos_fname moduleName =
           {Location.loc_start = pos; loc_end = pos; loc_ghost = false}
         else loc
       in
-      Log_.warning ~loc
-        (Common.DeadModule
-           {
-             message =
-               Format.asprintf "@{<info>%s@} %s"
-                 (moduleName |> Name.toInterface |> Name.toString)
-                 "is a dead module as all its items are dead.";
-           })
-    | _ -> ()
+      Some (AnalysisResult.make_dead_module_issue ~loc ~moduleName)
+    | _ -> None