Restore transitive imports for cached modules#2677
Merged
Conversation
A module's direct imports are discovered in Compile from source or .ty headers and used to build import dependencies for scheduling. That is not the same thing as the full import closure needed by kinds and type checking. The actual import closure is reconstructed later in Env.mkEnv/doImp from the imported module's .ty files as we materialize the active environment for the module currently being compiled. That distinction broke down once a direct import was already present in the shared modules env. doImp treated that as a full cache hit and returned immediately, which meant we never revisited the imported module's recorded imports. In practice the scheduler could have a cached interface for b while the active env still lacked a transitive import such as logging that b's public interface referred to. Reusing b.ty then failed later during type checking of a dependent module with "Module logging does not exist". This change keeps the fast path for cached direct modules but still reads their .ty header imports and restores the recorded transitive import closure before returning. A visited set avoids looping while chasing the same module graph through cached hits and cold loads. A regression test was added to incremental compiler tests for a small project where b imports logging and main imports only b while using a type from b that mentions logging. The second build now succeeds when b is reused from cached .ty state. A new developer-guide note documents the split between scheduler discovery and environment materialization, the different env layers, and where .ty headers versus full .ty files are used. Nearby compiler pages now link to that note.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9447edda3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Thread the updated seen set through subImpSeen so overlapping import subgraphs are only walked once per reconstruction pass.
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
1 participant
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.
A module's direct imports are discovered in Compile from source or .ty headers and used to build import dependencies for scheduling. That is not the same thing as the full import closure needed by kinds and type checking. The actual import closure is reconstructed later in Env.mkEnv/doImp from the imported module's .ty files as we materialize the active environment for the module currently being compiled.
That distinction broke down once a direct import was already present in the shared modules env. doImp treated that as a full cache hit and returned immediately, which meant we never revisited the imported module's recorded imports. In practice the scheduler could have a cached interface for b while the active env still lacked a transitive import such as logging that b's public interface referred to. Reusing b.ty then failed later during type checking of a dependent module with "Module logging does not exist".
This change keeps the fast path for cached direct modules but still reads their .ty header imports and restores the recorded transitive import closure before returning. A visited set avoids looping while chasing the same module graph through cached hits and cold loads.
A regression test was added to incremental compiler tests for a small project where b imports logging and main imports only b while using a type from b that mentions logging. The second build now succeeds when b is reused from cached .ty state.
A new developer-guide note documents the split between scheduler discovery and environment materialization, the different env layers, and where .ty headers versus full .ty files are used. Nearby compiler pages now link to that note.