Fix DCE false positives for functors constrained by a named module type - #214
Fix DCE false positives for functors constrained by a named module type#214JonoPrest wants to merge 41 commits into
Conversation
Values in a functor body whose result is constrained to a module type defined in another module (via .mli or inline) are reported dead even when instances are used from live code.
When a module or functor result is constrained to a module type defined in another file (via .mli or inline), the typed tree's declaration dependencies link each implementation value to the `val` item of that module type, which lives in another compilation unit's cmt. The extractor only loaded the local cmt and cmti, so those links were dropped and every value in the functor body was reported dead. - Index all cmt/cmti files by compilation unit up front and resolve foreign uids from them (OCaml 5.3+). - Defer processing of value dependencies until all files are scanned. When the signature side is not a declaration (a `val` inside a named module type), forward the references made to it onto the implementation rather than treating it as an always-live reference, so unused items in the signature are still reported.
8c03985 to
3d8c667
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d8c667b7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- In ReScript project mode, collect the cmt files of every source directory before processing any, so declaration dependencies across directories resolve. - When forwarding references from a module-type val item onto its implementation, also forward the recorded optional-argument calls. Otherwise a call like H.f ~x:1 () through a constrained functor result would report x as never used. Covered by with_opt in the fixture.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa6d29aed9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Forwarding references from a module type's val item to its implementations keeps every implementation of a shared signature live when only one is used. Instead, reduce each cross-unit identifier occurrence via Shape_reduce with a loader over the indexed cmt files, and reference the resolved implementation directly. The decision is deferred until all declarations are known so that references to .mli declarations are kept as before. The forwarding remains as a conservative fallback for occurrences the reducer cannot resolve. Adds FunctorSigOther: a second functor over the same module type that is never instantiated and must be reported dead.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c193b69e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Precise attribution through a shared named module type relies on shape reduction of cmt_ident_occurrences, available from OCaml 5.3. On earlier versions the conservative forwarding applies, as it did before this change, so the deadcode fixture (whose expected output is shared across all supported versions) cannot contain the case. Assert it in the regression suite only on OCaml >= 5.3.
…isely - Key identifier resolutions by full location and last name component, so distinct identifiers a ppx emits at the same position do not collide. - Also use occurrences the compiler resolved locally, so modules defined in the same file as their use are attributed precisely. - Route functor argument coercions (Tmod_apply) through the argument module's shape, so passing one of several modules implementing a shared named signature only credits that module. - Never forward references made through a functor parameter to the implementations of the module type item: the application sites credit the actual arguments. First-class module uses still fall back to forwarding. Regression cases: Shared_signature_arg covers the functor argument and same-file module cases.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2dfc05969
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…edit parameter calls - Key the per-unit cache by the files the unit is loaded from and prefer the unit built alongside the current file, so same-named units in different build directories (notably each executable's Dune__exe alias module) are not confused. - Only a non-parameter reference that stays targeted at the module type item re-enables forwarding for a binding; a shape-redirected one does not, since it never lands on the item. - Optional-argument calls made through a functor parameter are no longer forwarded to every implementation. Instead they are credited to the actual argument at each functor application, resolved through the argument's shape (or its signature as a fallback), once all files are scanned. Regression cases: a binding mixing a parameter call with a resolved call, and an optional argument supplied only through a functor parameter.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5bb8c69699
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…n by digest - Unwrap (M : S) constraints around functor arguments before resolving the argument's shape, and expand an explicit constraint by a named module type to that module type's signature: the typed tree leaves it as Mty_ident, which exposed no items and silently dropped every reference to the argument (a pre-existing gap that the previous inert signature reference happened to mask). - Key inline functors ((functor (M : S) -> ...) (A)) by their own position so calls through their parameters are credited at the application. - When the same unit name exists in several build directories and none is next to the consumer, select the candidate whose interface digest matches the consumer's recorded import. Regression cases: constrained argument (value and optional argument), inline functor.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22edc55b75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… functors, app identity - Include the consumer's import digest in the unit-info cache key. - A parameter call credited at an application whose argument resolves only to a module type item (no shape) is forwarded to that item's implementations rather than discarded. Without shape resolution at all (before OCaml 5.3) parameter references stay forwardable, as the functor cannot be identified. - Carry the module binding's key through a Tmod_constraint wrapper, so functors with a whole-functor signature credit their parameter calls. - Deduplicate curried applications by node identity, not position. Regression case: Apply_sig, a functor with an explicit functor signature.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de59edbd89
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- When the head of an application is a module bound to a partial application (module G = F (A); G (B)), follow the binding to the head functor and offset the argument index by the arguments already consumed, so calls through F's later parameters are credited to B. - Key functors bound with let module by their binding (Texp_letmodule before OCaml 5.5; Texp_struct_item goes through the Tstr_module hook from 5.5 on). Regression cases: Apply2 partially applied, Apply_local via let module.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 446daf58b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two problems specific to versions where let module is Texp_letmodule: - The mapper visits a binding's body before its module expression, so a nested let module overwrote the single pending-key slot. Functor keys are now a list keyed by node identity. - Such bindings are not registered as module binding declarations, so applications could not find them by uid. They are also keyed by their identifier. Also adds debug logging of functor applications and parameter calls.
Parameter aliases in a recursive module group are registered to a fixed point, so module rec G = H and H = M inside a functor makes G stand for the parameter M too. Regression case: Apply_recfwd.
|
@codex review |
…Caml 5.5) Found by sweeping parameter-alias forms: on 5.5 a let module is a Texp_struct_item whose body the mapper visits before the item, so an alias chain (let module G = M in let module H = G in H.g ~x:1 ()) left H unregistered and its call forwardable to every implementation of the module type item. The alias is now registered in the expression hook before the body. Regression cases: Apply_sw1..Apply_sw4 (recursive alias of a parameter submodule, let-module alias chain, recursive alias of an outer alias, alias passed as a functor argument), each credited exactly once, with Opt_sw_other keeping its never-used warning.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30f8eb154d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… source paths - Two functor definitions a ppx emitted at one position share a key. Rather than dropping the calls through their parameters (which could report a supplied argument as unused), they are credited to the arguments of every application of that key, which is tighter than forwarding to every implementation. - The signature declaration filter resolves the recorded source file and positions against the cmt's build directory and compares exactly; no suffix matching.
|
@codex review |
Found by sweeping the exact-path filter: a position path may carry ./ or ../ segments the recorded source file does not, which Filename.concat keeps. Segments are now normalized before comparing.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ac1335675
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let loaded = | ||
| match | ||
| loaded |> List.map (fun (p, _) -> Filename.dirname p) | ||
| |> List.sort_uniq compare | ||
| with | ||
| | _ :: _ :: _ -> [] | ||
| | _ -> loaded |
There was a problem hiding this comment.
When -dce-cmt scans a broad Dune root such as _build, a public library's CMT can appear both in its object directory and under _build/install; both copies have the same unit name and interface digest but different parent directories. This branch consequently replaces the valid candidates with [], so foreign UID and shape lookup silently fails and the cross-unit functor false positives addressed by this change return. Deduplicate identical artifacts, such as by real path or compilation-unit identity, before treating multiple directories as ambiguous.
Useful? React with 👍 / 👎.
| let segments = | ||
| String.split_on_char '/' (Filename.dirname path) | ||
| |> List.fold_left |
There was a problem hiding this comment.
On Windows, Filename.dirname produces backslash-separated paths, so splitting only on / leaves . and .. components untouched. If cmt_sourcefile and a location spell the same source with different relative segments—the exact variation this normalization is meant to handle—they compare unequal, causing local signature values to be skipped as foreign and corrupting dead/live results. Use a platform-aware path canonicalizer or handle both directory separators.
Useful? React with 👍 / 👎.
…arators A broad root such as _build holds copies of the same artifact: a library's objects and its _build/install copy, byte and native objects. - Scanning: a unit (by source file, build directory, interface digest) is scanned once; scanning copies recorded every application twice, multiplying optional-argument call counts (4096 through the ten-functor chain). - Loading: copies of one source are one candidate, so they no longer count as ambiguous directories and lose cross-unit resolution. - The source-file comparison normalizes both separators. The regression runner now scans a layout with dependencies duplicated in two directories and consumers in a third, and requires the same result as the single directory.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 110d0beee5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| match !identResolutions.moduleTypeOf lid.loc (Path.last path) with | ||
| | Some moduleType -> expand moduleType | ||
| | None -> expand argumentExpr.mod_type) |
There was a problem hiding this comment.
On OCaml 5.3+, when the analyzed CMT root omits the unit defining a named signature and a functor receives an inline constrained argument such as F ((struct let g ?x () = ... end : External.S)), moduleTypeOf returns None and this branch retains the unresolved outer Mty_ident instead of inspecting the inner structure's concrete type. Because an anonymous argument also has no identifier shape, both coercion traversal and argumentItemResolver then see no values, so uses through the functor parameter are not credited to g and it can be reported dead or its supplied optional argument reported unused. The None case should fall back through argumentModuleType inner.
Useful? React with 👍 / 👎.
When an argument's constraint names a module type whose unit is outside the analysis root, the type of the module under the constraints is used instead, with all of its values referenced since the coercion computed against the constraint does not apply to it. Regression cases: Apply_ord applied to an inline and to a named module constrained by Set.OrderedType.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d7d18dc00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| moduleTypeRangesOfModuleExpr inner @ moduleTypeRangesOfModuleType mty | ||
| | Tmod_constraint (inner, _, _, _) -> moduleTypeRangesOfModuleExpr inner | ||
| | Tmod_functor (_, body) -> moduleTypeRangesOfModuleExpr body | ||
| | _ -> [] |
There was a problem hiding this comment.
When a binding applies an inline functor whose result contains a module-type declaration, such as module M = (functor (A : S) -> struct module type T = sig val f : unit -> int end; module X : T = ... end) (Arg), the enclosing Tmod_apply falls through this wildcard and the range of T is never collected. The flattened signature can then register T.f as the concrete declaration of M.X.f, preventing dependency forwarding to the implementation and allowing that implementation to be reported dead despite a live call. Recurse into at least the functor side of Tmod_apply (and the argument as well when its members can flow into the result).
Useful? React with 👍 / 👎.
| | Tmod_constraint (inner, _, Tmodtype_explicit mty, _) -> | ||
| moduleTypeRangesOfModuleExpr inner @ moduleTypeRangesOfModuleType mty | ||
| | Tmod_constraint (inner, _, _, _) -> moduleTypeRangesOfModuleExpr inner | ||
| | Tmod_functor (_, body) -> moduleTypeRangesOfModuleExpr body |
There was a problem hiding this comment.
When an inline functor parameter declares a nested module type and the result exposes the parameter, for example module F (M : sig module type T = sig val f : unit -> int end; module X : T end) = struct include M end, this branch scans only the body. The resulting F.X.f signature item can retain the location of T.f, but that location is absent from moduleTypeRanges, so it is mistaken for an implementation declaration and references are not forwarded to concrete arguments. Traverse the named parameter's module type here and in the corresponding Tmty_functor branch.
Useful? React with 👍 / 👎.
Module types declared in an inline applied functor's body, in an argument that flows into the result, and in a functor parameter's type (both Tmod_functor and Tmty_functor) are now excluded from signature declarations; the val items of a parameter's module type were registered as implementations of every application. Regression cases: M_app, F_param applied to Arg_p and, unused, to Arg_p2, whose application must produce no report.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e798e5b51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ctor keys Constraints are unwrapped wherever a functor key is derived from a binding's right-hand side: recursive bindings of a constrained partial application or alias (module rec G : FT = (F (A) : FT)), and, before OCaml 5.5, let-module bindings of a constrained functor, whose key was the inline position while its parameter calls were keyed by the binding. Regression cases: G_c, run_letc.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1842d2483
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Generative applications ([F () (Arg)], [Tmod_apply_unit] on OCaml 5.1+) now consume a parameter index in application chains: in functor keys, recorded applications, recursive bindings, and in the resolver's head, member and module type walkers. An argument that is itself an application ([Use (Make (A))]) gets the shape of the application, built from the functor's and the argument's, so calls and coercion references land in the applied functor's body rather than on every implementation of its result signature. Covered: direct, constrained, curried, nested, generative, through an alias of the functor, bound in another file, with a functor parameter as the inner argument, and an applied functor re-exporting its argument. When no shape is available (an inline functor, a constrained inline structure), values are found by name in the concrete type of the module.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b1c3e16bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| | Tmod_apply_unit functorExpr -> appliedBody functorExpr (applied + 1) | ||
| #endif | ||
| | Tmod_functor (_, body) when applied > 0 -> appliedBody body (applied - 1) | ||
| | Tmod_functor _ | Tmod_ident _ -> None |
There was a problem hiding this comment.
When an applied inline functor returns or re-exports its parameter, such as Use ((functor (A : S) -> A) (Chosen)), this branch discards the body instead of preserving the application's actual argument. Because moduleShapeOfExpr also cannot construct the inline functor's shape, argumentItemResolver falls back to the shared S signature item; on OCaml 5.3+, calls through Use are consequently attributed to every implementation of S, keeping unrelated values live and suppressing their unused-optional-argument diagnostics. Preserve the consumed parameter's actual shape or resolver for Tmod_ident result bodies.
Useful? React with 👍 / 👎.
| | _ -> ()); | ||
| call | ||
| |> DeadOptionalArgs.addReferences ~locFrom ~locTo ?locToImpl | ||
| ~forwardable:(parameter = None || not Compat.shapeResolutionAvailable) |
There was a problem hiding this comment.
On OCaml 5.3+, a functor applied through a first-class-module unpack, for example module G = (val packed : FT) followed by G (Arg), has its application keyed to G because the Tmod_unpack definition cannot be chased, while calls through the original functor parameter remain keyed to the packed functor's definition. No delayed application therefore matches those calls, and marking them permanently non-forwardable here prevents the conservative signature dependency from crediting Arg.g ~x:1 (), so x can be reported as never used. Defer this decision until matching applications are known, or restore forwarding for unmatched functor keys.
Useful? React with 👍 / 👎.
… keys A functor parameter applied in the body ([F (M)] in [Outer (F) (M)]) now records an application headed by the parameter, resolved once all files are scanned to the functors passed for it, through as many levels as needed, and paired with the arguments of the same enclosing application. Covered: the parameter applied to a parameter, a fixed module, a submodule of a parameter, through an alias, in an include, a partially applied functor argument, two levels, and the same functor applied twice. First-class modules bound with [let packed = (module M : S)] are resolved through a table filled where they are bound, as functors ([(val packed) (Arg)]) and as arguments, from any file. Other ways of obtaining one, and functors passed to applications whose head cannot be chased, mark the functor as escaping: its parameter calls are then forwarded to every implementation of the item rather than dropped. A functor never applied and not escaping credits nothing. An application of an inline functor whose body returns (a submodule of) a parameter, or applies a functor to it, resolves to the actual argument. The regression debug output now exceeds node's default buffer on older compilers; the runner raises it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 319ce460c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| |> List.iter (fun {call} -> | ||
| DeadOptionalArgs.addCallToImplementation | ||
| ~posTo:locTo.loc_start call)) | ||
| else if List.mem def escaped then |
There was a problem hiding this comment.
When a functor has at least one directly resolved application but also escapes through an untracked first-class-module flow, this else if suppresses the conservative forwarding required for the unresolved applications. For example, after both F (A) and packing F, an alias produced by a function can be unpacked and applied to B; the known application makes hasApplication true, so the parameter call is credited only to A and an optional argument supplied by the call through F can incorrectly be reported unused on B. Escaped calls need fallback handling even when some applications are known.
Useful? React with 👍 / 👎.
1 participant
When a functor result is constrained to a module type defined in another file, every value in the functor body is reported dead even when instances are used from live code.
Before this change:
Expected: only
unused_in_sigandtruly_dead.On OCaml 5.3+ the
find_optbinding inhash.mldepends on the uid ofval find_optinsidemodule type Singen.ml. The dependency extractor only loaded the local.cmtand.cmti, so the link was dropped. This is the missing case anticipated in c7661c5 when the cross-cmt uid table was removed.Fix:
valin a module type rather than a declaration, forward its references onto the implementation instead of treating it as always live, so unused signature items are still reported.Adds a fixture covering both
.mliand inline constraints. The first commit fails on its own, the second makes it pass. Over the ReScript compiler, dead values drop from 1563 to 1089.