fix: soundness recovery for wrapped libs, ppx-runtime, virtual-impl deps#14517
fix: soundness recovery for wrapped libs, ppx-runtime, virtual-impl deps#14517robinbb wants to merge 2 commits into
Conversation
da056f0 to
a0893d7
Compare
c82233e to
0373a72
Compare
4e63363 to
4c95b95
Compare
0373a72 to
a94a5c6
Compare
|
While going through Three directories, all
The
#14517 fails identically to #14516, so the recovery layer does not close this Mechanism, from
The source-level reason the BFS cannot see it: One rebuttal worth pre-empting: it is not a missing user declaration. With On scope, to keep this honestly bounded:
One structural note that might be useful for the fixture set rather than the I have a discriminating cram fixture in the |
Closes a soundness gap in the per-module narrowing pipeline missed by the wrapped / ppx-runtime / virtual-impl recoveries: when a dependency library's stanza injects [-open M] via [(flags (...))], its source can reference [M]'s identifiers without naming [M], and ocamldep emits no token to drive the cross-library walker. Reported by RyanJamesStewart on #14517 with this fixture: an unwrapped [middle] depending on unwrapped [prelude] with [(flags (:standard -open Prelude))], exposing [val pick : unit -> color] (where [color] resolves through the open to [Prelude.color]). The consumer pattern-matches the result against bare constructors. The compile genuinely needs [prelude.cmi] to resolve the constructors; the BFS over [ocamldep -modules] cannot reach [prelude] (no syntactic [Prelude] token on either side); the three existing recoveries do not catch it ([prelude] is not wrapped, not a ppx-runtime lib, not a virtual-impl). [Module_compilation.cross_lib_tight_set]: - Add [~mode] (the consumer's compile mode) so we can expand a dep lib's stanza flags via [Ocaml_flags.get]. - Extend [read_entry_deps] to compute [read_stanza_opens] for the visited lib and union the result into the BFS frontier. Localised in the BFS rather than the initial-frontier computation so the reachability rule reads as: a module is reachable iff the consumer references it, or some reached module's ocamldep names it, or some reached module's owning lib stanza-opens it. Returns empty for [Spec.standard] (short-circuits external libs). [Lib_info]: - Add [stanza_flags : Dune_lang.Ocaml_flags.Spec.t] field plus accessor. Local libs carry their stanza's [conf.buildable.flags]; external libs ([findlib], [dune_package]) carry [Spec.standard]. Regression test: [cross-lib-open-flag-barrier.t]. Fails on L4 and L5 head before this patch (Unbound constructor Green under [--sandbox=copy]); passes after.
|
Thanks for the careful reproduction — the gap is real and now fixed. The diagnosis matches what I see: ocamldep on The fix sits inside the BFS rather than alongside the existing Regression test landed at Commit: |
|
@RyanJamesStewart Please verify whether the cram test that you volunteered to attach is covered by the newly added test cases. Are you an LLM that is running wild, fixing "highly active" PRs? What criteria do you use to find PRs to which to contribute? Does @stuboo monitor your work? |
Closes a soundness gap in the per-module narrowing pipeline missed by the wrapped / ppx-runtime / virtual-impl recoveries: when a dependency library's stanza injects [-open M] via [(flags (...))], its source can reference [M]'s identifiers without naming [M], and ocamldep emits no token to drive the cross-library walker. Reported by RyanJamesStewart on #14517 with this fixture: an unwrapped [middle] depending on unwrapped [prelude] with [(flags (:standard -open Prelude))], exposing [val pick : unit -> color] (where [color] resolves through the open to [Prelude.color]). The consumer pattern-matches the result against bare constructors. The compile genuinely needs [prelude.cmi] to resolve the constructors; the BFS over [ocamldep -modules] cannot reach [prelude] (no syntactic [Prelude] token on either side); the three existing recoveries do not catch it ([prelude] is not wrapped, not a ppx-runtime lib, not a virtual-impl). [Module_compilation.cross_lib_tight_set]: - Add [~mode] (the consumer's compile mode) so we can expand a dep lib's stanza flags via [Ocaml_flags.get]. - Extend [read_entry_deps] to compute [read_stanza_opens] for the visited lib and union the result into the BFS frontier. Localised in the BFS rather than the initial-frontier computation so the reachability rule reads as: a module is reachable iff the consumer references it, or some reached module's ocamldep names it, or some reached module's owning lib stanza-opens it. Returns empty for [Spec.standard] (short-circuits external libs). [Lib_info]: - Add [stanza_flags : Dune_lang.Ocaml_flags.Spec.t] field plus accessor. Local libs carry their stanza's [conf.buildable.flags]; external libs ([findlib], [dune_package]) carry [Spec.standard]. Regression test: [cross-lib-open-flag-barrier.t]. Fails on L4 and L5 head before this patch (Unbound constructor Green under [--sandbox=copy]); passes after.
a94a5c6 to
34a1914
Compare
Restores correctness for three cases the bare BFS filter mishandles: - Deps that implement a virtual library: dep-graph through them is computed elsewhere ([Dep_rules.imported_vlib_deps]); the per-module filter can miss cmi changes. Gate: fall through to glob whenever the cctx has [has_virtual_impl]. - Wrapped local libs the consumer references through the wrapper name: the ocamldep walk can't see the alias chain into the lib's [wrapped_compat] / inner modules. Reach: glob the wrapped lib's [Lib.closure]. - [ppx_runtime_libraries] introduced by [pps] in the consumer's preprocessor: their modules appear in the post-pp source which ocamldep can't see. Reach: glob their [Lib.closure]. [Module_compilation.lib_deps_for_module]: - After [can_filter], read [Compilation_context.has_virtual_impl]; if true, fall back to glob. - Read [Compilation_context.pps_runtime_libs] and include them in [direct_libs] so [Lib.closure] sees them. - Compute [wrapped_libs_referenced] from the consumer's [referenced_modules] (BFS-initial frontier — pre-cross-lib-walk). Take the [Lib.closure] of that set union [pps_runtime_libs] to get [must_glob_libs]; the classification fold sends every member to the glob path. [Modules]: - [Wrapped.entry_modules]: new function. Returns the wrapper ([lib_interface]) plus every [wrapped_compat] shim. Mirrors what [(wrapped (transition ...))] libraries expose to consumers. - [entry_modules]'s wrapped case switches to use it. Net effect: in transition wrapped libs, consumers can resolve any of the bare module names the lib exposes; this lifts a false-negative in the index that previously hid the consumer's reference to a [wrapped_compat] shim from the per-module filter. Tests (cherry-picked from #14492): - New soundness fixtures land here: [cross-lib-instrumentation-barrier.t], [cross-lib-preprocess-barrier.t], [cross-lib-pps-runtime-no-ocamldep-barrier.t], [wrapped-from-vlib-soundness.t], [wrapped-transition-soundness.t], [mixed-per-module-preprocess.t], [mixed-per-module-preprocess-precision.t], [cmx-native-tight-deps.t]. - The five pre-existing tests broken by L4 ([auto-wrapped-child-reexport.t], [ppx-runtime-libraries.t], [virtual-library.t], [wrapped-closure-precision.t], [wrapped-reexport-via-open-flag.t]) pass again — soundness recovery restores their original behavior; no test file change in #14492's diff for them. Changelog: [doc/changes/added/14492.md] lands now. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
Closes a soundness gap in the per-module narrowing pipeline missed by the wrapped / ppx-runtime / virtual-impl recoveries: when a dependency library's stanza injects [-open M] via [(flags (...))], its source can reference [M]'s identifiers without naming [M], and ocamldep emits no token to drive the cross-library walker. Reported by RyanJamesStewart on #14517 with this fixture: an unwrapped [middle] depending on unwrapped [prelude] with [(flags (:standard -open Prelude))], exposing [val pick : unit -> color] (where [color] resolves through the open to [Prelude.color]). The consumer pattern-matches the result against bare constructors. The compile genuinely needs [prelude.cmi] to resolve the constructors; the BFS over [ocamldep -modules] cannot reach [prelude] (no syntactic [Prelude] token on either side); the three existing recoveries do not catch it ([prelude] is not wrapped, not a ppx-runtime lib, not a virtual-impl). [Module_compilation.cross_lib_tight_set]: - Add [~mode] (the consumer's compile mode) so we can expand a dep lib's stanza flags via [Ocaml_flags.get]. - Extend [read_entry_deps] to compute [read_stanza_opens] for the visited lib and union the result into the BFS frontier. Localised in the BFS rather than the initial-frontier computation so the reachability rule reads as: a module is reachable iff the consumer references it, or some reached module's ocamldep names it, or some reached module's owning lib stanza-opens it. Returns empty for [Spec.standard] (short-circuits external libs). [Lib_info]: - Add [stanza_flags : Dune_lang.Ocaml_flags.Spec.t] field plus accessor. Local libs carry their stanza's [conf.buildable.flags]; external libs ([findlib], [dune_package]) carry [Spec.standard]. Regression test: [cross-lib-open-flag-barrier.t]. Fails on L4 and L5 head before this patch (Unbound constructor Green under [--sandbox=copy]); passes after.
34a1914 to
a75f81a
Compare
Closes a soundness gap in the per-module narrowing pipeline missed by the wrapped / ppx-runtime / virtual-impl recoveries: when a dependency library's stanza injects [-open M] via [(flags (...))], its source can reference [M]'s identifiers without naming [M], and ocamldep emits no token to drive the cross-library walker. Reported by RyanJamesStewart on #14517 with this fixture: an unwrapped [middle] depending on unwrapped [prelude] with [(flags (:standard -open Prelude))], exposing [val pick : unit -> color] (where [color] resolves through the open to [Prelude.color]). The consumer pattern-matches the result against bare constructors. The compile genuinely needs [prelude.cmi] to resolve the constructors; the BFS over [ocamldep -modules] cannot reach [prelude] (no syntactic [Prelude] token on either side); the three existing recoveries do not catch it ([prelude] is not wrapped, not a ppx-runtime lib, not a virtual-impl). [Module_compilation.cross_lib_tight_set]: - Add [~mode] (the consumer's compile mode) so we can expand a dep lib's stanza flags via [Ocaml_flags.get]. - Extend [read_entry_deps] to compute [read_stanza_opens] for the visited lib and union the result into the BFS frontier. Localised in the BFS rather than the initial-frontier computation so the reachability rule reads as: a module is reachable iff the consumer references it, or some reached module's ocamldep names it, or some reached module's owning lib stanza-opens it. Returns empty for [Spec.standard] (short-circuits external libs). [Lib_info]: - Add [stanza_flags : Dune_lang.Ocaml_flags.Spec.t] field plus accessor. Local libs carry their stanza's [conf.buildable.flags]; external libs ([findlib], [dune_package]) carry [Spec.standard]. Regression test: [cross-lib-open-flag-barrier.t]. Fails on L4 and L5 head before this patch (Unbound constructor Green under [--sandbox=copy]); passes after. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
a75f81a to
8655699
Compare
|
On the test: the fixture I offered is the one you have added as On the rest: I am not an LLM running wild. I post under my full name and stand behind what I post under my own judgment. My criteria for participating in a PR are deliberately narrow: I sweep open issues and recently merged PRs, look for a load-bearing case the existing patches do not cover, reproduce it myself under the same oracle the in-tree tests use, and only post when the finding is clearly outside what is already addressed. I set aside far more than I send. I have no idea who stuboo is. This is my first contribution here; it will not be the last. |
|
I have no connection to this account or this repository. |
That is a lot of work, @RyanJamesStewart! What inspires you to go to such lengths? (In any case, I appreciate your having found a weakness in this PR.) |
@stuboo Is the user who committed the following to your (@RyanJamesStewart's) time tracker repo: RyanJamesStewart/time-tracker@77cfb81 |
@stuboo Perhaps a bot has falsely claimed to have made this commit by you: RyanJamesStewart/time-tracker@77cfb81 |
Layer 5 of 9 of #14492.
Restores correctness for three cases the bare BFS filter mishandles: virtual-impl deps (fall back to glob via
has_virtual_impl), wrapped local libs reached through the wrapper name (glob the wrapped lib'sLib.closure), andppx_runtime_librariesintroduced bypps(glob theirLib.closure).Module_compilation.lib_deps_for_module: readshas_virtual_impland falls back to glob if true; readspps_runtime_libsand folds them intodirect_libs; computeswrapped_libs_referencedand unions withpps_runtime_libsunderLib.closureto producemust_glob_libs; the classification fold sends everymust_glob_setmember to the glob path.Modules.Wrapped.entry_modulesreturns the wrapper plus everywrapped_compatshim.Modules.entry_modules's wrapped case switches to use it; in transition wrapped libs the index now sees the bare module names consumers can resolve.Adds 8 new soundness fixtures (
cross-lib-instrumentation-barrier.t,cross-lib-preprocess-barrier.t,cross-lib-pps-runtime-no-ocamldep-barrier.t,wrapped-from-vlib-soundness.t,wrapped-transition-soundness.t,mixed-per-module-preprocess.t,mixed-per-module-preprocess-precision.t,cmx-native-tight-deps.t). Lands the changelog (doc/changes/added/14492.md).The five cram tests broken on layer 4 pass again here.
Stack: rebases on #14516. Next: #14518.
Part of #14492. Related to #4572.