Skip to content

Commit 72edbe9

Browse files
committed
refactor(dce): extract configuration into explicit value (Task 2)
Replace global config reads with explicit ~config parameter threading throughout the DCE analysis pipeline. This makes the analysis pure and testable with different configurations. ## Changes ### New module - DceConfig: Encapsulates DCE configuration (cli + run config) - DceConfig.current() captures global state once - All analysis functions now take explicit ~config parameter ### DCE Analysis (fully pure - no global reads) - DeadCode: threads config to all Dead* modules - DeadValue: replaced ~15 !Cli.debug reads with config.cli.debug - DeadType: replaced ~7 !Cli.debug reads with config.cli.debug - DeadOptionalArgs: takes ~config, passes to Log_.warning - DeadModules: uses config.run.transitive - DeadCommon: threads config through reporting pipeline - WriteDeadAnnotations: uses config.cli.write/json - ProcessDeadAnnotations: uses config.cli.live_names/live_paths ### Logging infrastructure - Log_.warning: now requires ~config (no optional) - Log_.logIssue: now requires ~config (no optional) - Log_.Stats.report: now requires ~config (no optional) - Consistent API - no conditional logic on Some/None ### Non-DCE analyses (call DceConfig.current() at use sites) - Exception: 4 call sites updated - Arnold: 7 call sites updated - TODO: Thread config through these for full purity ### Other - Common.ml: removed unused lineAnnotationStr field - Reanalyze: single DceConfig.current() call at entry point - DEADCODE_REFACTOR_PLAN.md: updated Task 2, added verification task ## Impact ✅ DCE analysis is now pure - takes explicit config, no global reads ✅ All config parameters required (zero 'config option' types) ✅ Can run analysis with different configs without mutating globals ✅ All tests pass - no regressions ## Remaining Work (Task 2) - Thread config through Exception/Arnold to eliminate DceConfig.current() - Verify zero DceConfig.current() calls in analysis code Signed-off-by: Cursor AI <ai@cursor.com> Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
1 parent a4c5466 commit 72edbe9

16 files changed

+276
-195
lines changed

AGENTS.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ The Makefile’s targets build on each other in this order:
4444
- **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
4545
- **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.
4646

47+
- **Don't use unit `()` with mandatory labeled arguments** - When a function has a mandatory labeled argument (like `~config`), don't add a trailing `()` parameter. The labeled argument already prevents accidental partial application. Only use `()` when all parameters are optional and you need to force evaluation. Example: `let forceDelayedItems ~config = ...` not `let forceDelayedItems ~config () = ...`
48+
4749
- **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.
4850

4951
- **Avoid warning suppressions** - Never use `[@@warning "..."]` to silence warnings. Instead, fix the underlying issue properly

analysis/reanalyze/DEADCODE_REFACTOR_PLAN.md

Lines changed: 27 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -149,13 +149,17 @@ Each task should:
149149
**Value**: Can run analysis with different configs without mutating globals. Can test with different configs.
150150

151151
**Changes**:
152-
- [ ] Use the `DceConfig.t` already created, thread it through analysis functions
153-
- [ ] Replace all `!Common.Cli.debug`, `runConfig.transitive`, etc. reads with `config.debug`, `config.run.transitive`
154-
- [ ] Only `DceConfig.current()` reads globals; everything else uses explicit config
152+
- [x] ~~Use the `DceConfig.t` already created, thread it through DCE analysis functions~~
153+
- [x] ~~Replace all DCE code's `!Common.Cli.debug`, `runConfig.transitive`, etc. reads with `config.debug`, `config.run.transitive`~~
154+
- [x] ~~Make all config parameters required (not optional) - no `config option` anywhere~~
155+
- [ ] **Thread config through Exception and Arnold analyses** - they currently call `DceConfig.current()` at each use site
156+
- [ ] **Single entry point**: Only `Reanalyze.runAnalysisAndReport` should call `DceConfig.current()` once, then pass explicit config everywhere
157+
158+
**Status**: DCE code complete ✅. Exception/Arnold still need threading.
155159

156160
**Test**: Create two configs with different settings, run analysis with each - should respect the config, not read globals.
157161

158-
**Estimated effort**: Medium (many small changes across multiple files)
162+
**Estimated effort**: Medium (DCE done; Exception/Arnold similar effort)
159163

160164
### Task 3: Make `ProcessDeadAnnotations` state explicit (P3)
161165

@@ -253,7 +257,20 @@ Each task should:
253257

254258
**Estimated effort**: Small (single module)
255259

256-
### Task 10: Integration and order-independence verification
260+
### Task 10: Verify zero `DceConfig.current()` calls in analysis code
261+
262+
**Value**: Enforce purity - no hidden global reads.
263+
264+
**Changes**:
265+
- [ ] Verify `DceConfig.current()` only called in `Reanalyze.runAnalysisAndReport` (entry point)
266+
- [ ] Verify no calls to `DceConfig.current()` in `Dead*.ml`, `Exception.ml`, `Arnold.ml` analysis code
267+
- [ ] All analysis functions take explicit `~config` parameter
268+
269+
**Test**: `grep -r "DceConfig.current" analysis/reanalyze/src/{Dead,Exception,Arnold}.ml` returns zero results.
270+
271+
**Estimated effort**: Trivial (verification only, assuming Task 2 complete)
272+
273+
### Task 11: Integration and order-independence verification
257274

258275
**Value**: Verify the refactor achieved its goals.
259276

@@ -271,13 +288,15 @@ Each task should:
271288

272289
## Execution Strategy
273290

274-
**Recommended order**: 1 → 2 → 3 → 4 → 5 → 6 → 7 → 8 → 9 → 10
291+
**Recommended order**: 1 → 2 (complete all analyses) → 3 → 4 → 5 → 6 → 7 → 8 → 9 → 10 (verify) → 11 (test)
275292

276293
**Why this order?**
277294
- Tasks 1-2 remove implicit dependencies (file context, config) - these are foundational
295+
- Task 2 must be **fully complete** (DCE + Exception + Arnold) before proceeding
278296
- Tasks 3-7 localize global state - can be done incrementally once inputs are explicit
279297
- Tasks 8-9 separate pure/impure - can only do this once state is local
280-
- Task 10 validates everything
298+
- Task 10 verifies no global config reads remain
299+
- Task 11 validates everything
281300

282301
**Alternative**: Could do 3-7 in any order (they're mostly independent).
283302

@@ -295,6 +314,7 @@ After all tasks:
295314
**No global mutable state in analysis path**
296315
- No `ref` or mutable `Hashtbl` in `Dead*.ml` modules
297316
- All state is local or explicitly threaded
317+
- **Zero `DceConfig.current()` calls in analysis code** - only at entry point
298318

299319
**Order independence**
300320
- Processing files in any order gives identical results

analysis/reanalyze/src/Arnold.ml

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ module Stats = struct
111111
incr nCacheChecks;
112112
if hit then incr nCacheHits;
113113
if !Common.Cli.debug then
114-
Log_.warning ~forStats:false ~loc
114+
Log_.warning ~config:(DceConfig.current ()) ~forStats:false ~loc
115115
(Termination
116116
{
117117
termination = TerminationAnalysisInternal;
@@ -125,7 +125,7 @@ module Stats = struct
125125

126126
let logResult ~functionCall ~loc ~resString =
127127
if !Common.Cli.debug then
128-
Log_.warning ~forStats:false ~loc
128+
Log_.warning ~config:(DceConfig.current ()) ~forStats:false ~loc
129129
(Termination
130130
{
131131
termination = TerminationAnalysisInternal;
@@ -610,7 +610,7 @@ module ExtendFunctionTable = struct
610610
then (
611611
functionTable |> FunctionTable.addFunction ~functionName;
612612
if !Common.Cli.debug then
613-
Log_.warning ~forStats:false ~loc
613+
Log_.warning ~config:(DceConfig.current ()) ~forStats:false ~loc
614614
(Termination
615615
{
616616
termination = TerminationAnalysisInternal;
@@ -632,7 +632,8 @@ module ExtendFunctionTable = struct
632632
functionTable
633633
|> FunctionTable.addLabelToKind ~functionName ~label;
634634
if !Common.Cli.debug then
635-
Log_.warning ~forStats:false ~loc
635+
Log_.warning ~config:(DceConfig.current ()) ~forStats:false
636+
~loc
636637
(Termination
637638
{
638639
termination = TerminationAnalysisInternal;
@@ -699,7 +700,8 @@ module CheckExpressionWellFormed = struct
699700
functionTable
700701
|> FunctionTable.addLabelToKind ~functionName ~label;
701702
if !Common.Cli.debug then
702-
Log_.warning ~forStats:false ~loc:body.exp_loc
703+
Log_.warning ~config:(DceConfig.current ())
704+
~forStats:false ~loc:body.exp_loc
703705
(Termination
704706
{
705707
termination = TerminationAnalysisInternal;
@@ -873,7 +875,7 @@ module Compile = struct
873875
newFunctionName;
874876
newFunctionDefinition.body <- Some (vb_expr |> expression ~ctx:newCtx);
875877
if !Common.Cli.debug then
876-
Log_.warning ~forStats:false ~loc:pat_loc
878+
Log_.warning ~config:(DceConfig.current ()) ~forStats:false ~loc:pat_loc
877879
(Termination
878880
{
879881
termination = TerminationAnalysisInternal;
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
(** Configuration for dead code elimination analysis.
2+
3+
This module encapsulates all configuration needed for DCE,
4+
gathered from RunConfig and CLI flags. *)
5+
6+
type cli_config = {
7+
debug: bool;
8+
ci: bool;
9+
json: bool;
10+
write: bool;
11+
live_names: string list;
12+
live_paths: string list;
13+
exclude_paths: string list;
14+
}
15+
16+
type t = {run: RunConfig.t; cli: cli_config}
17+
18+
(** Capture the current DCE configuration from global state.
19+
20+
This reads from [RunConfig.runConfig] and [Common.Cli] refs
21+
to produce a single immutable configuration value. *)
22+
let current () =
23+
let cli =
24+
{
25+
debug = !Common.Cli.debug;
26+
ci = !Common.Cli.ci;
27+
json = !Common.Cli.json;
28+
write = !Common.Cli.write;
29+
live_names = !Common.Cli.liveNames;
30+
live_paths = !Common.Cli.livePaths;
31+
exclude_paths = !Common.Cli.excludePaths;
32+
}
33+
in
34+
{run = Common.runConfig; cli}

analysis/reanalyze/src/DeadCode.ml

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,33 @@
11
open DeadCommon
22

3-
let processSignature ~doValues ~doTypes (signature : Types.signature) =
3+
let processSignature ~config ~doValues ~doTypes (signature : Types.signature) =
44
signature
55
|> List.iter (fun sig_item ->
6-
DeadValue.processSignatureItem ~doValues ~doTypes
6+
DeadValue.processSignatureItem ~config ~doValues ~doTypes
77
~moduleLoc:Location.none
88
~path:[!Common.currentModuleName]
99
sig_item)
1010

11-
let processCmt ~cmtFilePath (cmt_infos : Cmt_format.cmt_infos) =
11+
let processCmt ~config ~cmtFilePath (cmt_infos : Cmt_format.cmt_infos) =
1212
(match cmt_infos.cmt_annots with
1313
| Interface signature ->
14-
ProcessDeadAnnotations.signature signature;
15-
processSignature ~doValues:true ~doTypes:true signature.sig_type
14+
ProcessDeadAnnotations.signature ~config signature;
15+
processSignature ~config ~doValues:true ~doTypes:true signature.sig_type
1616
| Implementation structure ->
1717
let cmtiExists =
1818
Sys.file_exists ((cmtFilePath |> Filename.remove_extension) ^ ".cmti")
1919
in
20-
ProcessDeadAnnotations.structure ~doGenType:(not cmtiExists) structure;
21-
processSignature ~doValues:true ~doTypes:false structure.str_type;
20+
ProcessDeadAnnotations.structure ~config ~doGenType:(not cmtiExists)
21+
structure;
22+
processSignature ~config ~doValues:true ~doTypes:false structure.str_type;
2223
let doExternals =
2324
(* This is already handled at the interface level, avoid issues in inconsistent locations
2425
https://github.com/BuckleScript/syntax/pull/54
2526
Ideally, the handling should be less location-based, just like other language aspects. *)
2627
false
2728
in
28-
DeadValue.processStructure ~doTypes:true ~doExternals
29+
DeadValue.processStructure ~config ~doTypes:true ~doExternals
2930
~cmt_value_dependencies:cmt_infos.cmt_value_dependencies structure
3031
| _ -> ());
31-
DeadType.TypeDependencies.forceDelayedItems ();
32+
DeadType.TypeDependencies.forceDelayedItems ~config;
3233
DeadType.TypeDependencies.clear ()

0 commit comments

Comments
 (0)