test: regression guard for include-flag soundness through pre-pp entry - #14435
Conversation
The cross-library walker stops at preprocessed entry modules because ocamldep on the pre-pp source can fail (the neighbour test [cross-lib-walk-pre-pp-source.t] guards that stopping behaviour). When the preprocessed module's [.mli] mentions a type from another library, the walker cannot observe the link. Any per-module filtering of include flags must be conservative enough to keep that other library's [-I]/[-H] on the consumer's compile rule. The test wires [other_dep] -> [pp_dep] -> [consumer]: [other_dep] is unwrapped with a record type [Other.t]; [pp_dep] is unwrapped + preprocessed and exposes [val make_thing : unit -> Other.t]; the consumer destructures [.x] without ever naming [Other]. The field access forces the type checker to load [other.cmi], so a narrower include filter that drops [other_dep] makes the build fail. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
There was a problem hiding this comment.
Pull request overview
Adds a new blackbox (cram) regression test to ensure Dune’s per-module dependency/include-flag logic remains conservative when a consumer only becomes dependent on a transitive library via a preprocessed library’s .mli—even if the consumer never syntactically mentions the transitive module.
Changes:
- Introduces a new cross-library test case covering an “implicit transitive through pre-pp entry” path not exercised by existing tests.
- Sets up three small libraries (
other_dep→pp_dep(preprocessed) →consumer) and assertsdune build @checksucceeds when the consumer forces loading the transitive.cmivia record-field access.
shonfeder
left a comment
shonfeder
left a comment
There was a problem hiding this comment.
Despite several rereads, I'm having trouble understanding what is meant to be tested or demonstrated here.
It's possible some time rewording the description would help, or just that I don't have enough prior context to follow along clearly. My suggestions are superficial, and while I can approve to merge the test (which is passing, so doesn't do any harm), you may want to get a review from someone who understands the context better, or try rewording to make it easier for someone in my position to digest!
Replaces the inline OCaml helper executable with `(action (run cat
%{input-file}))`, matching the convention established in
`test/blackbox-tests/test-cases/include-qualified/pp-github6866.t` and
`pp.t/dune`. The walker only cares that some preprocess action is
declared on the entry module; the pass-through's contents are
irrelevant.
Signed-off-by: Robin Bate Boerop <me@robinbb.com>
Rewrite the test's prose so a reader without prior context can follow what is set up and why the assertion is non-trivial. Lead with the concrete three-library scenario before explaining why dune cannot observe the dependency. Drop "tight" / "tight-eligible" terminology that has no meaning in the context of this test, drop the parenthetical "(used to compute per-module dependency sets)", and trim the per-block prose now that the header carries the load. Signed-off-by: Robin Bate Boerop <me@robinbb.com>
|
@shonfeder, thanks for the careful read; your trouble understanding is fair signal that the prose wasn't doing its job. Reworked the test on the latest commits:
If it still doesn't read clearly, please push back — happy to keep iterating. |
shonfeder
left a comment
shonfeder
left a comment
There was a problem hiding this comment.
This reads much clearer now! Everything makes sense to me, up to limit of my knowledge. Thanks :)
Summary
Adds a cross-library cram test: a consumer reaches a transitive type only through a preprocessed library's
.mli, where the type is never syntactically named in the consumer's source.Three libraries:
other_dep— unwrapped, exposestype t = { x : int; y : string }.pp_dep— unwrapped + preprocessed, depends onother_dep, exposesval make_thing : unit -> Other.t.consumer— depends onpp_dep, accesses(D.make_thing ()).x.consumer/c.mlnever mentionsOther, but the field access forces the type checker to loadother.cmi. The test assertsdune build @checksucceeds.Sibling to #14400's
cross-lib-walk-pre-pp-source.t. That test exercises behaviour at a preprocessed entry directly; this one exercises the transitive leg — where the preprocessed entry's.mlire-exports a type from a deeper library.