Skip to content

Commit

Permalink
chore: Push FileManager population up to nargo_cli (#3844)
Browse files Browse the repository at this point in the history
# Description

This tries to undo some of the changes done in #3496 where cli methods
were moved into the nargo library. This pushes the populating of the
file manager up to the cli commands, in most places this is just after
the workspace has been resolved.

A possible next step after this is to have the nargo library to no
longer ask for a file manager struct, but instead it asks for a
FileManager trait which the previous FileManager struct would implement.

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*



## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <[email protected]>
Co-authored-by: Tom French <[email protected]>
  • Loading branch information
3 people authored Dec 19, 2023
1 parent bcbd49b commit 04dc478
Show file tree
Hide file tree
Showing 23 changed files with 275 additions and 111 deletions.
2 changes: 1 addition & 1 deletion compiler/fm/src/file_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ impl From<&PathBuf> for PathString {
PathString::from(pb.to_owned())
}
}
#[derive(Debug)]
#[derive(Debug, Clone)]
pub struct FileMap {
files: SimpleFiles<PathString, String>,
name_to_id: HashMap<PathString, FileId>,
Expand Down
2 changes: 1 addition & 1 deletion compiler/fm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use std::{
};

pub const FILE_EXTENSION: &str = "nr";

#[derive(Clone)]
pub struct FileManager {
root: PathBuf,
file_map: FileMap,
Expand Down
27 changes: 23 additions & 4 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
#![warn(clippy::semicolon_if_nothing_returned)]

use clap::Args;
use fm::FileId;
use fm::{FileId, FileManager};
use iter_extended::vecmap;
use noirc_abi::{AbiParameter, AbiType, ContractEvent};
use noirc_errors::{CustomDiagnostic, FileDiagnostic};
Expand Down Expand Up @@ -81,17 +81,36 @@ pub type ErrorsAndWarnings = Vec<FileDiagnostic>;
/// Helper type for connecting a compilation artifact to the errors or warnings which were produced during compilation.
pub type CompilationResult<T> = Result<(T, Warnings), ErrorsAndWarnings>;

/// Adds the file from the file system at `Path` to the crate graph as a root file
pub fn prepare_crate(context: &mut Context, file_name: &Path) -> CrateId {
/// Helper method to return a file manager instance with the stdlib already added
///
/// TODO: This should become the canonical way to create a file manager and
/// TODO if we use a File manager trait, we can move file manager into this crate
/// TODO as a module
pub fn file_manager_with_stdlib(root: &Path) -> FileManager {
let mut file_manager = FileManager::new(root);

add_stdlib_source_to_file_manager(&mut file_manager);

file_manager
}

/// Adds the source code for the stdlib into the file manager
fn add_stdlib_source_to_file_manager(file_manager: &mut FileManager) {
// Add the stdlib contents to the file manager, since every package automatically has a dependency
// on the stdlib. For other dependencies, we read the package.Dependencies file to add their file
// contents to the file manager. However since the dependency on the stdlib is implicit, we need
// to manually add it here.
let stdlib_paths_with_source = stdlib::stdlib_paths_with_source();
for (path, source) in stdlib_paths_with_source {
context.file_manager.add_file_with_source_canonical_path(Path::new(&path), source);
file_manager.add_file_with_source_canonical_path(Path::new(&path), source);
}
}

/// Adds the file from the file system at `Path` to the crate graph as a root file
///
/// Note: This methods adds the stdlib as a dependency to the crate.
/// This assumes that the stdlib has already been added to the file manager.
pub fn prepare_crate(context: &mut Context, file_name: &Path) -> CrateId {
let path_to_std_lib_file = Path::new(STD_CRATE_NAME).join("lib.nr");
let std_file_id = context
.file_manager
Expand Down
27 changes: 20 additions & 7 deletions compiler/noirc_frontend/src/hir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,22 @@ use crate::node_interner::{FuncId, NodeInterner, StructId};
use def_map::{Contract, CrateDefMap};
use fm::FileManager;
use noirc_errors::Location;
use std::borrow::Cow;
use std::collections::BTreeMap;

use self::def_map::TestFunction;

/// 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 {
pub struct Context<'file_manager> {
pub def_interner: NodeInterner,
pub crate_graph: CrateGraph,
pub(crate) def_maps: BTreeMap<CrateId, CrateDefMap>,
pub file_manager: FileManager,
// In the WASM context, we take ownership of the file manager,
// which is why this needs to be a Cow. In all use-cases, the file manager
// is read-only however, once it has been passed to the Context.
pub file_manager: Cow<'file_manager, FileManager>,

/// 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.
Expand All @@ -35,15 +39,24 @@ pub enum FunctionNameMatch<'a> {
Contains(&'a str),
}

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

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

Expand Down
9 changes: 5 additions & 4 deletions compiler/wasm/src/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,9 @@ use nargo::artifacts::{
program::PreprocessedProgram,
};
use noirc_driver::{
add_dep, compile_contract, compile_main, prepare_crate, prepare_dependency, CompileOptions,
CompiledContract, CompiledProgram, NOIR_ARTIFACT_VERSION_STRING,
add_dep, compile_contract, compile_main, file_manager_with_stdlib, prepare_crate,
prepare_dependency, CompileOptions, CompiledContract, CompiledProgram,
NOIR_ARTIFACT_VERSION_STRING,
};
use noirc_frontend::{
graph::{CrateId, CrateName},
Expand Down Expand Up @@ -224,7 +225,7 @@ pub fn compile(
// should be considered as immutable.
pub(crate) fn file_manager_with_source_map(source_map: PathToFileSourceMap) -> FileManager {
let root = Path::new("");
let mut fm = FileManager::new(root);
let mut fm = file_manager_with_stdlib(root);

for (path, source) in source_map.0 {
fm.add_file_with_source(path.as_path(), source);
Expand Down Expand Up @@ -327,7 +328,7 @@ mod test {
use super::{file_manager_with_source_map, process_dependency_graph, DependencyGraph};
use std::{collections::HashMap, path::Path};

fn setup_test_context(source_map: PathToFileSourceMap) -> Context {
fn setup_test_context(source_map: PathToFileSourceMap) -> Context<'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());
Expand Down
4 changes: 3 additions & 1 deletion compiler/wasm/src/compile_new.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ use wasm_bindgen::prelude::wasm_bindgen;
/// then the impl block is not picked up in javascript.
#[wasm_bindgen]
pub struct CompilerContext {
context: Context,
// `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>,
}

#[wasm_bindgen(js_name = "CrateId")]
Expand Down
9 changes: 6 additions & 3 deletions tooling/lsp/src/notifications/mod.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use std::ops::ControlFlow;

use async_lsp::{ErrorCode, LanguageClient, ResponseError};
use nargo::prepare_package;
use nargo::{insert_all_files_for_workspace_into_file_manager, prepare_package};
use nargo_toml::{find_file_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{check_crate, NOIR_ARTIFACT_VERSION_STRING};
use noirc_driver::{check_crate, file_manager_with_stdlib, NOIR_ARTIFACT_VERSION_STRING};
use noirc_errors::{DiagnosticKind, FileDiagnostic};

use crate::types::{
Expand Down Expand Up @@ -100,10 +100,13 @@ pub(super) fn on_did_save_text_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 diagnostics: Vec<_> = workspace
.into_iter()
.flat_map(|package| -> Vec<Diagnostic> {
let (mut context, crate_id) = prepare_package(package);
let (mut context, crate_id) = prepare_package(&workspace_file_manager, package);

let file_diagnostics = match check_crate(&mut context, crate_id, false, false) {
Ok(((), warnings)) => warnings,
Expand Down
8 changes: 6 additions & 2 deletions tooling/lsp/src/requests/goto_definition.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ use async_lsp::{ErrorCode, LanguageClient, ResponseError};
use fm::codespan_files::Error;
use lsp_types::{GotoDefinitionParams, GotoDefinitionResponse, Location};
use lsp_types::{Position, Url};
use nargo::insert_all_files_for_workspace_into_file_manager;
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::NOIR_ARTIFACT_VERSION_STRING;
use noirc_driver::{file_manager_with_stdlib, NOIR_ARTIFACT_VERSION_STRING};

pub(crate) fn on_goto_definition_request(
state: &mut LspState,
Expand Down Expand Up @@ -51,8 +52,11 @@ fn on_goto_definition_inner(

let mut definition_position = None;

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);

for package in &workspace {
let (mut context, crate_id) = nargo::prepare_package(package);
let (mut context, crate_id) = nargo::prepare_package(&workspace_file_manager, package);

// We ignore the warnings and errors produced by compilation while resolving the definition
let _ = noirc_driver::check_crate(&mut context, crate_id, false, false);
Expand Down
10 changes: 8 additions & 2 deletions tooling/lsp/src/requests/profile_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ use std::{

use acvm::ExpressionWidth;
use async_lsp::{ErrorCode, ResponseError};
use nargo::artifacts::debug::DebugArtifact;
use nargo::{artifacts::debug::DebugArtifact, insert_all_files_for_workspace_into_file_manager};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{CompileOptions, DebugFile, NOIR_ARTIFACT_VERSION_STRING};
use noirc_driver::{
file_manager_with_stdlib, CompileOptions, DebugFile, NOIR_ARTIFACT_VERSION_STRING,
};
use noirc_errors::{debug_info::OpCodesCount, Location};

use crate::{
Expand Down Expand Up @@ -48,6 +50,9 @@ fn on_profile_run_request_inner(
ResponseError::new(ErrorCode::REQUEST_FAILED, err)
})?;

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);

// Since we filtered on crate name, this should be the only item in the iterator
match workspace.into_iter().next() {
Some(_package) => {
Expand All @@ -60,6 +65,7 @@ fn on_profile_run_request_inner(
let expression_width = ExpressionWidth::Bounded { width: 3 };

let (compiled_programs, compiled_contracts) = nargo::ops::compile_workspace(
&workspace_file_manager,
&workspace,
&binary_packages,
&contract_packages,
Expand Down
10 changes: 8 additions & 2 deletions tooling/lsp/src/requests/test_run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@ use std::future::{self, Future};

use async_lsp::{ErrorCode, ResponseError};
use nargo::{
insert_all_files_for_workspace_into_file_manager,
ops::{run_test, TestStatus},
prepare_package,
};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{check_crate, CompileOptions, NOIR_ARTIFACT_VERSION_STRING};
use noirc_driver::{
check_crate, file_manager_with_stdlib, CompileOptions, NOIR_ARTIFACT_VERSION_STRING,
};
use noirc_frontend::hir::FunctionNameMatch;

use crate::{
Expand Down Expand Up @@ -47,10 +50,13 @@ fn on_test_run_request_inner(
ResponseError::new(ErrorCode::REQUEST_FAILED, err)
})?;

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);

// 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(package);
let (mut context, crate_id) = prepare_package(&workspace_file_manager, package);
if check_crate(&mut context, crate_id, false, false).is_err() {
let result = NargoTestRunResult {
id: params.id.clone(),
Expand Down
9 changes: 6 additions & 3 deletions tooling/lsp/src/requests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ use std::future::{self, Future};

use async_lsp::{ErrorCode, LanguageClient, ResponseError};
use lsp_types::{LogMessageParams, MessageType};
use nargo::prepare_package;
use nargo::{insert_all_files_for_workspace_into_file_manager, prepare_package};
use nargo_toml::{find_package_manifest, resolve_workspace_from_toml, PackageSelection};
use noirc_driver::{check_crate, NOIR_ARTIFACT_VERSION_STRING};
use noirc_driver::{check_crate, file_manager_with_stdlib, NOIR_ARTIFACT_VERSION_STRING};

use crate::{
get_package_tests_in_crate,
Expand Down Expand Up @@ -50,10 +50,13 @@ fn on_tests_request_inner(
ResponseError::new(ErrorCode::REQUEST_FAILED, err)
})?;

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 package_tests: Vec<_> = workspace
.into_iter()
.filter_map(|package| {
let (mut context, crate_id) = prepare_package(package);
let (mut context, crate_id) = prepare_package(&workspace_file_manager, package);
// We ignore the warnings and errors produced by compilation for producing tests
// because we can still get the test functions even if compilation fails
let _ = check_crate(&mut context, crate_id, false, false);
Expand Down
20 changes: 14 additions & 6 deletions tooling/nargo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,21 @@ pub fn prepare_dependencies(
}
}

pub fn insert_all_files_for_workspace_into_file_manager(
workspace: &workspace::Workspace,
file_manager: &mut FileManager,
) {
for package in workspace.clone().into_iter() {
insert_all_files_for_package_into_file_manager(package, file_manager);
}
}
// We will pre-populate the file manager with all the files in the package
// This is so that we can avoid having to read from disk when we are compiling
//
// This does not require parsing because we are interested in the files under the src directory
// it may turn out that we do not need to include some Noir files that we add to the file
// manager
pub fn insert_all_files_for_package_into_file_manager(
fn insert_all_files_for_package_into_file_manager(
package: &Package,
file_manager: &mut FileManager,
) {
Expand Down Expand Up @@ -87,11 +95,11 @@ fn insert_all_files_for_packages_dependencies_into_file_manager(
}
}

pub fn prepare_package(package: &Package) -> (Context, CrateId) {
let mut fm = FileManager::new(&package.root_dir);
insert_all_files_for_package_into_file_manager(package, &mut fm);

let mut context = Context::new(fm);
pub fn prepare_package<'file_manager>(
file_manager: &'file_manager FileManager,
package: &Package,
) -> (Context<'file_manager>, CrateId) {
let mut context = Context::from_ref_file_manager(file_manager);

let crate_id = prepare_crate(&mut context, &package.entry_path);

Expand Down
Loading

0 comments on commit 04dc478

Please sign in to comment.