Skip to content

fix(resolver): resolve modules by declared name; stop silently dropping test modules - #106

Merged
Ch4s3 merged 2 commits into
mainfrom
fix/dotted-import-stdlib-prefix
Jul 29, 2026
Merged

Ch4s3 merged 2 commits into
mainfrom
fix/dotted-import-stdlib-prefix

Conversation

@Ch4s3

@Ch4s3 Ch4s3 commented Jul 29, 2026 •

Copy link
Copy Markdown
Contributor

Three resolver bugs found while moving forgepm onto current main. Each is independently verified and has a regression test that fails with only its own fix reverted.

1. A module was importable only at its conventional path

import Depot.Query failed with Module `Depot.Query` not found (looked for `depot/query.march`) for depot's own modules, which live at lib/api/depot_query.march. forge check passed and forge build did not, so the two disagreed about what resolves.

Nine Depot* names sat in stdlib_module_names, whose documented job is suppressing "not found" for modules that ship in stdlib/. None of them do — depot is a third-party package. While import_refs yielded only a path's first segment that bogus entry was accidentally load-bearing: use Depot.Query was skipped as stdlib and auto-discovery supplied the module. Once 7849f0d made candidates dotted, the stdlib filter dropped the bare Depot and the surviving Depot.Query became a hard error. (URI was listed too, but stdlib/uri.march declares mod Uri, so the casing mismatch meant a plain import Uri reported "not found".)

Deleting those entries alone leaves the real gap: find_file resolved only by the conventional dotted→path mapping, while auto-discovery indexes by the mod declaration — so a module whose file doesn't follow the convention was reachable by reference but not by explicit import. find_file now falls back to scanning for the file that declares the name, only when the conventional path misses, so a genuinely-absent module keeps its precise looked for <path> diagnostic.

This is not Depot-specific: mod Config.Test hit the identical failure, and Config genuinely does ship in stdlib.

2. forge test silently ran a fraction of the suite

forge test compiles ONE test file as the entry and relies on auto-discovery for the siblings. Reachability pruning (48cf926) keeps a discovered module only when the entry references it or it carries a global-effect decl — and nothing references a test module by name, which is what a test module is. Every sibling was dropped, collect_tests never saw its bodies, and the run reported a PASS over whatever survived.

forgepm went from 544 tests to 21, exit 0. Silent test loss with a green exit is the worst available failure mode.

DDescribe/DTest/DSetup/DSetupAll now count as global-effect. DDescribe is the one that matters — a test file's top-level decls are describe blocks, and adding only DTest changes nothing.

3. Checking order discarded the search path

Fixing (2) then produced 552 expected () but got Unit errors. Phase 2 sorted discovered modules by NAME (dot-count, then alphabetical), discarding the order the search path was given in. forge deliberately puts lib/ before test/ on MARCH_LIB_PATH, and its own comment explains why: a lib function still bound to its pass-1 Mono placeholder gets pinned by whichever call site is checked first. Forgepm.Test.* and Forgepm.Web.* have equal dot-counts, so Test sorted ahead of Web and pinned every shared helper from a test call site. Search-path index is now the primary sort key; the dot-count rule still orders within a directory.

Verification

  • run_compiler 607/607, run_eval 256/256
  • Both new tests verified to fail with only their own fix reverted. The pruning test writes a real entry file — resolve_imports disables pruning outright when it cannot read source_file, so a synthetic path makes such a test vacuous (my first version passed either way)
  • End to end: depot, bastion and forgepm all build against this. forgepm goes from 21 tests, 0 failures (unfixed main) to 544 tests, 0 failures, matching the pre-regression count on the old pin

Related

Needs march-language/depot#1 and march-language/bastion#5 to build those two against current main; neither is required for this PR.

Ch4s3 added 2 commits July 28, 2026 18:05
`import Depot.Query` failed with "Module `Depot.Query` not found (looked
for `depot/query.march`)" for depot's own modules, which live at
lib/api/depot_query.march. `forge check` passed and `forge build` did
not, so the two disagreed about what resolves.

Two independent causes:

1. Nine Depot* names (Depot, Depot.Gate, DepotForm, DepotGate,
   DepotSchema, DepotRepo, DepotQuery, DepotMigration, DepotTest) sat in
   [stdlib_module_names], whose documented job is suppressing "not found"
   for modules that ship in stdlib/. None of them do — depot is a
   third-party package. While [import_refs] yielded only a path's first
   segment, that bogus entry accidentally load-bearing: `use Depot.Query`
   was skipped as stdlib and auto-discovery supplied the module. Once
   7849f0d made candidates dotted, the stdlib filter dropped the bare
   "Depot" and the surviving "Depot.Query" became a hard error.

   "URI" was listed too, but stdlib/uri.march declares `mod Uri`, so the
   casing mismatch meant a plain `import Uri` reported "not found".
   Both corrected.

2. Deleting those entries alone leaves the real gap: [find_file] resolved
   ONLY by the conventional dotted->path mapping, while step 2's
   auto-discovery indexes by the `mod` declaration. A module whose file
   does not follow the convention was therefore reachable by reference
   but not by explicit `import`. [find_file] now falls back to scanning
   the search path for the file that declares the name. The scan runs
   only when the conventional path misses, so the common case stays one
   Sys.file_exists and a genuinely-absent module keeps its precise
   "looked for <path>" diagnostic.

This also covers any user module under a REAL stdlib prefix — `mod
Config.Test` (forgepm has exactly that) hit the identical failure, since
Config does ship in stdlib.

run_compiler 606/606 with a new regression test that fails on main.
Verified end to end: depot, bastion and forgepm all build against this.
…ecking

`forge test` compiles ONE test file as the entry and relies on
auto-discovery for the siblings. Two bugs made that silently wrong.

1. Reachability pruning (48cf926) keeps a discovered module only when the
   entry references it or it carries a global-effect decl. Nothing
   references a test module by name — that is what a test module IS — so
   every sibling was dropped, collect_tests never saw its bodies, and the
   run reported a PASS over the few that survived. forgepm went from 544
   tests to 21, exit 0. Silent test loss with a green exit is the worst
   available failure mode. DDescribe/DTest/DSetup/DSetupAll now count as
   global-effect; DDescribe is the one that matters, since a test file's
   top-level decls are `describe` blocks.

2. Keeping them then produced 552 `expected () but got Unit` errors,
   because phase 2 sorted discovered modules by NAME (dot-count, then
   alphabetical), discarding the order the search path was given in.
   forge deliberately puts lib/ before test/ on MARCH_LIB_PATH — test
   modules call into lib modules, never the reverse, and a lib function
   still bound to its pass-1 Mono placeholder gets pinned by whichever
   call site is checked first. `Forgepm.Test.*` and `Forgepm.Web.*` have
   equal dot-counts, so "Test" sorted ahead of "Web" and pinned every
   shared helper from a test call site. Search-path index is now the
   primary sort key; the dot-count rule still orders within a directory.

Two regression tests, each verified to fail with only its own fix
reverted. The pruning one writes a REAL entry file: resolve_imports
disables pruning outright when it cannot read source_file, so a synthetic
path makes such a test vacuous.

run_compiler 607/607, run_eval 256/256. forgepm: 544 tests, 0 failures
(was 544/10 on the old pin, 21/0 on unfixed main).
@Ch4s3
Ch4s3 merged commit 63a8e1d into main Jul 29, 2026
23 checks passed
Ch4s3 added a commit to march-language/conduit that referenced this pull request Jul 29, 2026
…ted)

forge.toml declared depot via { registry = "forge", version = "0.1.0" },
switched from a path dep in bcead90 ("prepare conduit 0.1.0 for forge
registry publish"). forge's RegistryDep resolution is an unbuilt
placeholder (forge/lib/cmd_deps.ml: "RegistryDep: placeholder (registry
not yet built)"), so that dependency silently resolved to nothing —
depot was completely absent from MARCH_LIB_PATH, and every depot-backed
module (Db, Pool, Connection, ParamText, ...) in postgres.march showed as
unknown. This has been the actual cause of CI failing since that commit;
none of it was related to the March version bump in this branch.

CI's workflow already clones depot to ../depot specifically for this,
so switching back to a path dep re-enables what CI was built for.

Verified against a from-scratch sandbox: fresh depot clone, march built
from upstream main (after merging march-language/march#106 and
march-language/depot#1, both required for this to typecheck) — forge
build and forge test --release both pass clean (167 tests, 0 failures).
Ch4s3 added a commit to march-language/conduit that referenced this pull request Jul 29, 2026
march's resolver fix (march-language/march#106, merged) makes forge test
actually typecheck every auto-discovered test file instead of silently
dropping siblings the entry doesn't reference by name. That surfaced a
real, previously-invisible gap here: the module imports Connection
(needs IO.NetConnect) and calls random_bytes transitively via the
Postgres storage backend (needs IO.Random) but declared neither.
Ch4s3 added a commit to march-language/bastion that referenced this pull request Jul 29, 2026
Until march-language/march#106, reachability pruning dropped every test
module except the entry, so `forge test` compiled one file and passed.
These five files had not been compiled in a long time.

  - test_form: `Form.Wrapper.render` call sites updated to the parameter
    order this branch introduced (they exist after all — my earlier claim
    that nothing calls it came from grepping `Form.render`, and the call
    is spelled `Form.Wrapper.render`); `import Bastion.Test.Conn` and the
    capabilities it propagates.
  - bastion_test: 25 ambiguous `Header` constructors qualified to
    Http.Header; `needs IO.NetListen`.
  - test_gate: LenMin/LenMax/NumMin/NumMax qualified to Gate.* — depot's
    Gate declares the same names.
  - test_html_css: `import Css`.
  - test_islands_dataflow: `needs IO.NetListen`.

Form's components pointed at the WRONG Gate. `Depot.Gate.error_for` does
not exist, so Form.Input/Select/Error failed to link the moment something
pulled them into an executable — invisible while the tests were pruned,
and invisible to `forge build`, which only ever builds a library. The
components' callers build with `Gate.cast` (bastion's own
lib/security/gate.march), and that module has exactly the API in use:
error_for/2 and get_field/2. Repointed all 12 references.

bastion's suite: 81 compile errors -> 0, and 6 runtime failures -> 0.
Ch4s3 added a commit that referenced this pull request Jul 29, 2026
`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 added a commit that referenced this pull request Jul 29, 2026
#117)

`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 added a commit to march-language/bastion that referenced this pull request Jul 29, 2026
…tricter default-arg checking (#5)

* fix: declare IO.NetListen; adapt to FileError and ambiguous ctors

march main makes capability propagation through imports an ERROR rather
than a warning: a module importing HttpServer (directly or via Conn) must
declare `needs IO.NetListen` itself. Adds it to the 21 modules that do.

Two further adaptations to current main:

  - File.* now returns Result(_, FileError), a variant, not Result(_, String).
    Six sites concatenated the error straight into a message; they now go
    through to_string, which renders the underlying message verbatim
    ("No such file or directory"). Forge.Routes.generate also propagated a
    FileError out of a Result(_, String), now converted at the boundary.

  - `Header` is ambiguous between Http.Header and WebSocket.Header. Both
    sites destructure headers from HttpServer.req_headers/resp_headers, so
    both qualify as Http.Header.

Takes `forge check` against main-a5c3dc70 from 44 errors to 18. The rest
are not bastion's: 17 ambiguous JoinKind/SortDir constructors (Ast vs
DataFrame) inside depot, plus Form.render, whose trailing required
`render_inner` after three defaulted params does not type-check under the
stricter checking — that signature needs a decision, not a mechanical fix.

* fix(view): move Form.render's render_inner ahead of the defaulted params

A required parameter after defaulted ones does not type-check: the
reduced-arity desugaring matches the trailing argument against the first
default's type, so `render_inner` was checked against `method : String`
and `enctype`'s default against `render_inner`. Not a regression — the
shape fails on main-8c87ccb2 too; earlier errors simply masked it.

No call sites exist in bastion or forgepm, so reordering breaks nothing,
and `Form.render(conn, action, fn -> ...)` still reads naturally.

* fix(testing): declare IO.NetListen in Bastion.Test.Auth

Missed by the earlier sweep: the testing helpers are not in bastion's own
`forge check` graph, so the requirement only surfaced when a consumer's
test build pulled Bastion.Test.Auth in.

* fix(testing): make Bastion.Test.Conn compile under current march

The testing helpers are not in bastion's own `forge check` graph, so
these only surface when a consumer's test build pulls them in — which,
before the resolver kept sibling test modules, never happened.

  - `needs IO.NetListen` + `needs IO.Clock` (both propagate in through
    HttpServer).
  - `Conn`, `Header` and `NoUpgrade` are each declared by two modules in
    scope; every use here means the HTTP one, so they qualify to
    HttpServer.Conn / Http.Header / HttpServer.NoUpgrade.

* fix(forge): uncurry the scaffold writers

`write(path)(content)` is no longer accepted — March functions are not
curried, and the parser now rejects `f(...)(...)` outright. The three
scaffolding generators build their files through such a writer; they take
(path, content) as one call now.

* fix: repair the test suite march#106 makes visible again

Until march-language/march#106, reachability pruning dropped every test
module except the entry, so `forge test` compiled one file and passed.
These five files had not been compiled in a long time.

  - test_form: `Form.Wrapper.render` call sites updated to the parameter
    order this branch introduced (they exist after all — my earlier claim
    that nothing calls it came from grepping `Form.render`, and the call
    is spelled `Form.Wrapper.render`); `import Bastion.Test.Conn` and the
    capabilities it propagates.
  - bastion_test: 25 ambiguous `Header` constructors qualified to
    Http.Header; `needs IO.NetListen`.
  - test_gate: LenMin/LenMax/NumMin/NumMax qualified to Gate.* — depot's
    Gate declares the same names.
  - test_html_css: `import Css`.
  - test_islands_dataflow: `needs IO.NetListen`.

Form's components pointed at the WRONG Gate. `Depot.Gate.error_for` does
not exist, so Form.Input/Select/Error failed to link the moment something
pulled them into an executable — invisible while the tests were pruned,
and invisible to `forge build`, which only ever builds a library. The
components' callers build with `Gate.cast` (bastion's own
lib/security/gate.march), and that module has exactly the API in use:
error_for/2 and get_field/2. Repointed all 12 references.

bastion's suite: 81 compile errors -> 0, and 6 runtime failures -> 0.

* ci: resolve dependencies before building

`forge build` does not fetch deps. On a fresh runner the CAS is empty, so
depot was never cloned and every module it provides reported as unknown —
`Unknown module \`Pool\`. Did you mean \`Plot\`?` from
depot_middleware. Pre-existing; it looked like a depot problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant