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!: Make file manager read-only to the compiler #3760

Merged
merged 36 commits into from
Dec 14, 2023
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
7e0a3d1
make `find_module` be an immutable reference
kevaundray Dec 10, 2023
b2ff05b
add method in nargo that pre-populates the FileManager
kevaundray Dec 10, 2023
5592348
change add_file to `name_to_id` -- we assume that the file manager ha…
kevaundray Dec 10, 2023
d5fd4e0
cargo
kevaundray Dec 10, 2023
2f137b7
Update tooling/nargo/src/lib.rs
kevaundray Dec 10, 2023
41ae81f
add a method in file manager that allows us to add a file with its so…
kevaundray Dec 10, 2023
c7f576c
add deprecation TODO to file_reader
kevaundray Dec 10, 2023
a2460e6
add stdlib file in noirc_driver that returns the stdlib paths alongsi…
kevaundray Dec 10, 2023
270a0f4
add the contents of the stdlib whenever we call prepare_crate
kevaundray Dec 10, 2023
52d33a5
cargo
kevaundray Dec 10, 2023
a38c27c
cargo fmt
kevaundray Dec 10, 2023
6a5bf33
move tempfile to dev dependencies
kevaundray Dec 10, 2023
f72ce99
add files into file manager in test since find_module does not add fi…
kevaundray Dec 10, 2023
b30ad54
insert all files for this packages dependencies into the file manager…
kevaundray Dec 10, 2023
15a94ee
cargo fmt
kevaundray Dec 10, 2023
4599361
remove un-needed fully qualified path
kevaundray Dec 10, 2023
f20109e
Add note on stdlib
kevaundray Dec 10, 2023
4413350
cargo fmt
kevaundray Dec 10, 2023
d6a24e8
add comment to process_dep_graph and fix setup_test_context
kevaundray Dec 11, 2023
552a4dd
replace expect with unwrap_or_else: expect doesn't allow you to add p…
kevaundray Dec 11, 2023
3f8551b
try: add a `PathToFileSourceMap` object
kevaundray Dec 11, 2023
dd94b46
remove extraneous forward slash
kevaundray Dec 11, 2023
3495de5
add file_manager_with_source_map method so we ensure that FileManager…
kevaundray Dec 11, 2023
851fab1
add expect
kevaundray Dec 11, 2023
3cd35fd
fix node tests
kevaundray Dec 11, 2023
d2590d5
fix browser tests and naming nit
kevaundray Dec 11, 2023
5b867d5
chore!: Remove `add_file` and `file_reader` from FileManager (#3762)
kevaundray Dec 11, 2023
5ebf83a
Update compiler/fm/src/lib.rs
kevaundray Dec 11, 2023
7a787d7
Merge branch 'master' into kw/make-fm-read-only
kevaundray Dec 11, 2023
419273e
file_reader is no longer being used
kevaundray Dec 11, 2023
d120852
Merge branch 'master' into kw/make-fm-read-only
kevaundray Dec 11, 2023
1e9ac97
chore: add simple doc comments for `add_file_with_source` methods
TomAFrench Dec 13, 2023
cc92c07
Merge remote-tracking branch 'origin/master' into kw/make-fm-read-only
kevaundray Dec 13, 2023
9aa0bb7
Update tooling/nargo/src/lib.rs
kevaundray Dec 13, 2023
3e26e3c
read entry_path and get all files in its parent
kevaundray Dec 13, 2023
0b53332
clippy
kevaundray Dec 13, 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: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions compiler/fm/src/file_reader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ use std::path::Path;
// Based on the environment, we either read files using the rust standard library or we
// read files using the javascript host function

// TODO: DO NOT MERGE PR WITH THIS TODO
// TODO: We have duplicated this logic in noirc_driver. For now, we leave this here
// TODO: until we make the breaking change to the API which will allow us to remove this
// TODO: file.

pub type FileReader = dyn Fn(&Path) -> std::io::Result<String> + Send;

#[derive(RustEmbed)]
Expand Down
48 changes: 38 additions & 10 deletions compiler/fm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,22 @@ impl FileManager {
Some(file_id)
}

// TODO: this will become the default strategy for adding files. Possibly with file_reader.
// TODO: we are still migrating to this strategy, so we keep the old one for now.
// TODO: For the stdlib crate, we need to do this preemptively due to the way we special handle it
pub fn add_file_with_source(&mut self, file_name: &Path, source: String) -> Option<FileId> {
// Check that the file name already exists in the file map, if it is, we return it.
if let Some(file_id) = self.path_to_id.get(file_name) {
return Some(*file_id);
}
let file_name_path_buf = file_name.to_path_buf();

// Otherwise we add the file
let file_id = self.file_map.add_file(file_name_path_buf.clone().into(), source);
self.register_path(file_id, file_name_path_buf);
Some(file_id)
}

fn register_path(&mut self, file_id: FileId, path: PathBuf) {
let old_value = self.id_to_path.insert(file_id, path.clone());
assert!(
Expand All @@ -94,7 +110,10 @@ impl FileManager {
self.id_to_path.get(&file_id).unwrap().as_path()
}

pub fn find_module(&mut self, anchor: FileId, mod_name: &str) -> Result<FileId, String> {
// TODO: This should also ideally not be here, so that the file manager
// TODO: does not know about rust modules.
// TODO: Ideally this is moved to def_collector_mod and we make this method accept a FileManager
kevaundray marked this conversation as resolved.
Show resolved Hide resolved
pub fn find_module(&self, anchor: FileId, mod_name: &str) -> Result<FileId, String> {
let anchor_path = self.path(anchor).with_extension("");
let anchor_dir = anchor_path.parent().unwrap();

Expand All @@ -107,14 +126,19 @@ impl FileManager {
anchor_path.join(format!("{mod_name}.{FILE_EXTENSION}"))
};

self.add_file(&candidate).ok_or_else(|| candidate.as_os_str().to_string_lossy().to_string())
self.name_to_id(candidate.clone())
.ok_or_else(|| candidate.as_os_str().to_string_lossy().to_string())
}

// TODO: This should accept a &Path instead of a PathBuf
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
pub fn name_to_id(&self, file_name: PathBuf) -> Option<FileId> {
self.file_map.get_file_id(&PathString::from_path(file_name))
}
}

// TODO: This should not be here because the file manager should not know about the
// TODO: rust modules. See comment on `find_module``
// TODO: Moreover, the check for main, lib, mod should ideally not be done here
kevaundray marked this conversation as resolved.
Show resolved Hide resolved
/// Returns true if a module's child module's are expected to be in the same directory.
/// Returns false if they are expected to be in a subdirectory matching the name of the module.
fn should_check_siblings_for_module(module_path: &Path, parent_path: &Path) -> bool {
Expand Down Expand Up @@ -216,9 +240,11 @@ mod tests {
use super::*;
use tempfile::{tempdir, TempDir};

fn create_dummy_file(dir: &TempDir, file_name: &Path) {
// Returns the absolute path to the file
fn create_dummy_file(dir: &TempDir, file_name: &Path) -> PathBuf {
let file_path = dir.path().join(file_name);
let _file = std::fs::File::create(file_path).unwrap();
let _file = std::fs::File::create(&file_path).unwrap();
file_path
}

#[test]
Expand Down Expand Up @@ -257,10 +283,10 @@ mod tests {

// Create a lib.nr file at the root.
// we now have dir/lib.nr
let file_name = Path::new("lib.nr");
create_dummy_file(&dir, file_name);

let file_id = fm.add_file(file_name).unwrap();
let lib_nr_path = create_dummy_file(&dir, Path::new("lib.nr"));
let file_id = fm
.add_file(lib_nr_path.as_path())
.expect("could not add file to file manager and obtain a FileId");

// Create a sub directory
// we now have:
Expand All @@ -273,14 +299,16 @@ mod tests {
// we no have:
// - dir/lib.nr
// - dir/sub_dir/foo.nr
create_dummy_file(&sub_dir, Path::new("foo.nr"));
let foo_nr_path = create_dummy_file(&sub_dir, Path::new("foo.nr"));
fm.add_file(foo_nr_path.as_path());

// Add a parent module for the sub_dir
// we no have:
// - dir/lib.nr
// - dir/sub_dir.nr
// - dir/sub_dir/foo.nr
create_dummy_file(&dir, Path::new(&format!("{sub_dir_name}.nr")));
let sub_dir_nr_path = create_dummy_file(&dir, Path::new(&format!("{sub_dir_name}.nr")));
fm.add_file(sub_dir_nr_path.as_path());

// First check for the sub_dir.nr file and add it to the FileManager
let sub_dir_file_id = fm.find_module(file_id, sub_dir_name).unwrap();
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_driver/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ iter-extended.workspace = true
fm.workspace = true
serde.workspace = true
fxhash.workspace = true
rust-embed = "6.6.0"

aztec_macros ={path = "../../aztec_macros", optional = true}

Expand Down
18 changes: 16 additions & 2 deletions compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ mod abi_gen;
mod contract;
mod debug;
mod program;
mod stdlib;

use debug::filter_relevant_files;

Expand Down Expand Up @@ -76,9 +77,19 @@ pub type CompilationResult<T> = Result<(T, Warnings), ErrorsAndWarnings>;
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.add_file(&path_to_std_lib_file).unwrap();

// 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(Path::new(&path), source);
}

let std_crate_id = context.crate_graph.add_stdlib(std_file_id);

let root_file_id = context.file_manager.add_file(file_name).unwrap();
let root_file_id = context.file_manager.name_to_id(file_name.to_path_buf()).unwrap();

let root_crate_id = context.crate_graph.add_crate_root(root_file_id);

Expand All @@ -89,7 +100,10 @@ pub fn prepare_crate(context: &mut Context, file_name: &Path) -> CrateId {

// Adds the file from the file system at `Path` to the crate graph
pub fn prepare_dependency(context: &mut Context, file_name: &Path) -> CrateId {
let root_file_id = context.file_manager.add_file(file_name).unwrap();
let root_file_id = context
.file_manager
.name_to_id(file_name.to_path_buf())
.unwrap_or_else(|| panic!("files are expected to be added to the FileManager before reaching the compiler file_path: {file_name:?}"));

let crate_id = context.crate_graph.add_crate(root_file_id);

Expand Down
24 changes: 24 additions & 0 deletions compiler/noirc_driver/src/stdlib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
use rust_embed::RustEmbed;

#[derive(RustEmbed)]
#[folder = "../../noir_stdlib/src"]
#[cfg_attr(not(target_os = "windows"), prefix = "std/")]
kevaundray marked this conversation as resolved.
Show resolved Hide resolved
#[cfg_attr(target_os = "windows", prefix = r"std\")] // Note reversed slash direction
struct StdLibAssets;

// Returns a vector of tuples containing the path to a stdlib file in the std lib crate
// along with the source code of that file.
//
// This is needed because when we preload the file manager, it needs to know where
// the source code for the stdlib is. The stdlib is treated special because it comes with
// the compiler and is never included as a dependency like other user defined crates.
pub(crate) fn stdlib_paths_with_source() -> Vec<(String, String)> {
StdLibAssets::iter()
.map(|path| {
let source = std::str::from_utf8(StdLibAssets::get(&path).unwrap().data.as_ref())
.unwrap()
.to_string();
(path.to_string(), source)
})
.collect()
}
99 changes: 90 additions & 9 deletions compiler/wasm/src/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,31 @@ struct DependencyGraph {
library_dependencies: HashMap<CrateName, Vec<CrateName>>,
}

#[wasm_bindgen]
// This is a map containing the paths of all of the files in the entry-point crate and
// the transitive dependencies of the entry-point crate.
//
// This is for all intents and purposes the file system that the compiler will use to resolve/compile
// files in the crate being compiled and its dependencies.
#[derive(Deserialize, Default)]
pub struct PathToFileSourceMap(HashMap<std::path::PathBuf, String>);
kevaundray marked this conversation as resolved.
Show resolved Hide resolved
kevaundray marked this conversation as resolved.
Show resolved Hide resolved

#[wasm_bindgen]
impl PathToFileSourceMap {
#[wasm_bindgen(constructor)]
pub fn new() -> PathToFileSourceMap {
PathToFileSourceMap::default()
}
// Inserts a path and its source code into the map.
//
// Returns true, if there was already source code in the map for the given path
pub fn add_source_code(&mut self, path: String, source_code: String) -> bool {
let path_buf = Path::new(&path).to_path_buf();
let old_value = self.0.insert(path_buf, source_code);
old_value.is_some()
}
}

pub enum CompileResult {
Contract { contract: PreprocessedContract, debug: DebugArtifact },
Program { program: PreprocessedProgram, debug: DebugArtifact },
Expand All @@ -136,6 +161,7 @@ pub fn compile(
entry_point: String,
contracts: Option<bool>,
TomAFrench marked this conversation as resolved.
Show resolved Hide resolved
dependency_graph: Option<JsDependencyGraph>,
file_source_map: PathToFileSourceMap,
kevaundray marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<JsCompileResult, JsCompileError> {
console_error_panic_hook::set_once();

Expand All @@ -154,7 +180,7 @@ pub fn compile(
let path = Path::new(&entry_point);
let crate_id = prepare_crate(&mut context, path);

process_dependency_graph(&mut context, dependency_graph);
process_dependency_graph(&mut context, dependency_graph, file_source_map);

let compile_options = CompileOptions::default();

Expand Down Expand Up @@ -200,9 +226,24 @@ pub fn compile(
}
}

fn process_dependency_graph(context: &mut Context, dependency_graph: DependencyGraph) {
// Root dependencies are dependencies which the entry-point package relies upon.
// These will be in the Nargo.toml of the package being compiled.
//
// Library dependencies are transitive dependencies; for example, if the entry-point relies
// upon some library `lib1`. Then the packages that `lib1` depend upon will be placed in the
// `library_dependencies` list and the `lib1` will be placed in the `root_dependencies` list.
fn process_dependency_graph(
context: &mut Context,
dependency_graph: DependencyGraph,
file_source_map: PathToFileSourceMap,
) {
let mut crate_names: HashMap<&CrateName, CrateId> = HashMap::new();

// Add all files from the file_source_map into the file manager
for (path, source) in file_source_map.0 {
context.file_manager.add_file_with_source(path.as_path(), source);
}

for lib in &dependency_graph.root_dependencies {
let crate_id = add_noir_lib(context, lib);
crate_names.insert(lib, crate_id);
Expand Down Expand Up @@ -279,6 +320,14 @@ fn preprocess_contract(contract: CompiledContract) -> CompileResult {
CompileResult::Contract { contract: preprocessed_contract, debug: debug_artifact }
}

// TODO: This is no longer needed because we are passing in a map from every path
// TODO: to the source file.
// TODO: how things get resolved are now the responsibility of the caller
// TODO: We will have future PRs which make this resolution nicer by taking in a Nargo.toml
// TODO: and producing paths with source files, though for now, I think this API is okay
//
// TODO: We will also be able to remove the file_reader being a parameter to FileManager but
// TODO will stay until we have this working so we don't break the API too much.
cfg_if::cfg_if! {
if #[cfg(target_os = "wasi")] {
fn get_non_stdlib_asset(path_to_file: &Path) -> std::io::Result<String> {
Expand Down Expand Up @@ -312,6 +361,8 @@ mod test {
hir::Context,
};

use crate::compile::PathToFileSourceMap;

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

Expand All @@ -320,10 +371,10 @@ mod test {
}

fn setup_test_context() -> Context {
let fm = FileManager::new(Path::new("/"), Box::new(mock_get_non_stdlib_asset));
let mut fm = FileManager::new(Path::new("/"), Box::new(mock_get_non_stdlib_asset));
fm.add_file_with_source(Path::new("/main.nr"), "stuff".to_string());
let graph = CrateGraph::default();
let mut context = Context::new(fm, graph);

prepare_crate(&mut context, Path::new("/main.nr"));

context
Expand All @@ -339,7 +390,7 @@ mod test {
let dependency_graph =
DependencyGraph { root_dependencies: vec![], library_dependencies: HashMap::new() };

process_dependency_graph(&mut context, dependency_graph);
process_dependency_graph(&mut context, dependency_graph, PathToFileSourceMap::default());

// one stdlib + one root crate
assert_eq!(context.crate_graph.number_of_crates(), 2);
Expand All @@ -353,7 +404,13 @@ mod test {
library_dependencies: HashMap::new(),
};

process_dependency_graph(&mut context, dependency_graph);
let path_to_source_map = PathToFileSourceMap(
vec![(Path::new("/lib1/lib.nr").to_path_buf(), "fn foo() {}".to_string())]
.into_iter()
.collect(),
);

process_dependency_graph(&mut context, dependency_graph, path_to_source_map);

assert_eq!(context.crate_graph.number_of_crates(), 3);
}
Expand All @@ -366,7 +423,12 @@ mod test {
library_dependencies: HashMap::new(),
};

process_dependency_graph(&mut context, dependency_graph);
let path_to_source_map = PathToFileSourceMap(
vec![(Path::new("lib1/lib.nr").to_path_buf(), "fn foo() {}".to_string())]
.into_iter()
.collect(),
);
process_dependency_graph(&mut context, dependency_graph, path_to_source_map);

assert_eq!(context.crate_graph.number_of_crates(), 3);
}
Expand All @@ -382,7 +444,17 @@ mod test {
]),
};

process_dependency_graph(&mut context, dependency_graph);
let path_to_source_map = PathToFileSourceMap(
vec![
(Path::new("lib1/lib.nr").to_path_buf(), "fn foo() {}".to_string()),
(Path::new("lib2/lib.nr").to_path_buf(), "fn foo() {}".to_string()),
(Path::new("lib3/lib.nr").to_path_buf(), "fn foo() {}".to_string()),
]
.into_iter()
.collect(),
);

process_dependency_graph(&mut context, dependency_graph, path_to_source_map);

assert_eq!(context.crate_graph.number_of_crates(), 5);
}
Expand All @@ -395,7 +467,16 @@ mod test {
library_dependencies: HashMap::from([(crate_name("lib2"), vec![crate_name("lib3")])]),
};

process_dependency_graph(&mut context, dependency_graph);
let path_to_source_map = PathToFileSourceMap(
vec![
(Path::new("lib1/lib.nr").to_path_buf(), "fn foo() {}".to_string()),
(Path::new("lib2/lib.nr").to_path_buf(), "fn foo() {}".to_string()),
(Path::new("lib3/lib.nr").to_path_buf(), "fn foo() {}".to_string()),
]
.into_iter()
.collect(),
);
process_dependency_graph(&mut context, dependency_graph, path_to_source_map);

assert_eq!(context.crate_graph.number_of_crates(), 5);
}
Expand Down
Loading
Loading