Fix DCE attribution for functors sharing a named module type - #218
Merged
Merged
Conversation
JonoPrest
added this pull request to stack #219
September 17, 2026 10:38
Contributor
Author
|
@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. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Contributor
Author
|
@codex review |
![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: 6c8dbaf647
ℹ️ 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".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bac61667ca
ℹ️ 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".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e128be390a
ℹ️ 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".
Contributor
Author
|
@codex review — gpt-6-astra via Codex (author) |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
JonoPrest
marked this pull request as ready for review
September 18, 2026 09:22
Contributor
Author
|
@cristianoc this opened a can of worms and an explosion of cases 😅 I've run reviews back and forth and made sure there's regression tests for everything that came up. Sorry for all the new code! Majority is regression tests. |
Collaborator
What model did the last review?
|
Contributor
Author
Between each commit - I used multi agent setup to review back and forth with astra on extra high. At stages I also did full clean branch review using Fable 5.1. Then when it was satisfied locally I triggered the codex review on the PR.
Did you see the PR stacked below this one? That is the stable one that only removes the false positive bugs.
Happy if they are experimental. All of these issues were exposed while trying to do DCE on the rescript compiler. |
Collaborator
|
Forgot it was on this repo.
Merge whenever you want -- you don't need permission right.
…On Fri, Sep 18, 2026 at 12:41 PM Jono Prest ***@***.***> wrote:
*JonoPrest* left a comment (rescript-lang/reanalyze#218)
<#218 (comment)>
What model did the last review? Btw I think we can be pragmatic here. We
want:
Between each commit - I used multi agent setup to review back and forth
with astra on extra high. At stages I also did full clean branch review
using Fable 5.1. Then when it was satisfied locally I triggered the codex
review on the PR.
1. Keep stable the existing features: should be pretty easy by
existing tests plus a dedicated review pass
Did you see the PR stacked below this one? That is the stable one that
only removes the false positive bugs.
2. Declare the new features experimental.
What do you think?
Happy if they are experimental. All of these issues were exposed while
trying to do DCE on the rescript compiler.
—
Reply to this email directly, view it on GitHub
<#218?email_source=notifications&email_token=AB4YVF6TPUCRGPGV62BEVZ35PUGNBA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNZSHA4DKMBYG42KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5728850874>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB4YVF24YAZ3CYWBIFS2MVL5PUGNBAVCNFSNUABFKJSXA33TNF2G64TZHMZDKNJSGM2DGMJVHNEXG43VMU5TKNBYGYZDCNZVG442C5QC>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Contributor
Author
Ok cool - I will do 🙏🏼 I don't need permission but would be good to just get sign off that you're aware of the changes and happy. Once this stack is fully in and seems to be working well I will try port the useful changes over to to rescript reanalyse. |
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.
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.
…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.
…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.
… 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.
- 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.
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.
…tions The whole family of module type paths inside functor bodies: - Outer (M).S, whatever the argument: the module type is found in Outer's body structurally. - M.T and M.Sub.T2 with M a parameter: found in the parameter's declared module type, which the functor declaration carries. - Through a parameter alias (module N = M; N.T) and through include of the parameter (include M; T). - When an argument's signature cannot be expanded at all, the values its shape exports are referenced, precise to that argument, rather than nothing. Regression cases: Apply_pt, Apply_app, Apply_pt2.
…source paths - A module type through an aliased or applied member of an applied functor's result (Outer (A).Alias.T, Outer (A).Applied.T) is followed in the unit defining the body. - Parameter-rooted module type expansion tracks visited paths instead of a fuel counter. - The signature declaration filter compares normalized full source paths (extensions and .pp dropped, path suffixes tolerated) rather than basenames, so same-named sources of different build targets are not confused. Regression cases: U_alias, U_applied.
When an argument's signature cannot be expanded, the values referenced from its shape now include those of nested modules at any depth, with aliases followed and recursive shapes guarded.
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.
…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.
… 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.
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.
…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.
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.
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.
…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.
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.
… 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.
JonoPrest
force-pushed
the
jono-functor-precise-attribution
branch
from
September 18, 2026 13:49
ec4bc76 to
7276e07
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Builds on #217. Forwarding references through a shared module type prevents false positives, but can also keep unrelated implementations live.
Before: the call keeps both
A.Make.fandB.Make.flive. Expected: onlyA.Make.fis live.The reference location points to
Gen.S.f, which both implementations satisfy. On OCaml 5.3+, compiler shapes let us resolve the reference to the implementation actuallyused.
Fix:
resolvable first-class modules.
Regression fixtures cover these cases, escaping functors, long and cross-file alias chains, and duplicated build layouts.
Validated with
dune buildandnpm teston OCaml 4.14.2, 5.2.1, 5.3.0, and 5.5.0.