Skip to content

Commit dfcb63e

Browse files
committed
Fix dead module location and hasRefBelow in reactive mode
- Dead module issues with ghost locations now use fileName from declaration instead of _none_ from the ghost location - hasRefBelow now correctly checks ALL refs (including cross-file) since refIsBelow returns true for any cross-file ref - Both fixes ensure reactive/non-reactive produce identical results - Verified against master: all 4 mode combinations match exactly
1 parent 8a1b2f3 commit dfcb63e

File tree

1 file changed

+33
-44
lines changed

1 file changed

+33
-44
lines changed

analysis/reanalyze/src/ReactiveSolver.ml

Lines changed: 33 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
- is_pos_live uses reactive live collection (no resolvedDead mutation)
1616
- shouldReport callback replaces report field mutation (no mutation needed)
1717
- isInsideReportedValue is per-file only, so files are independent
18-
- hasRefBelow uses per-file refs: O(file_refs) per dead decl (was O(total_refs))
18+
- hasRefBelow uses on-demand search: O(total_refs) per dead decl (cross-file refs count as "below")
1919
2020
All issues now match between reactive and non-reactive modes (380 on deadcode test):
2121
- Dead code issues: 362 (Exception:2, Module:31, Type:87, Value:233, ValueWithSideEffects:8)
@@ -29,8 +29,8 @@ type t = {
2929
live_decls: (Lexing.position, Decl.t) Reactive.t;
3030
annotations: (Lexing.position, FileAnnotations.annotated_as) Reactive.t;
3131
value_refs_from: (Lexing.position, PosSet.t) Reactive.t option;
32-
dead_modules: (Name.t, Location.t) Reactive.t;
33-
(** Modules where all declarations are dead. Reactive anti-join. *)
32+
dead_modules: (Name.t, Location.t * string) Reactive.t;
33+
(** Modules where all declarations are dead. Value is (loc, fileName). Reactive anti-join. *)
3434
dead_decls_by_file: (string, Decl.t list) Reactive.t;
3535
(** Dead declarations grouped by file. Reactive per-file grouping. *)
3636
issues_by_file: (string, Issue.t list * Name.t list) Reactive.t;
@@ -83,11 +83,15 @@ let create ~(decls : (Lexing.position, Decl.t) Reactive.t)
8383
~f:(fun _ _ -> [])
8484
()
8585
else
86-
(* modules_with_dead: (moduleName, loc) for each module with dead decls *)
86+
(* modules_with_dead: (moduleName, (loc, fileName)) for each module with dead decls *)
8787
let modules_with_dead =
8888
Reactive.flatMap ~name:"solver.modules_with_dead" dead_decls
89-
~f:(fun _pos decl -> [(decl_module_name decl, decl.moduleLoc)])
90-
~merge:(fun loc1 _loc2 -> loc1) (* keep first location *)
89+
~f:(fun _pos decl ->
90+
[
91+
( decl_module_name decl,
92+
(decl.moduleLoc, decl.pos.Lexing.pos_fname) );
93+
])
94+
~merge:(fun v1 _v2 -> v1) (* keep first *)
9195
()
9296
in
9397
(* modules_with_live: (moduleName, ()) for each module with live decls *)
@@ -99,10 +103,10 @@ let create ~(decls : (Lexing.position, Decl.t) Reactive.t)
99103
(* Anti-join: modules in dead but not in live *)
100104
Reactive.join ~name:"solver.dead_modules" modules_with_dead
101105
modules_with_live
102-
~key_of:(fun modName _loc -> modName)
103-
~f:(fun modName loc live_opt ->
106+
~key_of:(fun modName (_loc, _fileName) -> modName)
107+
~f:(fun modName (loc, fileName) live_opt ->
104108
match live_opt with
105-
| None -> [(modName, loc)] (* dead: no live decls *)
109+
| None -> [(modName, (loc, fileName))] (* dead: no live decls *)
106110
| Some () -> []) (* live: has at least one live decl *)
107111
()
108112
in
@@ -115,26 +119,12 @@ let create ~(decls : (Lexing.position, Decl.t) Reactive.t)
115119
()
116120
in
117121

118-
(* Reactive per-file grouping of value refs (for hasRefBelow optimization) *)
119122
let transitive = config.DceConfig.run.transitive in
120-
let value_refs_from_by_file =
121-
match value_refs_from with
122-
| None -> None
123-
| Some refs_from ->
124-
Some
125-
(Reactive.flatMap ~name:"solver.value_refs_from_by_file" refs_from
126-
~f:(fun posFrom posToSet ->
127-
[(posFrom.Lexing.pos_fname, [(posFrom, posToSet)])])
128-
~merge:(fun refs1 refs2 -> refs1 @ refs2)
129-
())
130-
in
131123

132124
(* Reactive per-file issues - recomputed when dead_decls_by_file changes.
133125
Returns (file, (value_issues, modules_with_reported_values)) where
134126
modules_with_reported_values are modules that have at least one reported dead value.
135-
Module issues are generated separately in collect_issues using dead_modules.
136-
137-
hasRefBelow now uses per-file refs: O(file_refs) instead of O(total_refs). *)
127+
Module issues are generated separately in collect_issues using dead_modules. *)
138128
let issues_by_file =
139129
Reactive.flatMap ~name:"solver.issues_by_file" dead_decls_by_file
140130
~f:(fun file decls ->
@@ -153,23 +143,16 @@ let create ~(decls : (Lexing.position, Decl.t) Reactive.t)
153143
Hashtbl.replace modules_with_values moduleName ();
154144
None (* Module issues generated separately *)
155145
in
156-
(* Per-file hasRefBelow: only scan refs from this file *)
146+
(* hasRefBelow: check if decl has any ref from "below" (including cross-file refs) *)
157147
let hasRefBelow =
158148
if transitive then fun _ -> false
159149
else
160-
match value_refs_from_by_file with
150+
match value_refs_from with
161151
| None -> fun _ -> false
162-
| Some refs_by_file -> (
163-
let file_refs = Reactive.get refs_by_file file in
164-
fun decl ->
165-
match file_refs with
166-
| None -> false
167-
| Some refs_list ->
168-
List.exists
169-
(fun (posFrom, posToSet) ->
170-
PosSet.mem decl.Decl.pos posToSet
171-
&& DeadCommon.refIsBelow decl posFrom)
172-
refs_list)
152+
| Some refs_from ->
153+
(* Must iterate ALL refs since cross-file refs also count as "below" *)
154+
DeadCommon.make_hasRefBelow ~transitive
155+
~iter_value_refs_from:(fun f -> Reactive.iter f refs_from)
173156
in
174157
(* Sort within file and generate issues *)
175158
let sorted = decls |> List.fast_sort Decl.compareForReporting in
@@ -210,15 +193,19 @@ let create ~(decls : (Lexing.position, Decl.t) Reactive.t)
210193
let dead_module_issues =
211194
Reactive.join ~name:"solver.dead_module_issues" dead_modules
212195
modules_with_reported
213-
~key_of:(fun moduleName _loc -> moduleName)
214-
~f:(fun moduleName loc has_reported_opt ->
196+
~key_of:(fun moduleName (_loc, _fileName) -> moduleName)
197+
~f:(fun moduleName (loc, fileName) has_reported_opt ->
215198
match has_reported_opt with
216199
| Some () ->
217200
let loc =
218201
if loc.Location.loc_ghost then
219-
let pos_fname = loc.loc_start.pos_fname in
220202
let pos =
221-
{Lexing.pos_fname; pos_lnum = 0; pos_bol = 0; pos_cnum = 0}
203+
{
204+
Lexing.pos_fname = fileName;
205+
pos_lnum = 0;
206+
pos_bol = 0;
207+
pos_cnum = 0;
208+
}
222209
in
223210
{Location.loc_start = pos; loc_end = pos; loc_ghost = false}
224211
else loc
@@ -245,18 +232,20 @@ let create ~(decls : (Lexing.position, Decl.t) Reactive.t)
245232

246233
(** Check if a module is dead using reactive collection. Returns issue if dead.
247234
Uses reported_modules set to avoid duplicate reports. *)
248-
let check_module_dead ~(dead_modules : (Name.t, Location.t) Reactive.t)
235+
let check_module_dead ~(dead_modules : (Name.t, Location.t * string) Reactive.t)
249236
~(reported_modules : (Name.t, unit) Hashtbl.t) ~fileName:pos_fname
250237
moduleName : Issue.t option =
251238
if Hashtbl.mem reported_modules moduleName then None
252239
else
253240
match Reactive.get dead_modules moduleName with
254-
| Some loc ->
241+
| Some (loc, fileName) ->
255242
Hashtbl.replace reported_modules moduleName ();
256243
let loc =
257244
if loc.Location.loc_ghost then
245+
(* Use fileName from dead_modules, fallback to pos_fname *)
246+
let fname = if fileName <> "" then fileName else pos_fname in
258247
let pos =
259-
{Lexing.pos_fname; pos_lnum = 0; pos_bol = 0; pos_cnum = 0}
248+
{Lexing.pos_fname = fname; pos_lnum = 0; pos_bol = 0; pos_cnum = 0}
260249
in
261250
{Location.loc_start = pos; loc_end = pos; loc_ghost = false}
262251
else loc

0 commit comments

Comments
 (0)