transpile: defer+cache imports alongside zero initializers #1455
Conversation
|
This is atop the "enable" branch, despite being a prerequisite fix, so that it gets test coverage in CI. |
|
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. |
fw-immunant
force-pushed
the
fw/defer-imports-v2
branch
3 times, most recently
from
f5d04aa to
8dae9ac
Compare
|
Ready for review now. |
fw-immunant
force-pushed
the
fw/defer-imports-v2
branch
from
8dae9ac to
58286f4
Compare
kkysen
left a comment
kkysen
left a comment
There was a problem hiding this comment.
Forgot to submit this last night. Still reviewing the rest.
fw-immunant
force-pushed
the
fw/defer-imports-v2
branch
from
58286f4 to
9fc0fff
Compare
kkysen
left a comment
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.
kkysen
force-pushed
the
kkysen/reorganize-definitions-enable
branch
from
b1993e9 to
c191695
Compare
kkysen
force-pushed
the
fw/defer-imports-v2
branch
from
9fc0fff to
ecdc322
Compare
|
Also, I force pushed to rebase since there was some issue with outdated CI that was changed on |
fw-immunant
force-pushed
the
fw/defer-imports-v2
branch
from
ecdc322 to
be7c164
Compare
kkysen
left a comment
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
kkysen
force-pushed
the
fw/defer-imports-v2
branch
from
be7c164 to
5f61068
Compare
kkysen
changed the title
kkysen
changed the base branch from
kkysen/reorganize-definitions-enable
to
master
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.