-
Notifications
You must be signed in to change notification settings - Fork 169
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Assembler
single-use
#1409
Conversation
/// | ||
/// # Errors | ||
/// Returns an error if compiling kernel source results in an error. | ||
pub fn with_kernel_from_module(module: impl Compile) -> Result<Self, Report> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we should remove with_kernel()
in this PR as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a way to construct the Assembler
with only a single kernel library, so we probably still need it. If we have metadata associated with a Library
that tells us whether it is a kernel library or not, then we can probably make it less explicit and check that we've only added a single library of kernel type in add_library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left just one small comment inline.
I pushed a few small commits. The biggest change was removing PhantomCall
struct (doesn't seem like it was used in any meaningful way).
Also, I think there is a pretty good way to combine ProcedureCache
and MastForestBuilder
- should make the structure quite a bit cleaner IMO - but we can do this in a different PR.
Lastly, I think there may be a relatively simple way to avoid introducing wrapper modules for the ModuleGraph
. Here is how it could work:
We have resolve_target() method in the assembler. This methods seems to be responsible for mapping procedure invocations to their corresponding MAST roots. My understanding is that if it can't resolve a procedure reference, it returns an error.
If the above is correct, we could do the following:
First, update the ModuleGraph::resolve_target()
to return something like ResolvedTarget::Unresolved
variant if procedure resolution fails. Then, in Assembler::resolve_target()
we could do something like this:
...
let resolved = self.module_graph.resolve_target(&caller, target)?;
match resolved {
ResolvedTarget::Phantom(digest) | ResolvedTarget::Cached { digest, .. } => Ok(digest),
ResolvedTarget::Exact { gid } | ResolvedTarget::Resolved { gid, .. } => Ok(self
.procedure_cache
.get(gid)
.map(|p| p.mast_root(mast_forest))
.expect("expected callee to have been compiled already")),
ResolvedTarget::Unresolved(proc_path, error) => {
// check if the procedure exists in the store; if not return the error
match self.store.get_procedure_hash(proc_path) {
Some(digest) => Ok(digest),
None => Err(error),
}
}
}
If the above works, I think the changes to the current ModuleGraph
should be minimal. But it's also possible that I'm missing something. cc @bitwalker.
@@ -81,7 +81,7 @@ impl Assembler { | |||
callee: mast_root, | |||
}); | |||
} | |||
None => context.register_phantom_call(Span::new(span, mast_root))?, | |||
None => (), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a comment to explain why we don't need to do anything here.
@@ -540,65 +453,6 @@ impl ModuleGraph { | |||
Ok(()) | |||
} | |||
|
|||
/// Resolves a [FullyQualifiedProcedureName] to its defining [Procedure]. | |||
pub fn find( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this was unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - just one question I left as a comment
With this PR it is no longer possible to use a single instance of
Assembler
to assemble multiple programs.Simplifies #1401.