feat: filter -I flags per-module to match dependency filtering#14473
Draft
robinbb wants to merge 4 commits into
Draft
feat: filter -I flags per-module to match dependency filtering#14473robinbb wants to merge 4 commits into
robinbb wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens OCaml compiler include flags (-I/-H) on a per-module basis so they match the already-computed per-module inter-library dependency filtering (from #14116 / follow-up to #14472). This closes a soundness gap (over-broad -I masking missing deps) and reduces cache-key churn (unreferenced libs no longer perturb compile rule hashes for unrelated modules).
Changes:
- Compute and inject per-module
-I/-Hinclude flags derived from the module’s filtered dependency result (with explicit fallbacks to cctx-wide flags when filtering can’t apply). - Add a per-compilation-context cache for filtered include-flag computations keyed by
(lib_mode, kept_libs)to reduce repeated work. - Update/promote blackbox observational tests and add a changelog entry documenting the new behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/blackbox-tests/test-cases/per-module-lib-deps/per-module-include-flags.t | Updates expected -I flags to only include the referenced dep library’s objdir. |
| test/blackbox-tests/test-cases/per-module-lib-deps/add-unreferenced-sibling-lib.t | Updates expected rebuild trace to show no rebuild when adding an unreferenced sibling library. |
| src/dune_rules/module_compilation.ml | Threads per-module include flags through lib_cm_deps and injects them via dynamic args, aligned with per-module dep filtering. |
| src/dune_rules/compilation_context.mli | Exposes filtered_include_flags API for per-module include flag generation. |
| src/dune_rules/compilation_context.ml | Implements a per-cctx cache and the filtered include-flag builder. |
| src/dune_lang/lib_mode.mli | Exposes equal, hash, to_string, to_dyn for Lib_mode.t (used for caching keys/debug). |
| src/dune_lang/lib_mode.ml | Implements hash and to_dyn for Lib_mode.t. |
| doc/changes/added/14186.md | Adds user-facing changelog entry describing per-module include filtering benefits. |
robinbb
force-pushed
the
robinbb-issue-4572-filtered-includes
branch
from
1e0dfeb to
7f349b4
Compare
robinbb
force-pushed
the
robinbb-issue-4572-filtered-includes
branch
from
7f349b4 to
1597830
Compare
robinbb
force-pushed
the
robinbb-issue-4572-combined-tests
branch
from
f528987 to
a73049c
Compare
robinbb
force-pushed
the
robinbb-issue-4572-filtered-includes
branch
from
1597830 to
a229748
Compare
robinbb
force-pushed
the
robinbb-issue-4572-filtered-includes
branch
2 times, most recently
from
408be36 to
a97c222
Compare
robinbb
force-pushed
the
robinbb-issue-4572-filtered-includes
branch
from
a97c222 to
f28a9ca
Compare
PR #14116 narrowed the [Dep.Set.t] of cmi/cmx artefacts each compile rule depends on, but the compiler invocation still received a cctx- wide [-I]/[-H] include list from [Compilation_context.includes]. Two consequences: 1. Soundness gap: a missing entry in the per-module dep set is silently masked because [ocamlc] can still resolve the symbol via a too-wide [-I] path. Sandboxed CI catches it; local [dune build] does not. 2. Cache key bloat: adding a library to a stanza changes every consumer module's compile-rule cache key (the [-I] list grew), invalidating modules that don't actually reference the new lib. Compute a per-module [Includes.t] inside [lib_deps_for_module]: filter [requires_compile] / [requires_hidden] by membership in the [kept_libs] set the existing dep classifier already produces, then build [Lib_flags.L.include_flags] from the filtered subsets. The direct/hidden split is preserved per-lib — a lib that was in [requires_hidden] keeps its [-H], one in [requires_compile] keeps its [-I]. Substitute the result for [Compilation_context.includes] in [build_cm]'s [Command.run] args via [Command.Args.Dyn]. Fallback to cctx-wide includes when [can_filter] is false (Melange, [has_virtual_impl], [Wrapped_compat], [Root], stanza-without- [ml_kind]-source) — same fallback shape as the dep filter. Extend [cross_lib_tight_set] to also report libs whose entry modules the walker bailed on because of preprocessing. ocamldep on the pre- preprocessing source can fail (the same blindspot covered by the existing [cross-lib-walk-pre-pp-source.t]), so the walker short-circuits there. Without compensation, transitive libs reachable only through such an entry's [.cmi] would be missing from the filtered include set; the consumer build then fails the moment it touches a type from one of those libs. Take [Lib.closure] of the stopped libs and union it into [must_glob_set], so each such dep is glob-included in both the rule's dep set and the per-module include flags. Guarded by [cross-lib-walk-pre-pp-implicit-transitive.t]. Cache the [Lib_flags.L.include_flags] result wrapped with [Command.Args.memo] in a top-level [Table.t] keyed on cctx [obj_dir] + [cm_kind] extension + sorted [kept_libs] lib names. Two consumers in the same stanza that share a kept-libs subset reuse one [Args.t] wrapper, so the underlying [include_paths] [Path.Map] computation runs once per distinct subset rather than once per consumer-rule. Mirrors the per-source memoisation in [Ocamldep.read_immediate_deps_words]. Promote two observational baselines added on main: * [per-module-include-flags.t]: now lists only [.dep_lib.objs/byte] on [consumer_module]'s compile rule (was listing [.unrelated_lib. objs/byte] too). * [add-unreferenced-sibling-lib.t]: rebuild lists for [consumes_dep] and [unrelated_module] are now [[]] (was one rule each). Untouched: merlin, [compile_commands.json], [ocaml_index], parameterised rules. They keep [cctx.includes] unchanged. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
The cache key in [Compilation_context.Filtered_includes.Key] had a local [lib_mode_tag : Lib_mode.t -> int] used by [equal] and [hash]. It duplicated facilities [Lib_mode] either already provides ([equal]) or could provide with a small addition ([hash]). Add [hash] in [Lib_mode] (a tiny int returned per constructor; the type only inhabits three values, so any combinator a caller pairs it with does the actual mixing). Rewrite [Key.equal] and [Key.hash] in terms of the [Lib_mode] API. The cache key no longer enumerates [Lib_mode]'s shape locally and a future constructor addition is caught at compile time in one place rather than two. The hash *shape* ([Poly.hash] over a tuple containing [List.map ~f:Lib.hash kept_libs]) is preserved here; the allocation it implies is its own concern, addressed separately. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
Previously [Compilation_context.filtered_include_flags] took
[kept_libs : Lib.t list], paying two avoidable costs on every
call:
1. The cache-key construction sorted the list with
[List.sort kept_libs ~compare:Lib.compare] — O(n log n) plus
a fresh list allocation, on every call including cache hits.
2. On cache misses the builder ran [Lib.Set.of_list kept_libs]
to filter [direct_requires] / [hidden_requires] by membership
— paying again for a set the caller already had structural
access to.
The semantic role of [kept_libs] is a set: callers ask "is this
library kept?", never "what is the kth kept library?". Make that
explicit. The producer in [Module_compilation.lib_deps_for_module]
now accumulates [kept_libs] as a [Lib.Set.t] inside the fold.
[filtered_include_flags] takes the set directly, derives the
sorted list for the cache key in O(n) via [Lib.Set.to_list], and
uses the set itself as the membership filter. Cache key shape on
the inside is unchanged ([Lib.t list], sorted), so the fix to the
cache-key surface is independent of this change.
Producer cost: the fold now does up to n [Lib.Set.add] ops
(O(log n) each) instead of cons cells. For typical n in the tens,
trivially below the per-call savings on the cache-hit fast path.
Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
force-pushed
the
robinbb-issue-4572-filtered-includes
branch
from
f28a9ca to
33208f1
Compare
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
May 10, 2026
Replace the cctx-wide library closure with a per-module dep set computed from ocamldep output. A consumer module's compile rule sees only the artifacts of libraries it actually references, so unrelated sibling modules no longer recompile when an unreferenced dependency library's cmi changes. User-facing detail is in the shipped changelog entry (doc/changes/added/14491.md). Components (each was a now-closed predecessor PR): - Per-module library dep filter via ocamldep BFS (was ocaml#14472). - Per-module -I/-H include flags matching the filter (was ocaml#14473). - Raw_refs Action_builder cache (was ocaml#14474) to deduplicate ocamldep results across cctxs sharing the same module sources. - Filtered_includes Action_builder cache to share rule deps between modules with identical kept-libs sets. Fixes ocaml#4572. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
robinbb
added a commit
to robinbb/dune
that referenced
this pull request
May 11, 2026
Replace the cctx-wide library closure with a per-module dep set computed from ocamldep output. A consumer module's compile rule sees only the artifacts of libraries it actually references, so unrelated sibling modules no longer recompile when an unreferenced dependency library's cmi changes. User-facing detail is in the shipped changelog entry (doc/changes/added/14491.md). Components (each was a now-closed predecessor PR): - Per-module library dep filter via ocamldep BFS (was ocaml#14472). - Per-module -I/-H include flags matching the filter (was ocaml#14473). - Raw_refs Action_builder cache (was ocaml#14474) to deduplicate ocamldep results across cctxs sharing the same module sources. - Filtered_includes Action_builder cache to share rule deps between modules with identical kept-libs sets. Fixes ocaml#4572. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
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.
Follow-up to #14472. Narrows the
-I/-Hcompiler flags per module to match the per-module dependency-filter result. Previously #14472 narrowed only the rule'sHidden_deps; the compiler invocation still used the cctx-widedirect_requires/hidden_requirespaths.Why
Two consequences of leaving
-I/-Hcctx-wide:Soundness gap: a missing entry in the per-module dep set is silently masked because
ocamlccan still resolve the symbol via a too-wide-Ipath. With per-module narrowing, a wrong filter produces a compile error rather than a silent success.Cache-key bloat: adding a library to a stanza changed every consumer's compile-rule cache key (the
-Ilist grew), invalidating modules that don't actually reference the new lib.How
The existing per-module reference set computed in
lib_deps_for_module(module_compilation.ml) already partitions each cctx library into one of three buckets:tight_modules(specific module references), tight-eligible-unreached (drop), andglob_libs(fall back to glob). The kept libs — those with tight entries or inglob_libs— are then filtered against the cctx'srequires_compileandrequires_hiddento preserve the direct/hidden classification per lib, andLib_flags.L.include_flagsproduces the per-moduleIncludes.tspliced into the compile command viaArgs.Dynin place of the previous cctx-wide injection.Fallbacks
When the dep filter falls back, so do the include flags:
can_filterfalse (Melange, dummy dep_graph, non-filterable module kind, virtual-impl consumer),has_virtual_impltrue on deps, orskip_lib_deps(Alias-not-stdlib, Wrapped_compat) — all return the cctx-wideIncludes.tfor the cm_kind.One additional fallback applies to preprocessed dep modules. The cross-lib walker (
cross_lib_tight_set) cannot run ocamldep on a foreign lib's pre-pp source, so it stops at any dep whose entry module is preprocessed. The walker reports those stopped libs and the caller takes theirLib.closure, which splits two ways:.cmideps suffice — no broadening of the rule's file-dep set for them..cmi) goes throughmust_glob_set, since the walker cannot observe types those entries re-export.Both groups stay in
kept_libsso their-I/-Hsurvive the per-module include filter. Test:per-module-lib-deps/cross-lib-walk-pre-pp-implicit-transitive.t.Tests
The commit promotes two observational baselines that already lived on
mainto reflect the new behavior:per-module-lib-deps/per-module-include-flags.t:consumer_module's compile-rule-Iflags now list only.dep_lib.objs/byte(was also listing.unrelated_lib.objs/byte).per-module-lib-deps/add-unreferenced-sibling-lib.t: rebuild lists forconsumes_depandunrelated_moduleare now[](was one rule each), recording that adding an unreferenced library to a stanza's(libraries ...)no longer rebuilds modules that don't reference it.See: #14116 (comment)