refactor(reanalyze): thread binding state and remove globals

Author: cristianoc

AGENTS.md

@@ -42,6 +42,7 @@ The Makefile’s targets build on each other in this order:
 - **Test early and often** - Add tests immediately after modifying each compiler layer to catch problems early, rather than waiting until all changes are complete
 
 - **Use underscore patterns carefully** - Don't use `_` patterns as lazy placeholders for new language features that then get forgotten. Only use them when you're certain the value should be ignored for that specific case. Ensure all new language features are handled correctly and completely across all compiler layers
+- **Avoid `let _ = …` for side effects** - If you need to call a function only for its side effects, use `ignore expr` (or bind the result and thread state explicitly). Do not write `let _ = expr in ()`, and do not discard stateful results—plumb them through instead.
 
 - **Be careful with similar constructor names across different IRs** - Note that `Lam` (Lambda IR) and `Lambda` (typed lambda) have variants with similar constructor names like `Ltrywith`, but they represent different things in different compilation phases.
 

analysis/reanalyze/DEADCODE_REFACTOR_PLAN.md

@@ -195,29 +195,13 @@ Goal: step‑wise removal of `Common.currentSrc`, `currentModule`, `currentModul
 
 Each bullet above should be done as a separate patch touching only a small set of functions.
 
-### 4.3 Localise `Current.*` binding state
+### 4.3 Localise `Current.*` binding/reporting state
 
-Goal: remove `DeadCommon.Current.bindings`, `lastBinding`, and `maxValuePosEnd` as mutable globals by turning them into local state threaded through functions.
+Goal: remove `DeadCommon.Current` globals for binding/reporting by threading explicit state.
 
-- [ ] In `DeadCommon`, define:
-      ```ocaml
-      type current_state = {
-        bindings : PosSet.t;
-        last_binding : Location.t;
-        max_value_pos_end : Lexing.position;
-      }
-
-      let empty_current_state = {
-        bindings = PosSet.empty;
-        last_binding = Location.none;
-        max_value_pos_end = Lexing.dummy_pos;
-      }
-      ```
-- [ ] Change `addValueReference` to take a `current_state` and return an updated `current_state` instead of reading/writing `Current.*`. For the first patch, implement it by calling the existing global‑based logic and then mirroring the resulting values into a `current_state`, so behaviour is identical.
-- [ ] Update the places that call `addValueReference` (mainly in `DeadValue`) to thread a `current_state` value through, starting from `empty_current_state`, and ignore `Current.*`.
-- [ ] In a follow‑up patch, re‑implement `addValueReference` and any other helpers that touch `Current.*` purely in terms of `current_state` and delete the `Current.*` refs from DCE code.
-
-At the end of this step, binding‑related state is explicit and confined to the call chains that need it.
+- [x] Add `Current.state`/helpers in `DeadCommon` and thread it through `DeadValue` (bindings) and `DeadException.markAsUsed` so `last_binding` is no longer a global ref.
+- [x] Replace `Current.maxValuePosEnd` with a per‑reporting `Current.state` in `Decl.report`/`reportDead`.
+- [ ] Follow‑up: remove `Current.state ref` usage by making traversals return an updated state (pure, no mutation). Adjust `addValueReference_state` (or its successor) to be purely functional and always return the new state.
 
 ### 4.4 Make `ProcessDeadAnnotations` state explicit
 
@@ -398,4 +382,3 @@ Recommended rough order of tasks (each remains independent and small):
 8. 4.14 – Add and maintain order‑independence tests.
 
 Each checkbox above should be updated to `[x]` as the corresponding change lands, keeping the codebase runnable and behaviour‑preserving after every step.
-

analysis/reanalyze/src/DeadCommon.ml

@@ -19,11 +19,26 @@ module Config = struct
 end
 
 module Current = struct
-  let bindings = ref PosSet.empty
-  let lastBinding = ref Location.none
+  type state = {
+    last_binding: Location.t;
+    max_value_pos_end: Lexing.position;
+  }
 
-  (** max end position of a value reported dead *)
-  let maxValuePosEnd = ref Lexing.dummy_pos
+  let empty_state =
+    {
+      last_binding = Location.none;
+      max_value_pos_end = Lexing.dummy_pos;
+    }
+
+  let get_last_binding (s : state) = s.last_binding
+
+  let with_last_binding (loc : Location.t) (s : state) : state =
+    {s with last_binding = loc}
+
+  let get_max_end (s : state) = s.max_value_pos_end
+
+  let with_max_end (pos : Lexing.position) (s : state) : state =
+    {s with max_value_pos_end = pos}
 end
 
 let rec checkSub s1 s2 n =
@@ -88,24 +103,26 @@ let declGetLoc decl =
   in
   {Location.loc_start; loc_end = decl.posEnd; loc_ghost = false}
 
-let addValueReference ~addFileReference ~(locFrom : Location.t)
-    ~(locTo : Location.t) =
-  let lastBinding = !Current.lastBinding in
-  let locFrom =
+let addValueReference_state ~(current : Current.state) ~addFileReference
+    ~(locFrom : Location.t) ~(locTo : Location.t) : unit =
+  let lastBinding = current.last_binding in
+  let effectiveFrom =
     match lastBinding = Location.none with
     | true -> locFrom
     | false -> lastBinding
   in
-  if not locFrom.loc_ghost then (
+  if not effectiveFrom.loc_ghost then (
     if !Cli.debug then
       Log_.item "addValueReference %s --> %s@."
-        (locFrom.loc_start |> posToString)
+        (effectiveFrom.loc_start |> posToString)
         (locTo.loc_start |> posToString);
-    ValueReferences.add locTo.loc_start locFrom.loc_start;
+    ValueReferences.add locTo.loc_start effectiveFrom.loc_start;
     if
-      addFileReference && (not locTo.loc_ghost) && (not locFrom.loc_ghost)
-      && locFrom.loc_start.pos_fname <> locTo.loc_start.pos_fname
-    then FileReferences.add locFrom locTo)
+      addFileReference && (not locTo.loc_ghost)
+      && (not effectiveFrom.loc_ghost)
+      && effectiveFrom.loc_start.pos_fname <> locTo.loc_start.pos_fname
+    then FileReferences.add effectiveFrom locTo);
+  ()
 
 let iterFilesFromRootsToLeaves iterFun =
   (* For each file, the number of incoming references *)
@@ -502,24 +519,20 @@ module Decl = struct
       (fname1, lnum1, bol1, cnum1, kind1)
       (fname2, lnum2, bol2, cnum2, kind2)
 
-  let isInsideReportedValue decl =
-    let fileHasChanged =
-      !Current.maxValuePosEnd.pos_fname <> decl.pos.pos_fname
-    in
+  let isInsideReportedValue (current_state : Current.state ref) decl =
+    let max_end = Current.get_max_end !current_state in
+    let fileHasChanged = max_end.pos_fname <> decl.pos.pos_fname in
     let insideReportedValue =
-      decl |> isValue && (not fileHasChanged)
-      && !Current.maxValuePosEnd.pos_cnum > decl.pos.pos_cnum
+      decl |> isValue && (not fileHasChanged) && max_end.pos_cnum > decl.pos.pos_cnum
     in
     if not insideReportedValue then
       if decl |> isValue then
-        if
-          fileHasChanged
-          || decl.posEnd.pos_cnum > !Current.maxValuePosEnd.pos_cnum
-        then Current.maxValuePosEnd := decl.posEnd;
+        if fileHasChanged || decl.posEnd.pos_cnum > max_end.pos_cnum then
+          current_state := Current.with_max_end decl.posEnd !current_state;
     insideReportedValue
 
-  let report decl =
-    let insideReportedValue = decl |> isInsideReportedValue in
+  let report current_state decl =
+    let insideReportedValue = decl |> isInsideReportedValue current_state in
     if decl.report then
       let name, message =
         match decl.declKind with
@@ -717,4 +730,5 @@ let reportDead ~checkOptionalArg =
     !deadDeclarations |> List.fast_sort Decl.compareForReporting
   in
   (* XXX *)
-  sortedDeadDeclarations |> List.iter Decl.report
+  let current_state = ref Current.empty_state in
+  sortedDeadDeclarations |> List.iter (Decl.report current_state)

analysis/reanalyze/src/DeadException.ml

@@ -21,13 +21,19 @@ let forceDelayedItems () =
          match Hashtbl.find_opt declarations exceptionPath with
          | None -> ()
          | Some locTo ->
-           addValueReference ~addFileReference:true ~locFrom ~locTo)
+           (* Delayed exception references don't need a binding context; use an empty state. *)
+           DeadCommon.addValueReference_state
+             ~current:DeadCommon.Current.empty_state ~addFileReference:true
+             ~locFrom ~locTo)
 
-let markAsUsed ~(locFrom : Location.t) ~(locTo : Location.t) path_ =
+let markAsUsed ~(current_state : Current.state ref) ~(locFrom : Location.t)
+    ~(locTo : Location.t) path_ =
   if locTo.loc_ghost then
     (* Probably defined in another file, delay processing and check at the end *)
     let exceptionPath =
       path_ |> Path.fromPathT |> Path.moduleToImplementation
     in
     delayedItems := {exceptionPath; locFrom} :: !delayedItems
-  else addValueReference ~addFileReference:true ~locFrom ~locTo
+  else
+    DeadCommon.addValueReference_state ~current:!current_state
+      ~addFileReference:true ~locFrom ~locTo

analysis/reanalyze/src/DeadValue.ml

@@ -15,9 +15,9 @@ let checkAnyValueBindingWithNoSideEffects
          ~sideEffects:false
   | _ -> ()
 
-let collectValueBinding super self (vb : Typedtree.value_binding) =
-  let oldCurrentBindings = !Current.bindings in
-  let oldLastBinding = !Current.lastBinding in
+let collectValueBinding current_state super self (vb : Typedtree.value_binding)
+    =
+  let oldLastBinding = Current.get_last_binding !current_state in
   checkAnyValueBindingWithNoSideEffects vb;
   let loc =
     match vb.vb_pat.pat_desc with
@@ -71,13 +71,11 @@ let collectValueBinding super self (vb : Typedtree.value_binding) =
             posStart = vb.vb_loc.loc_start;
           });
       loc
-    | _ -> !Current.lastBinding
+    | _ -> Current.get_last_binding !current_state
   in
-  Current.bindings := PosSet.add loc.loc_start !Current.bindings;
-  Current.lastBinding := loc;
+  current_state := Current.with_last_binding loc !current_state;
   let r = super.Tast_mapper.value_binding self vb in
-  Current.bindings := oldCurrentBindings;
-  Current.lastBinding := oldLastBinding;
+  current_state := Current.with_last_binding oldLastBinding !current_state;
   r
 
 let processOptionalArgs ~expType ~(locFrom : Location.t) ~locTo ~path args =
@@ -111,7 +109,7 @@ let processOptionalArgs ~expType ~(locFrom : Location.t) ~locTo ~path args =
     (!supplied, !suppliedMaybe)
     |> DeadOptionalArgs.addReferences ~locFrom ~locTo ~path)
 
-let rec collectExpr super self (e : Typedtree.expression) =
+let rec collectExpr current_state super self (e : Typedtree.expression) =
   let locFrom = e.exp_loc in
   (match e.exp_desc with
   | Texp_ident (_path, _, {Types.val_loc = {loc_ghost = false; _} as locTo}) ->
@@ -124,7 +122,9 @@ let rec collectExpr super self (e : Typedtree.expression) =
           (Location.none.loc_start |> Common.posToString)
           (locTo.loc_start |> Common.posToString);
       ValueReferences.add locTo.loc_start Location.none.loc_start)
-    else addValueReference ~addFileReference:true ~locFrom ~locTo
+    else
+      DeadCommon.addValueReference_state ~current:!current_state
+        ~addFileReference:true ~locFrom ~locTo
   | Texp_apply
       {
         funct =
@@ -190,7 +190,8 @@ let rec collectExpr super self (e : Typedtree.expression) =
         {cstr_loc = {Location.loc_start = posTo; loc_ghost} as locTo; cstr_tag},
         _ ) ->
     (match cstr_tag with
-    | Cstr_extension path -> path |> DeadException.markAsUsed ~locFrom ~locTo
+    | Cstr_extension path ->
+      path |> DeadException.markAsUsed ~current_state ~locFrom ~locTo
     | _ -> ());
     if !Config.analyzeTypes && not loc_ghost then
       DeadType.addTypeReference ~posTo ~posFrom:locFrom.loc_start
@@ -202,7 +203,7 @@ let rec collectExpr super self (e : Typedtree.expression) =
              ->
              (* Punned field in OCaml projects has ghost location in expression *)
              let e = {e with exp_loc = {exp_loc with loc_ghost = false}} in
-             collectExpr super self e |> ignore
+             collectExpr current_state super self e |> ignore
            | _ -> ())
   | _ -> ());
   super.Tast_mapper.expr self e
@@ -286,9 +287,10 @@ let rec processSignatureItem ~doTypes ~doValues ~moduleLoc ~path
 (* Traverse the AST *)
 let traverseStructure ~doTypes ~doExternals =
   let super = Tast_mapper.default in
-  let expr self e = e |> collectExpr super self in
+  let current_state = ref Current.empty_state in
+  let expr self e = e |> collectExpr current_state super self in
   let pat self p = p |> collectPattern super self in
-  let value_binding self vb = vb |> collectValueBinding super self in
+  let value_binding self vb = vb |> collectValueBinding current_state super self in
   let structure_item self (structureItem : Typedtree.structure_item) =
     let oldModulePath = ModulePath.getCurrent () in
     (match structureItem.str_desc with
@@ -365,7 +367,7 @@ let traverseStructure ~doTypes ~doExternals =
   {super with expr; pat; structure_item; value_binding}
 
 (* Merge a location's references to another one's *)
-let processValueDependency
+let processValueDependency current_state
     ( ({
          val_loc =
            {loc_start = {pos_fname = fnTo} as posTo; loc_ghost = ghost1} as
@@ -380,12 +382,14 @@ let processValueDependency
         Types.value_description) ) =
   if (not ghost1) && (not ghost2) && posTo <> posFrom then (
     let addFileReference = fileIsImplementationOf fnTo fnFrom in
-    addValueReference ~addFileReference ~locFrom ~locTo;
+    DeadCommon.addValueReference_state ~current:!current_state
+      ~addFileReference ~locFrom ~locTo;
     DeadOptionalArgs.addFunctionReference ~locFrom ~locTo)
 
 let processStructure ~cmt_value_dependencies ~doTypes ~doExternals
     (structure : Typedtree.structure) =
   let traverseStructure = traverseStructure ~doTypes ~doExternals in
   structure |> traverseStructure.structure traverseStructure |> ignore;
   let valueDependencies = cmt_value_dependencies |> List.rev in
-  valueDependencies |> List.iter processValueDependency
+  let current_state = ref Current.empty_state in
+  valueDependencies |> List.iter (processValueDependency current_state)