diff --git a/AGENTS.md b/AGENTS.md index 9afb254749..dcf296590f 100644 --- a/AGENTS.md +++ b/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. diff --git a/analysis/reanalyze/DEADCODE_REFACTOR_PLAN.md b/analysis/reanalyze/DEADCODE_REFACTOR_PLAN.md index 4b4004516d..ec86510545 100644 --- a/analysis/reanalyze/DEADCODE_REFACTOR_PLAN.md +++ b/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. - diff --git a/analysis/reanalyze/src/DeadCommon.ml b/analysis/reanalyze/src/DeadCommon.ml index 9dbacba7bf..c1a250b8aa 100644 --- a/analysis/reanalyze/src/DeadCommon.ml +++ b/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) diff --git a/analysis/reanalyze/src/DeadException.ml b/analysis/reanalyze/src/DeadException.ml index 023bee3f68..02e54b1e3b 100644 --- a/analysis/reanalyze/src/DeadException.ml +++ b/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 diff --git a/analysis/reanalyze/src/DeadValue.ml b/analysis/reanalyze/src/DeadValue.ml index df8b6aa0e2..3f80684def 100644 --- a/analysis/reanalyze/src/DeadValue.ml +++ b/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,7 +382,8 @@ 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 @@ -388,4 +391,5 @@ let processStructure ~cmt_value_dependencies ~doTypes ~doExternals 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)