Minor refactoring of assembler API #1426
Conversation
bobbinth
added
the
no changelog
assembly/src/library/mod.rs
Outdated
| @@ -77,9 +80,9 @@ impl CompiledLibrary { | |||
| pub fn new( | |||
| mast_forest: MastForest, | |||
| exports: BTreeMap<QualifiedProcedureName, RpoDigest>, | |||
| ) -> Result<Self, CompiledLibraryError> { | |||
| ) -> Result<Self, LibraryError> { | |||
| if exports.is_empty() { | |||
There was a problem hiding this comment.
For some reason, I never noticed this, but this check feels like something that should be an assertion, not an error that needs to be handled - by the time we're ever attempting to construct a library, we already would've raised errors due to the lack of modules/exports. The only exception to that is during deserialization, and it would make more sense to raise a deserialization error for that, since it is the only place where we can't assume that the library is well-formed.
Only reason I bring it up, is because Library::new feels like it should be infallible - it isn't actually a problem for the library to be empty, just sort of useless; and the assembler already handles validating that there is actually something to assemble before it ever calls this. It would also make it possible to construct a Library empty, and then extend it with MAST bit by bit.
It's not hugely important, but since we're cleaning up the API a bit, figured I'd mention it.
There was a problem hiding this comment.
I tend to agree that it is probably fine to have empty libraries - but then we don't need an assertion here either. The user can create, serialize, deserialize etc. libraries without any export and it should be fine. Perhaps we may want to show warnings if this happens, but no need to error out.
One current exception is that we won't allow KernelLibrary to be empty. Technically, this doesn't have to be enforced either, but because of the way we set kernels in the ModuleGraph, it would be a bigger refactoring to remove this restriction.
There was a problem hiding this comment.
Not sure the restriction is needed for KernelLibrary either, since the "default" kernel is empty - as far as I'm aware the ModuleGraph isn't opinionated on this, the only issue being whether or not any syscalls are made, in which case we'd raise an error about an undefined kernel procedure.
This PR performs minor cleanup of the
AssemblerAPI (though, I think we can clean things up a bit more). It consist of mostly renaming things and moving code around - but there should be no changes which affect the overall functionality or semantics.The main changes are:
Assembler::add_compiled_library()intoAssembler::add_library().Assembler::with_compiled_library()intoAssembler::with_library().ModuleInfoandProcedureInfostructs into a separate file and renamed some methods ofModuleInfo.CompiledLibraryintoLibrary.LibraryErrorwithCompiledLibraryErrorand eliminated unused error variants.