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

chore: Push FileManager population up to nargo_cli #3844

Merged
merged 37 commits into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
4472e4a
make Context have a Cow of FileManager
kevaundray Dec 17, 2023
1bf00ac
prepare_package now takes a reference to FileManager
kevaundray Dec 17, 2023
8147bce
annotate noir_wasm context with 'static
kevaundray Dec 17, 2023
9d4ef7e
compile_ methods take a reference to file manager
kevaundray Dec 17, 2023
4a95500
modify lsp methods to now take a reference to file manager
kevaundray Dec 17, 2023
7fc4779
modify cmd modules to accomodate the reference to file manager
kevaundray Dec 17, 2023
76eb80a
add a method to add stdlib source into file manager
kevaundray Dec 17, 2023
d0877d6
add lifetime
kevaundray Dec 17, 2023
77a3cf5
clippy fix
kevaundray Dec 17, 2023
24277fa
fmt
kevaundray Dec 17, 2023
7552dcc
no longer add stdlib in prepare_crate
kevaundray Dec 17, 2023
cf79724
add `file_manager_with_stdlib` method
kevaundray Dec 17, 2023
7ee031f
noir_wasm: modify to now use file_manager_with_stdlib
kevaundray Dec 17, 2023
1d79e1e
modify all methods to use `file_manager_with_stdlib`
kevaundray Dec 17, 2023
ddf47fe
move `file_manager_with_stdlib` to noirc_driver
kevaundray Dec 17, 2023
dea3e5a
modify code to match
kevaundray Dec 17, 2023
25428ab
add comment
kevaundray Dec 17, 2023
1c010e7
modify comments
kevaundray Dec 17, 2023
c0dd164
typo in comment
kevaundray Dec 17, 2023
3d7e450
add note on assuming stdlib is already added
kevaundray Dec 17, 2023
f016ab0
Merge remote-tracking branch 'origin/master' into kw/make-file-manage…
kevaundray Dec 18, 2023
e37224c
merge fix
kevaundray Dec 18, 2023
8c0edd3
fix merge
kevaundray Dec 18, 2023
159886e
Merge remote-tracking branch 'origin/master' into kw/make-file-manage…
kevaundray Dec 18, 2023
debf5e7
fix merge
kevaundray Dec 18, 2023
c93e81b
Update tooling/nargo/src/ops/compile.rs
kevaundray Dec 19, 2023
5f16f73
Update compiler/wasm/src/compile_new.rs
kevaundray Dec 19, 2023
aa9c612
Update compiler/noirc_driver/src/lib.rs
kevaundray Dec 19, 2023
85542d7
Update compiler/noirc_frontend/src/hir/mod.rs
kevaundray Dec 19, 2023
8afcf9e
Merge branch 'master' into kw/make-file-manager-reference
kevaundray Dec 19, 2023
4d5d130
chore: inline default crate graphs
TomAFrench Dec 19, 2023
0846e69
use workspace root dir
kevaundray Dec 19, 2023
115cdef
use one file manager in fmt_cmd
kevaundray Dec 19, 2023
58d591d
resolve todo
kevaundray Dec 19, 2023
e17918f
fix todo
kevaundray Dec 19, 2023
e89c0c3
fmt
kevaundray Dec 19, 2023
1e9464c
remove extra imports
kevaundray Dec 19, 2023
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
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
10 changes: 7 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_package_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,14 @@ pub(super) fn on_did_save_text_document(
}
};

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);

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

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
9 changes: 7 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_package_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,12 @@ fn on_goto_definition_inner(

let mut definition_position = None;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);

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

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
12 changes: 10 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_package_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,9 +50,14 @@ fn on_profile_run_request_inner(
ResponseError::new(ErrorCode::REQUEST_FAILED, err)
})?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);

// Since we filtered on crate name, this should be the only item in the iterator
match workspace.into_iter().next() {
Some(_package) => {
// TODO: We should not be adding files for the entire workspace because
// TODO: we are filtering out some packages
insert_all_files_for_package_into_file_manager(_package, &mut workspace_file_manager);
let (binary_packages, contract_packages): (Vec<_>, Vec<_>) = workspace
.into_iter()
.filter(|package| !package.is_library())
Expand All @@ -60,6 +67,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
11 changes: 9 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_package_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,14 @@ fn on_test_run_request_inner(
ResponseError::new(ErrorCode::REQUEST_FAILED, err)
})?;

let mut workspace_file_manager = file_manager_with_stdlib(&workspace.root_dir);

// 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);
insert_all_files_for_package_into_file_manager(package, &mut workspace_file_manager);

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

let package_tests: Vec<_> = workspace
.into_iter()
.filter_map(|package| {
let (mut context, crate_id) = prepare_package(package);
insert_all_files_for_package_into_file_manager(package, &mut workspace_file_manager);
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
10 changes: 5 additions & 5 deletions tooling/nargo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,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