Skip to content
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

feat: Extract parsing to its own pass and do it in parallel #4063

Merged
merged 7 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions compiler/fm/src/file_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ impl FileMap {
pub fn get_file_id(&self, file_name: &PathString) -> Option<FileId> {
self.name_to_id.get(file_name).cloned()
}

pub fn all_file_ids(&self) -> impl Iterator<Item = &FileId> {
self.name_to_id.values()
}
}
impl Default for FileMap {
fn default() -> Self {
Expand Down
9 changes: 7 additions & 2 deletions compiler/noirc_driver/tests/stdlib_warnings.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::path::Path;

use noirc_driver::{file_manager_with_stdlib, prepare_crate, ErrorsAndWarnings};
use noirc_frontend::hir::Context;
use noirc_frontend::hir::{def_map::parse_file, Context};

#[test]
fn stdlib_does_not_produce_constant_warnings() -> Result<(), ErrorsAndWarnings> {
Expand All @@ -15,8 +15,13 @@ fn stdlib_does_not_produce_constant_warnings() -> Result<(), ErrorsAndWarnings>
file_manager.add_file_with_source(file_name, source.to_owned()).expect(
"Adding source buffer to file manager should never fail when file manager is empty",
);
let parsed_files = file_manager
.as_file_map()
.all_file_ids()
.map(|&file_id| (file_id, parse_file(&file_manager, file_id)))
.collect();

let mut context = Context::new(file_manager);
let mut context = Context::new(file_manager, parsed_files);
let root_crate_id = prepare_crate(&mut context, file_name);

let ((), warnings) = noirc_driver::check_crate(&mut context, root_crate_id, false, false)?;
Expand Down
4 changes: 2 additions & 2 deletions compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use super::{
},
errors::{DefCollectorErrorKind, DuplicateType},
};
use crate::hir::def_map::{parse_file, LocalModuleId, ModuleData, ModuleId};
use crate::hir::def_map::{LocalModuleId, ModuleData, ModuleId};
use crate::hir::resolution::import::ImportDirective;
use crate::hir::Context;

Expand Down Expand Up @@ -555,7 +555,7 @@ impl<'a> ModCollector<'a> {
context.visited_files.insert(child_file_id, location);

// Parse the AST for the module we just found and then recursively look for it's defs
let (ast, parsing_errors) = parse_file(&context.file_manager, child_file_id);
let (ast, parsing_errors) = context.parsed_file_results(child_file_id);
let ast = ast.into_sorted();

errors.extend(
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/hir/def_map/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl CrateDefMap {

// First parse the root file.
let root_file_id = context.crate_graph[crate_id].root_file_id;
let (ast, parsing_errors) = parse_file(&context.file_manager, root_file_id);
let (ast, parsing_errors) = context.parsed_file_results(root_file_id);
let mut ast = ast.into_sorted();

for macro_processor in &macro_processors {
Expand Down
28 changes: 23 additions & 5 deletions compiler/noirc_frontend/src/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,22 @@ pub mod type_check;
use crate::graph::{CrateGraph, CrateId};
use crate::hir_def::function::FuncMeta;
use crate::node_interner::{FuncId, NodeInterner, StructId};
use crate::parser::ParserError;
use crate::ParsedModule;
use def_map::{Contract, CrateDefMap};
use fm::FileManager;
use noirc_errors::Location;
use std::borrow::Cow;
use std::collections::BTreeMap;
use std::collections::{BTreeMap, HashMap};

use self::def_map::TestFunction;

pub type ParsedFiles = HashMap<fm::FileId, (ParsedModule, Vec<ParserError>)>;

/// Helper object which groups together several useful context objects used
/// during name resolution. Once name resolution is finished, only the
/// def_interner is required for type inference and monomorphization.
pub struct Context<'file_manager> {
pub struct Context<'file_manager, 'parsed_files> {
pub def_interner: NodeInterner,
pub crate_graph: CrateGraph,
pub(crate) def_maps: BTreeMap<CrateId, CrateDefMap>,
Expand All @@ -30,6 +34,11 @@ pub struct Context<'file_manager> {
/// A map of each file that already has been visited from a prior `mod foo;` declaration.
/// This is used to issue an error if a second `mod foo;` is declared to the same file.
pub visited_files: BTreeMap<fm::FileId, Location>,

// A map of all parsed files.
// Same as the file manager, we take ownership of the parsed files in the WASM context.
// Parsed files is also read only.
pub parsed_files: Cow<'parsed_files, ParsedFiles>,
}

#[derive(Debug, Copy, Clone)]
Expand All @@ -39,27 +48,36 @@ pub enum FunctionNameMatch<'a> {
Contains(&'a str),
}

impl Context<'_> {
pub fn new(file_manager: FileManager) -> Context<'static> {
impl Context<'_, '_> {
pub fn new(file_manager: FileManager, parsed_files: ParsedFiles) -> Context<'static, 'static> {
Context {
def_interner: NodeInterner::default(),
def_maps: BTreeMap::new(),
visited_files: BTreeMap::new(),
crate_graph: CrateGraph::default(),
file_manager: Cow::Owned(file_manager),
parsed_files: Cow::Owned(parsed_files),
}
}

pub fn from_ref_file_manager(file_manager: &FileManager) -> Context<'_> {
pub fn from_ref_file_manager<'file_manager, 'parsed_files>(
file_manager: &'file_manager FileManager,
parsed_files: &'parsed_files ParsedFiles,
) -> Context<'file_manager, 'parsed_files> {
Context {
def_interner: NodeInterner::default(),
def_maps: BTreeMap::new(),
visited_files: BTreeMap::new(),
crate_graph: CrateGraph::default(),
file_manager: Cow::Borrowed(file_manager),
parsed_files: Cow::Borrowed(parsed_files),
}
}

pub fn parsed_file_results(&self, file_id: fm::FileId) -> (ParsedModule, Vec<ParserError>) {
self.parsed_files.get(&file_id).expect("noir file wasn't parsed").clone()
}

/// Returns the CrateDefMap for a given CrateId.
/// It is perfectly valid for the compiler to look
/// up a CrateDefMap and it is not available.
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ mod test {
) -> (ParsedModule, Context, Vec<(CompilationError, FileId)>) {
let root = std::path::Path::new("/");
let fm = FileManager::new(root);
let mut context = Context::new(fm);
let mut context = Context::new(fm, Default::default());
context.def_interner.populate_dummy_operator_traits();
let root_file_id = FileId::dummy();
let root_crate_id = context.crate_graph.add_crate_root(root_file_id);
Expand Down
19 changes: 13 additions & 6 deletions compiler/wasm/src/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use noirc_driver::{
};
use noirc_frontend::{
graph::{CrateId, CrateName},
hir::Context,
hir::{def_map::parse_file, Context, ParsedFiles},
};
use serde::Deserialize;
use std::{collections::HashMap, path::Path};
Expand Down Expand Up @@ -147,6 +147,10 @@ impl PathToFileSourceMap {
}
}

pub(crate) fn parse_all(fm: &FileManager) -> ParsedFiles {
fm.as_file_map().all_file_ids().map(|&file_id| (file_id, parse_file(fm, file_id))).collect()
}

pub enum CompileResult {
Contract { contract: ContractArtifact, debug: DebugArtifact },
Program { program: ProgramArtifact, debug: DebugArtifact },
Expand All @@ -169,8 +173,8 @@ pub fn compile(
};

let fm = file_manager_with_source_map(file_source_map);

let mut context = Context::new(fm);
let parsed_files = parse_all(&fm);
let mut context = Context::new(fm, parsed_files);

let path = Path::new(&entry_point);
let crate_id = prepare_crate(&mut context, path);
Expand Down Expand Up @@ -307,15 +311,18 @@ mod test {

use crate::compile::PathToFileSourceMap;

use super::{file_manager_with_source_map, process_dependency_graph, DependencyGraph};
use super::{
file_manager_with_source_map, parse_all, process_dependency_graph, DependencyGraph,
};
use std::{collections::HashMap, path::Path};

fn setup_test_context(source_map: PathToFileSourceMap) -> Context<'static> {
fn setup_test_context(source_map: PathToFileSourceMap) -> Context<'static, 'static> {
let mut fm = file_manager_with_source_map(source_map);
// Add this due to us calling prepare_crate on "/main.nr" below
fm.add_file_with_source(Path::new("/main.nr"), "fn foo() {}".to_string());
let parsed_files = parse_all(&fm);

let mut context = Context::new(fm);
let mut context = Context::new(fm, parsed_files);
prepare_crate(&mut context, Path::new("/main.nr"));

context
Expand Down
13 changes: 8 additions & 5 deletions compiler/wasm/src/compile_new.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use crate::compile::{
file_manager_with_source_map, generate_contract_artifact, generate_program_artifact,
file_manager_with_source_map, generate_contract_artifact, generate_program_artifact, parse_all,
JsCompileResult, PathToFileSourceMap,
};
use crate::errors::{CompileError, JsCompileError};
Expand All @@ -20,7 +20,7 @@ use wasm_bindgen::prelude::wasm_bindgen;
pub struct CompilerContext {
// `wasm_bindgen` currently doesn't allow lifetime parameters on structs so we must use a `'static` lifetime.
// `Context` must then own the `FileManager` to satisfy this lifetime.
context: Context<'static>,
context: Context<'static, 'static>,
}

#[wasm_bindgen(js_name = "CrateId")]
Expand All @@ -34,7 +34,9 @@ impl CompilerContext {
console_error_panic_hook::set_once();

let fm = file_manager_with_source_map(source_map);
CompilerContext { context: Context::new(fm) }
let parsed_files = parse_all(&fm);

CompilerContext { context: Context::new(fm, parsed_files) }
}

#[cfg(test)]
Expand Down Expand Up @@ -231,7 +233,7 @@ mod test {
use noirc_driver::prepare_crate;
use noirc_frontend::hir::Context;

use crate::compile::{file_manager_with_source_map, PathToFileSourceMap};
use crate::compile::{file_manager_with_source_map, parse_all, PathToFileSourceMap};

use std::path::Path;

Expand All @@ -241,8 +243,9 @@ mod test {
let mut fm = file_manager_with_source_map(source_map);
// Add this due to us calling prepare_crate on "/main.nr" below
fm.add_file_with_source(Path::new("/main.nr"), "fn foo() {}".to_string());
let parsed_files = parse_all(&fm);

let mut context = Context::new(fm);
let mut context = Context::new(fm, parsed_files);
prepare_crate(&mut context, Path::new("/main.nr"));

CompilerContext { context }
Expand Down
7 changes: 4 additions & 3 deletions tooling/lsp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use async_lsp::{
};
use fm::codespan_files as files;
use lsp_types::CodeLens;
use nargo::workspace::Workspace;
use nargo::{parse_all, workspace::Workspace};
use nargo_toml::{find_file_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{file_manager_with_stdlib, prepare_crate, NOIR_ARTIFACT_VERSION_STRING};
use noirc_frontend::{
Expand Down Expand Up @@ -227,15 +227,16 @@ pub(crate) fn resolve_workspace_for_source_path(file_path: &Path) -> Result<Work
/// Use case for this is the LSP server and code lenses
/// which operate on single file and need to understand this file
/// in order to offer code lenses to the user
fn prepare_source(source: String) -> (Context<'static>, CrateId) {
fn prepare_source(source: String) -> (Context<'static, 'static>, CrateId) {
let root = Path::new("");
let file_name = Path::new("main.nr");
let mut file_manager = file_manager_with_stdlib(root);
file_manager.add_file_with_source(file_name, source).expect(
"Adding source buffer to file manager should never fail when file manager is empty",
);
let parsed_files = parse_all(&file_manager);

let mut context = Context::new(file_manager);
let mut context = Context::new(file_manager, parsed_files);
let root_crate_id = prepare_crate(&mut context, file_name);

(context, root_crate_id)
Expand Down
6 changes: 4 additions & 2 deletions tooling/lsp/src/notifications/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::ops::ControlFlow;

use async_lsp::{ErrorCode, LanguageClient, ResponseError};
use nargo::{insert_all_files_for_workspace_into_file_manager, prepare_package};
use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all, prepare_package};
use noirc_driver::{check_crate, file_manager_with_stdlib};
use noirc_errors::{DiagnosticKind, FileDiagnostic};

Expand Down Expand Up @@ -130,11 +130,13 @@ fn process_noir_document(

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);
kevaundray marked this conversation as resolved.
Show resolved Hide resolved

let diagnostics: Vec<_> = workspace
.into_iter()
.flat_map(|package| -> Vec<Diagnostic> {
let (mut context, crate_id) = prepare_package(&workspace_file_manager, package);
let (mut context, crate_id) =
prepare_package(&workspace_file_manager, &parsed_files, package);

let file_diagnostics = match check_crate(&mut context, crate_id, false, false) {
Ok(((), warnings)) => warnings,
Expand Down
6 changes: 4 additions & 2 deletions tooling/lsp/src/requests/goto_declaration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use async_lsp::{ErrorCode, ResponseError};

use lsp_types::request::{GotoDeclarationParams, GotoDeclarationResponse};

use nargo::insert_all_files_for_workspace_into_file_manager;
use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all};
use noirc_driver::file_manager_with_stdlib;

use super::{position_to_byte_index, to_lsp_location};
Expand Down Expand Up @@ -36,8 +36,10 @@ fn on_goto_definition_inner(

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

let (mut context, crate_id) = nargo::prepare_package(&workspace_file_manager, package);
let (mut context, crate_id) =
nargo::prepare_package(&workspace_file_manager, &parsed_files, package);

let interner;
if let Some(def_interner) = _state.cached_definitions.get(&package_root_path) {
Expand Down
6 changes: 4 additions & 2 deletions tooling/lsp/src/requests/goto_definition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use async_lsp::{ErrorCode, ResponseError};

use lsp_types::request::GotoTypeDefinitionParams;
use lsp_types::{GotoDefinitionParams, GotoDefinitionResponse};
use nargo::insert_all_files_for_workspace_into_file_manager;
use nargo::{insert_all_files_for_workspace_into_file_manager, parse_all};
use noirc_driver::file_manager_with_stdlib;

use super::{position_to_byte_index, to_lsp_location};
Expand Down Expand Up @@ -44,8 +44,10 @@ fn on_goto_definition_inner(

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

let (mut context, crate_id) = nargo::prepare_package(&workspace_file_manager, package);
let (mut context, crate_id) =
nargo::prepare_package(&workspace_file_manager, &parsed_files, package);

let interner;
if let Some(def_interner) = _state.cached_definitions.get(&package_root_path) {
Expand Down
6 changes: 5 additions & 1 deletion tooling/lsp/src/requests/profile_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ use std::{

use acvm::ExpressionWidth;
use async_lsp::{ErrorCode, ResponseError};
use nargo::{artifacts::debug::DebugArtifact, insert_all_files_for_workspace_into_file_manager};
use nargo::{
artifacts::debug::DebugArtifact, insert_all_files_for_workspace_into_file_manager, parse_all,
};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{
file_manager_with_stdlib, CompileOptions, DebugFile, NOIR_ARTIFACT_VERSION_STRING,
Expand Down Expand Up @@ -52,6 +54,7 @@ fn on_profile_run_request_inner(

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

// Since we filtered on crate name, this should be the only item in the iterator
match workspace.into_iter().next() {
Expand All @@ -66,6 +69,7 @@ fn on_profile_run_request_inner(

let (compiled_programs, compiled_contracts) = nargo::ops::compile_workspace(
&workspace_file_manager,
&parsed_files,
&workspace,
&binary_packages,
&contract_packages,
Expand Down
6 changes: 4 additions & 2 deletions tooling/lsp/src/requests/test_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use async_lsp::{ErrorCode, ResponseError};
use nargo::{
insert_all_files_for_workspace_into_file_manager,
ops::{run_test, TestStatus},
prepare_package,
parse_all, prepare_package,
};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{
Expand Down Expand Up @@ -52,11 +52,13 @@ fn on_test_run_request_inner(

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);
insert_all_files_for_workspace_into_file_manager(&workspace, &mut workspace_file_manager);
let parsed_files = parse_all(&workspace_file_manager);

// Since we filtered on crate name, this should be the only item in the iterator
match workspace.into_iter().next() {
Some(package) => {
let (mut context, crate_id) = prepare_package(&workspace_file_manager, package);
let (mut context, crate_id) =
prepare_package(&workspace_file_manager, &parsed_files, package);
if check_crate(&mut context, crate_id, false, false).is_err() {
let result = NargoTestRunResult {
id: params.id.clone(),
Expand Down
Loading
Loading