Repository navigation
Fix DCE false positives found on the ReScript compiler sources - #224
Conversation
scripts/dead_code.sh builds the native and the browser (playground) profiles and runs reanalyze over both, so uses in either count, and fails on any finding. Vendored flow_parser and the frozen parsetree0 are suppressed. It needs a reanalyze with rescript-lang/reanalyze#224, given as REANALYZE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT Signed-off-by: Christoph Knittel <ck@cca.io>
Running DCE on the ReScript compiler (OCaml 5.5, dune, native and browser profiles) produced several classes of false positives: - On case-insensitive file systems (macOS, Windows), a unit's sibling .cmt was found under two spellings, so the unit looked ambiguous and exceptions raised across modules of a wrapped library were reported as never raised. Sibling candidates now need an exact directory entry. - Type re-exports `type t = M.t = A | B` declare the same constructors and labels as `M.t`. They are now linked both ways (OCaml 5.3+, found through shapes), also when `M` is a local module alias, whose shape carries no uid of its own. - Types in a submodule of an interface (`module M : sig type t = A end`) were declared without the submodule path, so they never matched their implementation. - Several build trees of one project (e.g. two dune profiles) can now be analyzed together: sources found only in a build tree (generated files) are read from the nearest ancestor of the .cmt that contains them, and a unit compiled from the same source in several trees resolves to the copy strictly closest to the consumer. Ties stay ambiguous, as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT
Removing unused optional arguments from the ReScript compiler surfaced three kinds of wrong "optional argument is never used" reports: - Aliases and partial applications of a function with optional parameters (`let assert_equal = OUnit.assert_equal`) were reported, though they cannot drop them. Only a binding that is itself a function owns its optional parameters now. - Calls through such an alias or partial application (`let lookup_value = lookup proj1 proj2`, then `lookup_value ?loc`) were not credited to the function it stands for. They are now forwarded to it. - A function passed as an argument with its optional parameters intact (`find_component Env.lookup_type`, where `find_component` takes `(lookup : ?loc:_ -> _)` and calls `lookup ~loc`) had them reported as never used. They now count as possibly supplied. A function stored elsewhere, e.g. in a list, still does not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT
- A binding is an alias only when its body is an identifier or an application whose labels all come from the applied function; any other binding (let ... in fun, if ... then fun ...) owns its optional parameters again. - Aliases are resolved before calls are settled and forwarded, so an alias of a value of a signature-constrained module counts for its implementation; an alias exported through an .mli is an alias too. - A function passed as an argument counts its optional parameters as possibly supplied only when the callee declares that parameter as a function with optional parameters; ignore f or ref f supplies nothing. - Re-export links apply only between declarations of the analysis: a re-export of a stdlib type no longer makes its constructors live. - Warn when one source path names different files in different build trees; their declarations cannot be told apart. - With -native-build-target, reports name absolute paths again. - Parse each .cmt at most once (memory on the ReScript compiler with two build trees: 5.0 GB -> 2.0 GB), memoize the ancestor source lookup, and skip the re-export lookup for abbreviations. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT
… state - A binding is an alias only when its optional parameters are those the applied function leaves after the application (skipped optional parameters included), not ones that merely share a name: in let g = wrap f with wrap : ?x:_ -> 'a -> 'a, g's ?x comes from f. - A function passed to a parameter of a named type (type lookup = ?loc:int -> int -> int) keeps its optional parameters. The argument is typed with the parameter's type, so its labels are read from its declaration. - OptionalArgs.empty was one mutable record shared by every function without optional parameters, so a call through one changed the state of all; it is now created fresh. - A file found from the current directory is also compared with each build tree's copy, and the warning names the two files. - The compilation context reads .cmt files through the shared cache (ReScript compiler, two trees: 2.0 GB -> 1.85 GB). - Changes.md documents the remaining limits: identifier callees only, and partial applications never make an argument "always supplied". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT
e202796 to
100f28e
Compare
- "Always supplied" is limited to labels the function owns: a call through an application that is not an alias (let g = wrap f) no longer reports g's labels. - Labels hidden by an abbreviation (type fn = ?x:int -> unit -> int): calls are recorded by their own optional labels, not only when the callee's type shows them; let g = f is an alias also when constrained (let g : fn = f); and a val f : fn in an interface stands for the implementation instead of hiding all of its reports. On the ReScript compiler this finds Ast_literal.type_bool's never-passed ?loc. - A current-directory file that differs from its build copy is reported as an out-of-date build, without the copy# advice. - compilationContext reuses parsed .cmt files but no longer keeps the ones it only probes (ReScript compiler, two trees: 1.85 GB -> 1.65 GB). - Changes.md: partial applications are only exempt from "always supplied" when they stand for the function; bindings that do not stand for it credit nobody. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT
The interface declared type_bool with the core_type_lit abbreviation, which hid its optional ?loc from reanalyze; with the abbreviation fix in reanalyze (rescript-lang/reanalyze#224) it is reported: the only caller, bs_builtin_ppx, passes no location. Before: val type_bool : core_type_lit (?loc:Location.t -> unit -> _) After: val type_bool : unit -> Parsetree.core_type Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT Signed-off-by: Christoph Knittel <ck@cca.io>
scripts/dead_code.sh builds the native and the browser (playground) profiles and runs reanalyze over both, so uses in either count, and fails on any finding. Vendored flow_parser and the frozen parsetree0 are suppressed. It needs a reanalyze with rescript-lang/reanalyze#224, given as REANALYZE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT Signed-off-by: Christoph Knittel <ck@cca.io>
The interface declared type_bool with the core_type_lit abbreviation, which hid its optional ?loc from reanalyze; with the abbreviation fix in reanalyze (rescript-lang/reanalyze#224) it is reported: the only caller, bs_builtin_ppx, passes no location. Before: val type_bool : core_type_lit (?loc:Location.t -> unit -> _) After: val type_bool : unit -> Parsetree.core_type Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT Signed-off-by: Christoph Knittel <ck@cca.io>
… functions - An alias records the optional arguments its partial application passes (let p = f ~x:1), and every call forwarded through it passes them too, so x can be reported as always supplied. - A binding that applies a function without standing for it, but whose result has optional parameters (let g = wrap f, also with a callee that is not an identifier, (fun h -> h) f), credits the functions among its arguments as possibly receiving them, like functions passed to a parameter declared with optional parameters. - The supplied/maybe classification of an application's optional arguments is shared by calls and aliases (suppliedLabels). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT
- The partial application that forms an alias is no longer recorded as a call of its target as well: each call through the alias is one, and the application counts once only when nobody calls through it. Counts are exact again, and labels that every call through the alias passes (base 1 then partial ~t:1 2) are reported as always supplied for the function, where they were lost. - A binding that applies a function without standing for it credits only the arguments that have all of its optional parameters (second k f no longer hides k's unused ?z). - Simplifications: FindSourceFile drops the scan-root lookup, which the walk up from the .cmt already covers, and folds sourceDir into sourceExists; one resolveItem replaces resolveAliases and its copy in forceDelayedItems (aliases are declarations, so settling never needs them resolved); collectValueBinding's alias logic is one match; the function-argument walk is hoisted out of collectExpr and shares one writer with creditFunctionArguments; smaller cleanups. - Changes.md states how alias calls are counted and lists the limits, including aliases of functor-parameter values. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT
e349793 skipped the application that forms an alias by its start position and added a synthetic call when nobody called through the alias. Both broke: - An operand starting at the same position was skipped too: in let p = h ~y:2 1 |> mk (typed as mk (h ~y:2 1)), h's ?y was reported as never used. - The synthetic call was made after calls were attributed to implementations, so for a value of a signature-constrained module or a functor parameter (let p = M.f ~x:1) it was dropped and M.f's ?x was reported as never used. The application is now recorded as an ordinary call again, tagged with its alias (keyed by its full location), and dropped at the end only when some call went through the alias. This also makes chains exact: an uncalled let q = p ~y:2 on top of let p = f ~x:1 counts once, with x and y. A sweep of the 52 review repros changes only these cases; fixtures cover all three and fail on e349793. Also: interface and implementation in FindSourceFile share one List.find_map. (Correction to e349793's message: the walk up from the .cmt covers the scan-root lookup except for dune's .ml-gen wrappers, which are deliberately not analyzed; reports are unchanged.) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT
let p = 1 |> f ~x:1 is typed as (f ~x:1) 1, an application of an application, so it was not an alias of f: calls through p counted for nobody and f's ?y was reported as never used. The alias check now looks through nested applications: residualLabels applies each argument group in turn (innermost first), and the innermost application, the one recorded as a call, is the alias's application. A sweep of all 67 review repros changes only r53 (this case); a fixture covers it and fails on bc4247e. Also: the fromAlias comment says what the field holds, and Changes.md lists the escaping-alias limit (an alias that escapes counts as not called through). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT
cristianoc
left a comment
There was a problem hiding this comment.
Approving, with one possible follow-up gap in optional-argument alias tracking:
type fn = ?x:int -> unit -> int
let plain ?x () = match x with Some x -> x | None -> 0
let first : fn = plain
let second = first
let () = print_int (second ~x:1 ())In DeadValue.ml, the Texp_ident branch registers an alias only when hasOptionalArgs vd.val_type succeeds, but that helper does not expand Tconstr. If first retains the abbreviated type fn, the second -> first link appears to be missed, potentially leaving plain's x reported as never used. Aliasing a value exported as val f : fn may have the same issue.
This is a source-level trace, not an executed reproduction. A regression test combining the abbreviated type with a second alias would help confirm it. Not blocking approval.
Review: let first : fn = plain followed by let second = first, and an alias of a value exported as val f : fn, were not aliases, because the alias was registered only when the aliased value's declared type showed optional parameters, which an abbreviation hides. plain's and f's ?x were then reported as never used. A binding that is just an identifier is now always an alias. An alias of a value without optional parameters receives no optional arguments, so it changes nothing. A sweep of all 69 review repros changes only the two reproductions of these cases; fixtures cover both and fail on e0e44e9. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT
|
Thanks! Both cases reproduced ( |
Releases the dead-code fixes of #224 (several build trees analyzed together, type re-exports, optional-argument aliases, functions passed as arguments, abbreviated function types), the move to an OCaml-only repository (#226-#228, #231, including the removal of @Gentype support) and the new minimum of OCaml 4.14 (#230). Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
scripts/dead_code.sh builds the native and the browser (playground) profiles and runs reanalyze over both, so uses in either count, and fails on any finding. Vendored flow_parser and the frozen parsetree0 are suppressed. It needs a reanalyze with rescript-lang/reanalyze#224, given as REANALYZE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT Signed-off-by: Christoph Knittel <ck@cca.io>
The interface declared type_bool with the core_type_lit abbreviation, which hid its optional ?loc from reanalyze; with the abbreviation fix in reanalyze (rescript-lang/reanalyze#224) it is reported: the only caller, bs_builtin_ppx, passes no location. Before: val type_bool : core_type_lit (?loc:Location.t -> unit -> _) After: val type_bool : unit -> Parsetree.core_type Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT Signed-off-by: Christoph Knittel <ck@cca.io>
The interface declared type_bool with the core_type_lit abbreviation, which hid its optional ?loc from reanalyze; with the abbreviation fix in reanalyze (rescript-lang/reanalyze#224) it is reported: the only caller, bs_builtin_ppx, passes no location. Before: val type_bool : core_type_lit (?loc:Location.t -> unit -> _) After: val type_bool : unit -> Parsetree.core_type Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT Signed-off-by: Christoph Knittel <ck@cca.io>
scripts/dead_code.sh builds the native and the browser (playground) profiles and runs reanalyze over both, so uses in either count, and fails on any finding. Vendored flow_parser and the frozen parsetree0 are suppressed. It needs a reanalyze with rescript-lang/reanalyze#224, given as REANALYZE. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT Signed-off-by: Christoph Knittel <ck@cca.io>
The interface declared type_bool with the core_type_lit abbreviation, which hid its optional ?loc from reanalyze; with the abbreviation fix in reanalyze (rescript-lang/reanalyze#224) it is reported: the only caller, bs_builtin_ppx, passes no location. Before: val type_bool : core_type_lit (?loc:Location.t -> unit -> _) After: val type_bool : unit -> Parsetree.core_type Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT Signed-off-by: Christoph Knittel <ck@cca.io>
Supersedes #222. (The CI fix it was stacked on, #223, is merged.)
Running DCE over the ReScript compiler's OCaml sources (OCaml 5.5.2, dune, native and browser profiles) produced several kinds of wrong reports. This PR fixes them. With it, the compiler's codebase analyzes to 0 findings (rescript-lang/rescript#9173,
make dead-code).Fixes
Each item: the wrong report, then its cause and the fix.
1. Exceptions "never raised" on macOS/Windows.
raise (Lib.Mod.Exn ...)from another module of a dune wrapped library didn't count.Cause: sibling
.cmtfiles are probed as bothUnit.cmtandunit.cmt. On a case-insensitive file system both exist, so one file showed up under two paths and the lookup gave up as "ambiguous".Fix: a probed sibling must exist under its exact name.
2. Type re-exports.
Building
Reexp.AleftOrig.A"never constructed"; readingx.fieldonOrig.tleft the re-export's label "never read".Fix: the re-exported declaration is resolved through shapes (OCaml ≥ 5.3), and same-named constructors and labels are linked both ways. A link applies only when both ends are declarations of the analysis, so re-exporting a stdlib type (
type 'a node = 'a Seq.node = Nil | Cons …) doesn't make its constructors live. This also works when the path goes through a local module alias, whose shape carries no uid, so the alias target is followed.3. Types in a submodule of an interface.
The constructors were registered as
setting.Autoinstead ofColor.setting.Auto, so they never matched the.ml, and the implementation's constructors looked unused.Fix: signature traversal passes the submodule path to type declarations.
4. Analyzing two build trees together. Needed so that a use in only one configuration counts, e.g. the ReScript playground, which only exists in the browser profile.
cmt_builddir). Generated sources that exist only in a build tree were skipped. They're now also looked up in the directories above the.cmt, and reports and-writeread them from there..cmt, which was treated as ambiguous. Now the copy in the consumer's own build tree is used (strictly closest path). Ties, and copies of different sources, stay ambiguous as before.copy#), instead of silently merging their declarations. A file that is also found from the current directory is compared with each tree's copy too; a difference there means the build is out of date, and the warning says so..cmtis parsed once and shared. On the ReScript compiler with two trees, peak memory goes from 5.0 GB to 1.65 GB.5. Optional arguments "never used" or "always supplied" by mistake.
let assert_equal = OUnit.assert_equal: an alias or partial application was reported for optional parameters it can't drop. A binding whose body is an identifier or an application of one is now an alias when its optional parameters are exactly those the applied function leaves (its declared type after the applied arguments, skipped optional ones included). Solet g = wrap fwithwrap : ?x:_ -> 'a -> 'ais not an alias ofwrap: its?xcomes fromf.let g = fis always an alias, also with a constraint naming its type (let g : fn = f), and so is a nested application (let p = 1 |> f ~x:1, typed as(f ~x:1) 1). That holds also when it's exported through an.mli. A function body (includinglet f = let y = … in fun ?x -> …) owns its optional parameters as before; any other application owns none, so calls through it are never "always supplied" for it, and a function among its arguments that has all of them (finwrap f) may receive them. Each call through a partial application that is an alias is a call of the function with the arguments the application passes (let p = f ~x:1); the application itself counts as a call only when nothing calls throughp.let lookup_value = lookup proj1 proj2: calls through an alias now count for the function it stands for. Aliases are resolved before calls are attributed to implementations, so this also works for values of modules constrained by a signature.find_component Env.lookup_type, wherefind_component (lookup : ?loc:_ -> _)callslookup ~loc: a function passed as an argument to a parameter that the callee declares as a function with optional parameters, or as a named type (type lookup = ?loc:int -> int -> int), now counts them as possibly supplied. Passing it to a parameter of type'a(ignore f,ref f) or storing it in a list supplies nothing, as the existingUnused_import.Victimtest expects.OptionalArgs.emptywas a single mutable record shared by every function without optional parameters, so a call through one of them changed the state of all of them (spurious "always supplied" reports). It is now created fresh each time.type fn = ?x:int -> unit -> int: calls through a value whose type is such an abbreviation were not recorded (the callee's type shows no labels), and aval f : fnin an interface hid all reports for the implementation. Calls are now recorded by their own optional labels, and such an interface item stands for the implementation. On the ReScript compiler this finds a real one:Ast_literal.type_bool's?locis never passed.Known limits, documented in
Changes.md: an alias that escapes (ignore p) counts as not called through, so with a chained alias the arguments only the chain passes are reported as always supplied; an alias needs the applied function to be an identifier (let q = (fun x -> x) pdoes not stand forp, thoughpmay receive what calls throughqpass); a function passed directly to a callee that is not an identifier is not credited; and calls through an alias of a functor parameter's value count for every implementation of the parameter's signature.New tests
In
examples/regression, with assertions inscripts/run_integration_tests.js:Type_reexport*: re-exports through another module and through a local alias. The assertions are gated on OCaml ≥ 5.3.Nested_sig_type*: types in an.mlisubmodule.Type_reexport_stdlib: a re-export of a type outside the analysis.Optional_arg_alias,Optional_arg_iface*,Optional_arg_passed*,Optional_arg_more: aliases (also through an.mliand of signature-constrained values), partial applications (also skipping an optional parameter, and passing one that is then always supplied), wrapped functions (wrap f,(fun h -> h) f, and an unrelated argument that stays reported), a partial application nobody calls through (also of a signature-constrained value, and an uncalled chain counting once), an operand starting where the alias's application starts (h ~y:2 1 |> mk), a pipe into a partial application (1 |> f ~x:1), functions passed as arguments (also to a named function type), and the cases that must stay reported (let … in fun,ignore f,ref f, a label that only shares a name with the applied function's).Optional_arg_abbrev*: an interface naming a function's type with an abbreviation;Optional_arg_morealso has a constrained alias.New script test
scripts/test_build_trees.js: one library built in two trees, each with its own generated module, analyzed together. It also checks the warning for the differing generated file, also when a copy is in the current directory.Each assertion fails without its fix (checked against 6309f4b and with the closest-copy rule disabled). Unused declarations in the same fixtures are still reported.
Fix 1 has no automated test: CI is Linux-only, and a case-insensitive file system can't be simulated there. On a macOS-backed mount, a wrapped-library repro went from 4 wrong "never raised" reports to 0.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Kbf3gLDo8mBJKpNE25WAwT