Repository navigation
fix(resolver): seed reachability from unconditionally-kept modules too - #117
Merged
Merged
Conversation
`forge test` compiles all test files together but picks only one as the compile entry (alphabetically first). Reachability pruning (#106) keeps a non-entry sibling test module's own decls unconditionally (DDescribe/DTest count as global-effect), but never fed that sibling's OWN textual references into the reachability BFS — only the entry file's source and explicit-import sources were seeded. So a plain library module referenced ONLY by a non-entry sibling test file (never by the entry file itself) was silently pruned from the merged program, surfacing as a misleading "Unknown module `Foo`. Did you mean `Bar`?" at typecheck time instead of a pruning diagnostic. This isn't an edge case: any multi-file test suite where the alphabetically-first test file doesn't happen to mention every lib module a sibling test file needs hits it. Fix: seed the reachability queue with the source text of every discovered module kept via `has_global_effect_decl`, not just entry_src + loaded_srcs. Regression test: test_lib_module_used_only_by_sibling_test_is_not_pruned, verified to fail with only the fix reverted (Expected true, Received false). run_compiler: 615/615, run_eval: 256/256.
Ch4s3
force-pushed
the
fix/sibling-test-lib-reachability-pruning
branch
from
July 29, 2026 17:41
dcd691c to
963b83e
Compare
4 tasks done
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
forge testcompiles all discovered test files together but picks only ONE as the compile entry (alphabetically first —forge/lib/cmd_test.ml'srun_files).DDescribe/DTestcount as "global effect". But it never fed that sibling's own textual references back into the reachability BFS — the seed set was only the entry file's source plus explicit-import sources.Unknown moduleFoo. Did you meanBar?at typecheck time — not a pruning diagnostic — for a module that plainly exists and is plainly used.main:test/markdown_test.marchsorts beforetest/sigil_test.marchand never mentionsPage/Build, whichsigil_test.marchuses extensively — so those two lib modules vanished from the compiled test binary.Fix
Seed the reachability queue with the source text of every discovered module that's kept unconditionally via
has_global_effect_decl, in addition to the entry file and explicit-import sources.Verification
test_lib_module_used_only_by_sibling_test_is_not_pruned, confirmed to fail with only the fix reverted (Expected true, Received false) and pass with it.run_compiler: 615/615run_eval: 256/256markdown_test.march,properties_test.march,sigil_test.march, ~1800 combined lines): theUnknown module Page/Builderrors are gone after this fix.main(past fix(join-points): never float a let that reads the case scrutinee #114 and fix(refinecheck): a stdlib import must not withdraw the List.length alias #115) — theList.lengthalias bug I'd originally bundled here landed independently as fix(refinecheck): a stdlib import must not withdraw the List.length alias #115, so this PR is back to just the resolver fix.Test plan
run_compilerfull suite greenrun_evalfull suite green