transpile: defer+cache imports alongside zero initializers#1455
Merged
Conversation
Contributor
Author
|
This is atop the "enable" branch, despite being a prerequisite fix, so that it gets test coverage in CI. |
Contributor
Author
|
The testsuite is failing in a way that indicates this isn't sufficient to capture all required imports. We might need to handle import deferral nestably. |
f5d04aa to
8dae9ac
Compare
Contributor
Author
|
Ready for review now. |
8dae9ac to
58286f4
Compare
kkysen
reviewed
Nov 18, 2025
Contributor
kkysen
left a comment
There was a problem hiding this comment.
Forgot to submit this last night. Still reviewing the rest.
58286f4 to
9fc0fff
Compare
kkysen
reviewed
Nov 20, 2025
Contributor
kkysen
left a comment
There was a problem hiding this comment.
The overall logic makes sense to me. I had a question about the fn import_for_type implementation, though, and then a few nitpicks of other things.
b1993e9 to
c191695
Compare
9fc0fff to
ecdc322
Compare
Contributor
|
Also, I force pushed to rebase since there was some issue with outdated CI that was changed on |
ecdc322 to
be7c164
Compare
kkysen
approved these changes
Nov 21, 2025
Contributor
kkysen
left a comment
There was a problem hiding this comment.
LGTM now! Don't merge it yet, though, as I want to remove the PR in front of this since we don't want to enable it default.
computing these is impure because it can call `import_type`. ideally we would not disable this caching but instead cache both the initializer and its set of required imports.
this lets us cache required imports alongside the zero-initializer expressions themselves we remove the `decl_file_id` passed in many places because we want to track the real dependencies of decls, without yet taking into account which file we are in; when we later turn this information into `use` items, we skip imports that are not necessary from a given importing location
be7c164 to
5f61068
Compare
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
2 participants
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.
I think this is the right way to fix this. I tried adding a
WithStmtsAndImportstype instead, to avoid larger shared state, but it ended up infecting almost the entire transpiler.