Skip to content

Commit

Permalink
Assembly: remove duplicate code and make recompute private (#1606)
Browse files Browse the repository at this point in the history 
* Assembly: share code between assemble_{library,kernel}

* Assembly: honor warnings_as_errors for assemble_library

* Assembly: error is already raised by library

* Assembly: make add_module return the index

* Assembly: error only for executable case

* Assembly: use existing add_module_with_options

* Assembly: add multiple modules and recompute graph only once

* Assembly: make recompute graph private
  • Loading branch information
@paracetamolo
paracetamolo authored Dec 20, 2024
1 parent 5944710 commit 8420dcd
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 81 deletions.
117 changes: 46 additions & 71 deletions assembly/src/assembler/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ impl Assembler {
///
/// The given module must be a library module, or an error will be returned.
#[inline]
pub fn add_module(&mut self, module: impl Compile) -> Result<(), Report> {
pub fn add_module(&mut self, module: impl Compile) -> Result<ModuleIndex, Report> {
self.add_module_with_options(module, CompileOptions::for_library())
}

Expand All @@ -169,22 +169,36 @@ impl Assembler {
&mut self,
module: impl Compile,
options: CompileOptions,
) -> Result<(), Report> {
) -> Result<ModuleIndex, Report> {
let ids = self.add_modules_with_options(vec![module], options)?;
Ok(ids[0])
}

pub fn add_modules_with_options(
&mut self,
modules: impl IntoIterator<Item = impl Compile>,
options: CompileOptions,
) -> Result<Vec<ModuleIndex>, Report> {
let kind = options.kind;
if kind != ModuleKind::Library {
return Err(Report::msg(
"only library modules are supported by `add_module_with_options`",
));
if kind == ModuleKind::Executable {
return Err(Report::msg("Executables are not supported by `add_module_with_options`"));
}

let module = module.compile_with_options(&self.source_manager, options)?;
assert_eq!(module.kind(), kind, "expected module kind to match compilation options");

self.module_graph.add_ast_module(module)?;

Ok(())
let modules = modules
.into_iter()
.map(|module| {
let module = module.compile_with_options(&self.source_manager, options.clone())?;
assert_eq!(
module.kind(),
kind,
"expected module kind to match compilation options"
);
Ok(module)
})
.collect::<Result<Vec<_>, Report>>()?;
let ids = self.module_graph.add_ast_modules(modules.into_iter())?;
Ok(ids)
}

/// Adds all modules (defined by ".masm" files) from the specified directory to the module
/// of this assembler graph.
///
Expand All @@ -202,10 +216,8 @@ impl Assembler {
namespace: crate::LibraryNamespace,
dir: &std::path::Path,
) -> Result<(), Report> {
for module in crate::parser::read_modules_from_dir(namespace, dir, &self.source_manager)? {
self.module_graph.add_ast_module(module)?;
}

let modules = crate::parser::read_modules_from_dir(namespace, dir, &self.source_manager)?;
self.module_graph.add_ast_modules(modules)?;
Ok(())
}

Expand Down Expand Up @@ -281,24 +293,12 @@ impl Assembler {
/// # Errors
///
/// Returns an error if parsing or compilation of the specified modules fails.
pub fn assemble_library(
pub fn assemble_common(
mut self,
modules: impl IntoIterator<Item = impl Compile>,
options: CompileOptions,
) -> Result<Library, Report> {
let ast_module_indices =
modules.into_iter().try_fold(Vec::default(), |mut acc, module| {
module
.compile_with_options(&self.source_manager, CompileOptions::for_library())
.and_then(|module| {
self.module_graph.add_ast_module(module).map_err(Report::from)
})
.map(move |module_id| {
acc.push(module_id);
acc
})
})?;

self.module_graph.recompute()?;
let ast_module_indices = self.add_modules_with_options(modules, options)?;

let mut mast_forest_builder = MastForestBuilder::default();

Expand All @@ -325,7 +325,6 @@ impl Assembler {
exports
};

// TODO: show a warning if library exports are empty?
let (mast_forest, id_remappings) = mast_forest_builder.build();
if let Some(id_remappings) = id_remappings {
for (_proc_name, node_id) in exports.iter_mut() {
Expand All @@ -338,53 +337,30 @@ impl Assembler {
Ok(Library::new(mast_forest.into(), exports)?)
}

pub fn assemble_library(
self,
modules: impl IntoIterator<Item = impl Compile>,
) -> Result<Library, Report> {
let options = CompileOptions {
kind: ModuleKind::Library,
warnings_as_errors: self.warnings_as_errors,
path: None,
};
self.assemble_common(modules, options)
}

/// Assembles the provided module into a [KernelLibrary] intended to be used as a Kernel.
///
/// # Errors
///
/// Returns an error if parsing or compilation of the specified modules fails.
pub fn assemble_kernel(mut self, module: impl Compile) -> Result<KernelLibrary, Report> {
pub fn assemble_kernel(self, module: impl Compile) -> Result<KernelLibrary, Report> {
let options = CompileOptions {
kind: ModuleKind::Kernel,
warnings_as_errors: self.warnings_as_errors,
path: Some(LibraryPath::from(LibraryNamespace::Kernel)),
};

let module = module.compile_with_options(&self.source_manager, options)?;
let module_idx = self.module_graph.add_ast_module(module)?;

self.module_graph.recompute()?;

let mut mast_forest_builder = MastForestBuilder::default();

// Note: it is safe to use `unwrap_ast()` here, since all modules looped over are
// AST (we just added them to the module graph)
let ast_module = self.module_graph[module_idx].unwrap_ast().clone();

let mut exports = ast_module
.exported_procedures()
.map(|(proc_idx, fqn)| {
let gid = module_idx + proc_idx;
self.compile_subgraph(gid, &mut mast_forest_builder)?;

let proc_root_node_id = mast_forest_builder
.get_procedure(gid)
.expect("compilation succeeded but root not found in cache")
.body_node_id();
Ok((fqn, proc_root_node_id))
})
.collect::<Result<BTreeMap<_, _>, Report>>()?;

// TODO: show a warning if library exports are empty?
let (mast_forest, id_remappings) = mast_forest_builder.build();
if let Some(id_remappings) = id_remappings {
for (_proc_name, node_id) in exports.iter_mut() {
if let Some(&new_node_id) = id_remappings.get(node_id) {
*node_id = new_node_id;
}
}
}
let library = Library::new(mast_forest.into(), exports)?;
let library = self.assemble_common([module], options)?;
Ok(library.try_into()?)
}

Expand All @@ -407,7 +383,6 @@ impl Assembler {

// Recompute graph with executable module, and start compiling
let ast_module_index = self.module_graph.add_ast_module(program)?;
self.module_graph.recompute()?;

// Find the executable entrypoint Note: it is safe to use `unwrap_ast()` here, since this is
// the module we just added, which is in AST representation.
Expand Down
17 changes: 15 additions & 2 deletions assembly/src/assembler/module_graph/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,9 @@ impl ModuleGraph {
/// This function will panic if the number of modules exceeds the maximum representable
/// [ModuleIndex] value, `u16::MAX`.
pub fn add_ast_module(&mut self, module: Box<Module>) -> Result<ModuleIndex, AssemblyError> {
self.add_module(PendingWrappedModule::Ast(module))
let res = self.add_module(PendingWrappedModule::Ast(module))?;
self.recompute()?;
Ok(res)
}

fn add_module(&mut self, module: PendingWrappedModule) -> Result<ModuleIndex, AssemblyError> {
Expand All @@ -238,6 +240,17 @@ impl ModuleGraph {
Ok(module_id)
}

pub fn add_ast_modules(
&mut self,
modules: impl Iterator<Item = Box<Module>>,
) -> Result<Vec<ModuleIndex>, AssemblyError> {
let idx = modules
.into_iter()
.map(|m| self.add_module(PendingWrappedModule::Ast(m)))
.collect::<Result<Vec<ModuleIndex>, _>>()?;
self.recompute()?;
Ok(idx)
}
fn is_pending(&self, path: &LibraryPath) -> bool {
self.pending.iter().any(|m| m.path() == path)
}
Expand Down Expand Up @@ -329,7 +342,7 @@ impl ModuleGraph {
/// NOTE: This will return `Err` if we detect a validation error, a cycle in the graph, or an
/// operation not supported by the current configuration. Basically, for any reason that would
/// cause the resulting graph to represent an invalid program.
pub fn recompute(&mut self) -> Result<(), AssemblyError> {
fn recompute(&mut self) -> Result<(), AssemblyError> {
// It is acceptable for there to be no changes, but if the graph is empty and no changes
// are being made, we treat that as an error
if self.modules.is_empty() && self.pending.is_empty() {
Expand Down
18 changes: 10 additions & 8 deletions assembly/src/testing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ impl TestContext {
/// other modules.
#[track_caller]
pub fn add_module(&mut self, module: impl Compile) -> Result<(), Report> {
self.assembler.add_module(module)
self.assembler.add_module(module).map(|_| ())
}

/// Add a module to the [Assembler] constructed by this context, with the fully-qualified
Expand All @@ -305,13 +305,15 @@ impl TestContext {
path: LibraryPath,
source: impl Compile,
) -> Result<(), Report> {
self.assembler.add_module_with_options(
source,
CompileOptions {
path: Some(path),
..CompileOptions::for_library()
},
)
self.assembler
.add_module_with_options(
source,
CompileOptions {
path: Some(path),
..CompileOptions::for_library()
},
)
.map(|_| ())
}

/// Add the modules of `library` to the [Assembler] constructed by this context.
Expand Down

0 comments on commit 8420dcd

Please sign in to comment.