feat: per-module narrowing for Melange consumers#14732
Draft
robinbb wants to merge 4 commits into
Draft
Conversation
Lifts the [can_filter] Melange opt-out installed by #14516 (L4). The BFS narrowing pipeline now activates for Melange consumer compiles on the same terms as OCaml — same [Lib_index], same wrapped-lib soundness recovery, same [must_glob_set] / [tight_set] split. L4 routed Melange through [deps_of_entries] (broad) on the rationale that the narrowing had been developed and tested with OCaml-mode in mind. The defensive gate has no remaining technical justification: the cross-library walk reads [ocamldep] output (mode-agnostic), the classification pipeline is keyed on [Lib_index] entries (already populated for libs compiled in either mode), and the only adjustment needed in [deps_of_entry_modules] is a symmetric [want_cmj] arm to round out per-module deps for [Melange Cmj] consumers. [Module_compilation.lib_deps_for_module]: drop the [match Lib_mode.of_cm_kind cm_kind] arm from [can_filter]. The guard collapses to the four mode-independent predicates ([dep_graph] is real, the consumer module is in it, the module kind is filterable, the cctx is neither virtual nor a parameter). [Dep_rules.rules]: drop the [|| Compilation_mode.equal for_ Melange] disjunct from the singleton-stanza short-circuit. Single-module Melange stanzas with library deps now produce a real dep graph (the per-module filter consumes it). Comment reworded to drop the stale "Melange [can_filter]" forward-reference. [Lib_file_deps.deps_of_entry_modules]: add a [want_cmj] arm symmetric with [want_cmx]. For [cm_kind = Melange Cmj] consumers, per-module deps now include the dep lib's [.cmj] in addition to its [.cmi]; mirrors [groups_for_cm_kind]'s [Melange Cmj -> [Cmi; Cmj]] expansion used by the broad-dep path. The mli prose is rewritten — the "future Melange caller" caveat is gone, replaced with a positive description of all four [cm_kind] cases. [doc/dev/per-module-narrowing.md]: drop the two "Melange paths bypass narrowing" statements; the [can_filter] predicate list no longer includes an [Ocaml _] arm. Rewrite the trailing-section paragraph to describe Melange's [want_cmj] participation. Soundness: the only edge surfaced by this layer is the alias-form issue already documented by [wrapped-internal-leak.t] on L4 — a consumer referencing [Foo__Bar] directly via the wrapped-lib mangled-form alias. GitHub code search on 2026-05-25 found zero matches for [= Core__], [= Async__], [= Ppxlib__], [= Bonsai__] outside Melange's own test suite; the pattern is essentially absent in the OCaml ecosystem. Melange's [test/unit-tests/ounit_unicode_tests.ml] is the one known caller and is fixed in melange-re/melange to route through the supported [Melange_ppx.String_interp] wrapper. Tests: six existing Melange cram tests are promoted for output drift under the new path: - [depend-on-installed.t], [emit-installed.t], [emit-installed-two-modes.t], [emit-private.t]: +1 [ocamldep (internal)] line in [--display=short] trace. - [melange-conditional-modules.t]: +1 [ocamldep (internal)] + reorder of [ocamlc helper] vs [melc foo] in the build trace. No artefact change. - [missing-melc.t]: duplicated [melc not found] error block — the narrowing now triggers the consumer's per-module compile, which fails with the same melc-missing error. Exit code unchanged. Stack: rebases on #14521 (L9). Part of #14492. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
Collaborator
Author
|
@anmonteiro Here is the Melange layer for 14492. |
[Melange_rules.compile_rules_for_target] was passing [~pps_runtime_libs:(Resolve.Memo.return [])] as a placeholder. Pre-L10 this was inert because the Melange compile path skipped narrowing entirely. With the gate-lift in this PR, the narrowing pipeline now reads [Compilation_context.pps_runtime_libs] and force-globs those libs into [must_glob_set] — but the empty placeholder dropped every ppx_runtime_libs declaration on melange.emit stanzas. Symptom: [melange-pps-runtime-libs.t] under [per-module-lib-deps/] loses the [*.cmi]/[*.cmj] globs over the runtime lib's melange objdir, exposing a soundness regression: a [(melange.emit ...)] stanza preprocessing with a ppx whose [(ppx_runtime_libraries ...)] points at a lib that's never named in source would silently drop that lib's cmi/cmj from the compile-rule deps. Compute [pps_runtime_libs] from [compile_info] the same way [Lib_rules] and [Exe_rules] do: let* pps = Lib.Compile.pps compile_info ~for_ in Resolve.Memo.List.concat_map pps ~f:(Lib.ppx_runtime_deps ~for_) The L5 soundness recovery for ppx_runtime_libs now applies to Melange consumers as well. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
Every OCaml/Melange compile implicitly opens [Stdlib] before reading its source. The implicit open is not visible in [ocamldep -modules] output (ocamldep reports only modules named in source), so it never appears in the BFS frontier and the lib that provides [Stdlib] is dropped from [kept_libs] under L6's filtered-include-flags path. For OCaml-mode compiles this is typically invisible: [Stdlib] lives on the compiler's built-in include path (via [+stdlib]); no dune-managed lib provides a module named [Stdlib], so the [Lib_index] lookup for [Stdlib] is empty and no narrowing decision depends on it. For Melange-mode compiles (post-L10), [Stdlib] lives in the [melange]/[stdlib] dune library, and dropping its [-I] from a consumer's compile rule causes [Error: Unbound module Stdlib] at the [-open] resolution step the moment that consumer's source references any [Stdlib] type ([int], [float], etc.) without syntactically naming a [Stdlib.X] member. Force [Stdlib] onto the initial BFS frontier in [lib_deps_for_module] alongside [all_raw] and [open_modules]. The injection is mode-agnostic — the [Lib_index] lookup decides what (if anything) to include: - Typical OCaml project: no dune lib named [Stdlib], lookup is empty, zero effect. - Melange project: pulls in the [melange]/[stdlib] lib via the normal classification path. Fixes the Stdlib-resolution bug. - Unwrapped OCaml lib that exports a [Stdlib] module (rare): the lookup hits and that lib's [-I] + [Stdlib.cmi] are kept in every consumer's compile rule. Matches the broad-dep path's behavior pre-narrowing — slight precision loss (extra dep) but soundness preserved. - Wrapped lib with a [Stdlib.ml] sibling: index records only the wrapper name, lookup is empty, no effect. Reproduced and fixed against: - belt unit lib (melange 6.0.1-54): rebuild from scratch under L10 dune now produces all [belt__Belt_*.cmi]/[cmj] artefacts (was failing with [Error: Unbound module Stdlib] on every [.pp.mli] whose source did not syntactically name [Stdlib.X]). - #14732 CI: macOS opam [Install deps] step failed compiling melange.6.0.1-54 with hundreds of identical errors, same root cause. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
There was a problem hiding this comment.
Pull request overview
Enables the per-module narrowing (ocamldep BFS) dependency pipeline for Melange consumer compilations, bringing Melange behavior in line with OCaml consumers by producing per-module .cmj deps (in addition to .cmi) and removing prior Melange-specific bypasses.
Changes:
- Remove the Melange opt-out so
Module_compilation.lib_deps_for_modulecan narrow library deps for Melange consumers, and forceStdlibinto the reference frontier to avoid dropping Melange stdlib deps. - Extend
Lib_file_deps.deps_of_entry_modulesto emit per-module.cmjdeps forMelange Cmj, symmetric with.cmxforOcaml Cmx. - Adjust dep-graph generation and update Melange blackbox cram outputs to account for extra
ocamldep (internal)traces.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/dune_rules/module_compilation.ml |
Allows narrowing for Melange and ensures Stdlib is considered referenced so Melange stdlib libs aren’t accidentally dropped. |
src/dune_rules/dep_rules.ml |
Removes the Melange singleton-stanza short-circuit so dep graphs are available for narrowing when library deps exist. |
src/dune_rules/lib_file_deps.ml |
Adds per-module .cmj file deps for Melange Cmj consumers. |
src/dune_rules/lib_file_deps.mli |
Updates the interface documentation to reflect Melange .cmj per-module deps. |
src/dune_rules/melange/melange_rules.ml |
Plumbs pps_runtime_libs into Melange compilation contexts (needed for narrowing soundness). |
doc/dev/per-module-narrowing.md |
Updates the algorithm reference to describe Melange participation in narrowing. |
test/blackbox-tests/test-cases/melange/missing-melc.t |
Promotes expected output (now includes an additional reported location/error instance). |
test/blackbox-tests/test-cases/melange/melange-conditional-modules.t |
Updates trace output for additional internal ocamldep invocation ordering. |
test/blackbox-tests/test-cases/melange/emit-private.t |
Promotes trace output to include additional ocamldep (internal) line. |
test/blackbox-tests/test-cases/melange/emit-installed.t |
Promotes trace output to include additional ocamldep (internal) line. |
test/blackbox-tests/test-cases/melange/emit-installed-two-modes.t |
Promotes trace output to include additional ocamldep (internal) line. |
test/blackbox-tests/test-cases/melange/depend-on-installed.t |
Promotes trace output to include additional ocamldep (internal) line. |
The Melange paragraph claimed the only mode-aware code was in [Lib_file_deps.deps_of_entry_modules], but [need_impl_deps_of] (inside [Module_compilation.lib_deps_for_module]) also matches on [cm_kind] and explicitly names [Melange _] in the pattern. The behaviour there is "Cmx reads trans-dep impl ocamldep; everything else does not" — [Melange _] is in the same bucket as [Ocaml (Cmi | Cmo)], not its own — but the doc oversimplified to the point of inaccuracy. Rewrite as a numbered list naming both [cm_kind] match sites and clarifying that the second site's distinction is Cmx-vs-rest, not OCaml-vs-Melange. Replies to #14732 review comment r3304097840 (Copilot). Signed-off-by: Robin Bate Boerop <me@robinbb.com>
Collaborator
Author
|
@anmonteiro Do you think it's worth fixing the "double missing melc" error problem in this PR? |
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
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.
Summary
Lifts the
can_filterMelange opt-out installed by #14516 (L4). The BFS per-module narrowing pipeline now activates for Melange consumer compiles on the same terms as OCaml — sameLib_index, same wrapped-lib soundness recovery, samemust_glob_set/tight_setsplit.Module_compilation.lib_deps_for_moduledrops thematch Lib_mode.of_cm_kind cm_kindarm fromcan_filter.Dep_rules.rulesdrops the|| Compilation_mode.equal for_ Melangedisjunct from the singleton-stanza short-circuit.Lib_file_deps.deps_of_entry_modulesgains awant_cmjarm symmetric withwant_cmx, soMelange Cmjconsumers see per-module.cmjdeps in addition to.cmi.Six Melange cram tests are promoted for output drift (additional
ocamldep (internal)trace lines + one duplicatedmelc not founderror inmissing-melc.t). No artefact changes; same exit codes.Stack
Rebases on #14521 (L9). Part of #14492.
Validation
test/blackbox-tests/test-cases/melange/*.tpass after promotion.test/unit-tests/ounit_unicode_tests.mlre-routed throughMelange_ppx.String_interp(a 3-line patch that's a no-op for upstream Melange and that the project is welcome to absorb).module String_interp is an alias for module Melange_ppx__String_interp, which is missing— the same alias-form issue already documented bywrapped-internal-leak.ton L4. The pattern is essentially absent outside Melange's own ppx (zero GitHub code-search hits for= Core__,= Async__,= Ppxlib__,= Bonsai__).Fixes
Part of #14492.