diff --git a/analysis/reanalyze/DEADCODE_REFACTOR_PLAN.md b/analysis/reanalyze/DEADCODE_REFACTOR_PLAN.md index addd0a7356..00e4a12897 100644 --- a/analysis/reanalyze/DEADCODE_REFACTOR_PLAN.md +++ b/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,127 @@ 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. -**Test**: Run analysis twice on same input, verify identical results. Verify no side effects. +--- + +#### 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 ✅ + +**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`). + +**Note**: Optional args and incorrect annotation issues were logged inline +during `resolveRecursiveRefs`. Fixed in Task 8b. + +### Task 8b: Collect all issues in AnalysisResult.t (P5) ✅ + +**Value**: Complete the pure analysis phase - all issues returned in result, no inline logging. + +**Problem**: `resolveRecursiveRefs` was logging two types of issues inline: +1. Optional args issues (from `checkOptionalArgFn`) +2. Incorrect `@dead` annotation issues + +These bypassed `AnalysisResult.t` and were logged directly via `Log_.warning`. + +**Changes**: +- [x] Pass `~issues:(Common.issue list ref)` through `resolveRecursiveRefs` +- [x] Collect optional args issues instead of logging inline +- [x] Collect incorrect annotation issues instead of logging inline +- [x] Add collected issues to `AnalysisResult.t` in `reportDead` +- [x] Remove all `Log_.warning` calls from `resolveRecursiveRefs` + +**Status**: Complete ✅ -**Estimated effort**: Medium (many logging call sites, but mechanical) +**Key guarantee**: No `Log_.warning` calls in `resolveRecursiveRefs`. All issues +are collected in `AnalysisResult.t` and logged by the caller. ### Task 9: ~~Separate annotation computation from file writing (P5)~~ REMOVED diff --git a/analysis/reanalyze/src/AnalysisResult.ml b/analysis/reanalyze/src/AnalysisResult.ml new file mode 100644 index 0000000000..c46df48f07 --- /dev/null +++ b/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 "@{%s@} %s" + (moduleName |> Name.toInterface |> Name.toString) + "is a dead module as all its items are dead."; + }; + } diff --git a/analysis/reanalyze/src/AnalysisResult.mli b/analysis/reanalyze/src/AnalysisResult.mli new file mode 100644 index 0000000000..15f85af628 --- /dev/null +++ b/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 *) diff --git a/analysis/reanalyze/src/Common.ml b/analysis/reanalyze/src/Common.ml index 8815b2b62b..f158ff9fd1 100644 --- a/analysis/reanalyze/src/Common.ml +++ b/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 diff --git a/analysis/reanalyze/src/DeadCommon.ml b/analysis/reanalyze/src/DeadCommon.ml index ba1c740db4..3b0041cb34 100644 --- a/analysis/reanalyze/src/DeadCommon.ml +++ b/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,9 +329,10 @@ 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) - ~deadDeclarations ~level ~orderedFiles ~refs ~refsBeingResolved decl : bool - = + ~checkOptionalArg: + (checkOptionalArgFn : config:DceConfig.t -> decl -> Common.issue list) + ~deadDeclarations ~issues ~level ~orderedFiles ~refs ~refsBeingResolved decl + : bool = match decl.pos with | _ when decl.resolvedDead <> None -> if Config.recursiveDebug then @@ -369,7 +379,7 @@ let rec resolveRecursiveRefs ~all_refs ~annotations ~config ~decls xDecl |> resolveRecursiveRefs ~all_refs ~annotations ~config ~decls ~checkOptionalArg:checkOptionalArgFn ~deadDeclarations - ~level:(level + 1) ~orderedFiles ~refs:xRefs + ~issues ~level:(level + 1) ~orderedFiles ~refs:xRefs ~refsBeingResolved in if xDecl.resolvedDead = None then allDepsResolved := false; @@ -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; + (* Collect optional args issues *) + checkOptionalArgFn ~config decl + |> List.iter (fun issue -> issues := issue :: !issues); 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 ( + (* Collect incorrect @dead annotation issue *) + 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 -> issues := mod_issue :: !issues); + issues := issue :: !issues)); if config.DceConfig.cli.debug then let refsString = newRefs |> References.PosSet.elements |> List.map posToString @@ -413,8 +433,11 @@ 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) = - let iterDeclInOrder ~deadDeclarations ~orderedFiles decl = + annotations:FileAnnotations.t -> + config:DceConfig.t -> + decl -> + Common.issue list) : AnalysisResult.t = + let iterDeclInOrder ~deadDeclarations ~issues ~orderedFiles decl = let decl_refs = match decl |> Decl.isValue with | true -> References.find_value_refs refs decl.pos @@ -422,7 +445,7 @@ let reportDead ~annotations ~config ~decls ~refs ~file_deps in resolveRecursiveRefs ~all_refs:refs ~annotations ~config ~decls ~checkOptionalArg:(checkOptionalArgFn ~annotations) - ~deadDeclarations ~level:0 ~orderedFiles + ~deadDeclarations ~issues ~level:0 ~orderedFiles ~refsBeingResolved:(ref PosSet.empty) ~refs:decl_refs decl |> ignore in @@ -454,10 +477,22 @@ let reportDead ~annotations ~config ~decls ~refs ~file_deps declarations |> List.fast_sort (Decl.compareUsingDependencies ~orderedFiles) in let deadDeclarations = ref [] in + let inline_issues = ref [] in orderedDeclarations - |> List.iter (iterDeclInOrder ~orderedFiles ~deadDeclarations); + |> List.iter + (iterDeclInOrder ~orderedFiles ~deadDeclarations ~issues:inline_issues); 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 dead_issues = + sortedDeadDeclarations + |> List.concat_map (fun decl -> + Decl.report ~config ~refs reporting_ctx decl) + in + (* Combine all issues: inline issues first (they were logged during analysis), + then dead declaration issues *) + let all_issues = List.rev !inline_issues @ dead_issues in + (* Return result - caller is responsible for logging *) + AnalysisResult.add_issues AnalysisResult.empty all_issues diff --git a/analysis/reanalyze/src/DeadModules.ml b/analysis/reanalyze/src/DeadModules.ml index 924a80bd30..5635ea47ec 100644 --- a/analysis/reanalyze/src/DeadModules.ml +++ b/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 "@{%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 diff --git a/analysis/reanalyze/src/DeadOptionalArgs.ml b/analysis/reanalyze/src/DeadOptionalArgs.ml index 282dfa93d9..8d8585d5b3 100644 --- a/analysis/reanalyze/src/DeadOptionalArgs.ml +++ b/analysis/reanalyze/src/DeadOptionalArgs.ml @@ -56,38 +56,64 @@ let addReferences ~config ~cross_file ~(locFrom : Location.t) (argNamesMaybe |> String.concat ", ") (posFrom |> posToString)) -let check ~annotations ~config:_ decl = +(** Check for optional args issues. Returns issues instead of logging. *) +let check ~annotations ~config:_ decl : Common.issue list = match decl with | {declKind = Value {optionalArgs}} when active () && not (FileAnnotations.is_annotated_gentype_or_live annotations decl.pos) -> - optionalArgs - |> OptionalArgs.iterUnused (fun s -> - Log_.warning ~loc:(decl |> declGetLoc) - (DeadOptional - { - deadOptional = WarningUnusedArgument; - message = - Format.asprintf - "optional argument @{%s@} of function @{%s@} \ - is never used" - s - (decl.path |> Path.withoutHead); - })); - optionalArgs - |> OptionalArgs.iterAlwaysUsed (fun s nCalls -> - Log_.warning ~loc:(decl |> declGetLoc) - (DeadOptional - { - deadOptional = WarningRedundantOptionalArgument; - message = - Format.asprintf - "optional argument @{%s@} of function @{%s@} \ - is always supplied (%d calls)" - s - (decl.path |> Path.withoutHead) - nCalls; - })) - | _ -> () + let loc = decl |> declGetLoc in + let unused_issues = + OptionalArgs.foldUnused + (fun s acc -> + let issue : Common.issue = + { + name = "Warning Unused Argument"; + severity = Warning; + loc; + description = + DeadOptional + { + deadOptional = WarningUnusedArgument; + message = + Format.asprintf + "optional argument @{%s@} of function \ + @{%s@} is never used" + s + (decl.path |> Path.withoutHead); + }; + } + in + issue :: acc) + optionalArgs [] + in + let redundant_issues = + OptionalArgs.foldAlwaysUsed + (fun s nCalls acc -> + let issue : Common.issue = + { + name = "Warning Redundant Optional Argument"; + severity = Warning; + loc; + description = + DeadOptional + { + deadOptional = WarningRedundantOptionalArgument; + message = + Format.asprintf + "optional argument @{%s@} of function \ + @{%s@} is always supplied (%d calls)" + s + (decl.path |> Path.withoutHead) + nCalls; + }; + } + in + issue :: acc) + optionalArgs [] + in + (* Reverse to maintain original order from iterUnused/iterAlwaysUsed *) + List.rev unused_issues @ List.rev redundant_issues + | _ -> [] diff --git a/analysis/reanalyze/src/Reanalyze.ml b/analysis/reanalyze/src/Reanalyze.ml index 24e1de7b47..5099ab4541 100644 --- a/analysis/reanalyze/src/Reanalyze.ml +++ b/analysis/reanalyze/src/Reanalyze.ml @@ -152,8 +152,15 @@ let runAnalysis ~dce_config ~cmtRoot = (* Now freeze refs and file_deps for solver *) let refs = References.freeze_builder refs_builder in let file_deps = FileDeps.freeze_builder file_deps_builder in - DeadCommon.reportDead ~annotations ~decls ~refs ~file_deps - ~config:dce_config ~checkOptionalArg:DeadOptionalArgs.check); + (* Run the solver - returns immutable AnalysisResult.t *) + let analysis_result = + DeadCommon.reportDead ~annotations ~decls ~refs ~file_deps + ~config:dce_config ~checkOptionalArg:DeadOptionalArgs.check + in + (* Report all issues *) + AnalysisResult.get_issues analysis_result + |> List.iter (fun (issue : Common.issue) -> + Log_.warning ~loc:issue.loc issue.description)); if dce_config.DceConfig.run.exception_ then Exception.Checks.doChecks ~config:dce_config; if dce_config.DceConfig.run.termination && dce_config.DceConfig.cli.debug then