ReviewOS

home-lang/home

fix(modules): resolve relative imports from the owning module path

#920
Closed glennmichael123 opened this 8 days ago ·
8 days ago

Follow-up to #884 and 5f700aa68. The now-tested real packages/modules/src/module_system.zig loader still implements resolveImport(from_module, import_name) by discarding from_module and calling loadModule(import_name). loadModule searches the process cwd and global search paths, so two modules in different directories cannot reliably import their own ./sibling.home; a same-basename module elsewhere can resolve incorrectly. This is a source-observed gap, not a passing/failing runtime test claim.

Acceptance:

  • Resolve relative imports against the importing module's owned file path, independent of process cwd, while preserving bare-name search-path behavior.
  • Canonicalize path/cache identity so aliases and repeated imports share one owned AST, but same basenames in different directories remain distinct; deinit each module exactly once.
  • Preserve the 10 MiB source limit, explicit std.Io, public/private export semantics, parse-error rejection and error propagation from #884.
  • Add real temporary-directory tests with two independent roots, nested ./ and ../ imports, aliases, missing targets, and allocation/lifetime checks. No simulation tests or assertion weakening.
  • Qualify under pinned Zig 0.17 and Zig 0.18 in Debug and ReleaseFast within the 3,840 MB guard; separately record cross-platform runtime limits.

Parent: #66.

8 days ago

Implemented and pushed directly to main in ca881af (3 files; author/committer glennmichael123, no coauthors). The loader now resolves ./ and ../ against the loaded importer’s owned canonical file path, preserves bare-name cwd/search-path lookup, caches by canonical path, and frees each module once. Real two-root tests cover same-basename isolation, explicit .home extension, nested ../, alias identity, bare-name identity, missing targets, and the requirement that the importer be loaded. Existing tests continue to cover source/AST lifetime, public exports, the 10 MiB boundary, and parse-error rejection/retry. The new test failed before the fix and passed afterward. Verification: modules 6/6 on Zig 0.17.0-dev.2163 and Zig 0.18.0-dev.146, each in Debug and ReleaseFast; one guarded build at a time, 3,840 MB ceiling, observed peak at most 449 MB. This is macOS runtime coverage only; Windows/Linux behavior is not claimed. The README records the std.Io.Dir.realPathFile platform-support limit and reproducible test commands. Parent tracking: #66 and #884.

8 days ago

Follow-up correction pushed directly to main in 5fa14ef (exactly 3 files; glennmichael123 author/committer, no coauthors). The canonical-path refactor in ca881af dropped the previous behavior where a successfully loaded bare request remains cached after its file is removed. A real temp-file regression failed on the shipped code (ModuleNotFound), then passed with a separate non-owning request-name index; the canonical path map remains the sole owner. Alias capacity is reserved before parsing so an allocation failure cannot publish only the canonical entry and return an error. Module tests 7/7 pass on Zig 0.17/0.18 Debug and ReleaseFast, run serially under the 3,840 MB guard (highest observed peak 458 MB). The previously documented Windows/Linux runtime limit remains. #920 stays closed.

8 days ago

Follow-up shipped directly to main in f61de38. A real temporary-directory regression test first reproduced that a repeated relative request (./shared) failed with ModuleNotFound after the source file was removed, even though the module remained loaded. The loader now retains a non-owning alias keyed by the canonical importing module plus the relative request, while the canonical path map remains the sole owner. The test covers two importers using the same spelling, source removal, missing targets, and retry after a parse failure. Guarded module tests pass 8/8 with pinned Zig 0.17 and Zig 0.18, each in Debug and ReleaseFast; all four runs stayed below the 3,840 MB ceiling. Windows/Linux runtime behavior was not tested here.

Sign in to comment on this issue.