Skip to content

Commit 707685c

Browse files
phatedphated
authored
chore: Rework CrateGraph to only have one root crate (#2391)
1 parent 1d3fe50 commit 707685c

File tree

5 files changed

+143
-25
lines changed

5 files changed

+143
-25
lines changed

‎crates/nargo/src/lib.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ pub mod workspace;
1717
use std::collections::BTreeMap;
1818

1919
use fm::FileManager;
20-
use noirc_driver::{add_dep, prepare_crate};
20+
use noirc_driver::{add_dep, prepare_crate, prepare_dependency};
2121
use noirc_frontend::{
2222
graph::{CrateGraph, CrateId, CrateName},
2323
hir::Context,
@@ -34,7 +34,7 @@ pub fn prepare_dependencies(
3434
for (dep_name, dep) in dependencies.iter() {
3535
match dep {
3636
Dependency::Remote { package } | Dependency::Local { package } => {
37-
let crate_id = prepare_crate(context, &package.entry_path);
37+
let crate_id = prepare_dependency(context, &package.entry_path);
3838
add_dep(context, parent_crate, crate_id, dep_name.clone());
3939
prepare_dependencies(context, crate_id, &package.dependencies);
4040
}

‎crates/noirc_driver/src/lib.rs

+8-1
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,20 @@ pub fn compile_file(
5656
compile_main(context, crate_id, &CompileOptions::default())
5757
}
5858

59-
/// Adds the file from the file system at `Path` to the crate graph
59+
/// Adds the file from the file system at `Path` to the crate graph as a root file
6060
pub fn prepare_crate(context: &mut Context, file_name: &Path) -> CrateId {
6161
let root_file_id = context.file_manager.add_file(file_name).unwrap();
6262

6363
context.crate_graph.add_crate_root(root_file_id)
6464
}
6565

66+
// Adds the file from the file system at `Path` to the crate graph
67+
pub fn prepare_dependency(context: &mut Context, file_name: &Path) -> CrateId {
68+
let root_file_id = context.file_manager.add_file(file_name).unwrap();
69+
70+
context.crate_graph.add_crate(root_file_id)
71+
}
72+
6673
/// Adds a edge in the crate graph for two crates
6774
pub fn add_dep(
6875
context: &mut Context,

‎crates/noirc_frontend/src/graph/mod.rs

+126-19
Original file line numberDiff line numberDiff line change
@@ -12,18 +12,24 @@ use smol_str::SmolStr;
1212

1313
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
1414
pub enum CrateId {
15+
Root(usize),
1516
Crate(usize),
1617
Stdlib(usize),
18+
Dummy,
1719
}
1820

1921
impl CrateId {
2022
pub fn dummy_id() -> CrateId {
21-
CrateId::Crate(std::usize::MAX)
23+
CrateId::Dummy
2224
}
2325

2426
pub fn is_stdlib(&self) -> bool {
2527
matches!(self, CrateId::Stdlib(_))
2628
}
29+
30+
pub fn is_root(&self) -> bool {
31+
matches!(self, CrateId::Root(_))
32+
}
2733
}
2834

2935
#[derive(Debug, Clone, PartialEq, Eq, Hash, Ord, PartialOrd)]
@@ -117,29 +123,73 @@ impl Dependency {
117123
}
118124

119125
impl CrateGraph {
126+
pub fn root_crate_id(&self) -> &CrateId {
127+
self.arena
128+
.keys()
129+
.find(|crate_id| crate_id.is_root())
130+
.expect("ICE: A root crate should exist in the CrateGraph")
131+
}
132+
120133
pub fn add_crate_root(&mut self, file_id: FileId) -> CrateId {
121-
let mut roots_with_file_id =
122-
self.arena.iter().filter(|(_, crate_data)| crate_data.root_file_id == file_id);
134+
for (crate_id, crate_data) in self.arena.iter() {
135+
if crate_id.is_root() {
136+
panic!("ICE: Cannot add two crate roots to a graph - use `add_crate` instead");
137+
}
123138

124-
let next_file_id = roots_with_file_id.next();
125-
if let Some(file_id) = next_file_id {
126-
return *file_id.0;
139+
if crate_data.root_file_id == file_id {
140+
panic!("ICE: This FileId was already added to the CrateGraph")
141+
}
127142
}
128143

129144
let data = CrateData { root_file_id: file_id, dependencies: Vec::new() };
130-
let crate_id = CrateId::Crate(self.arena.len());
145+
let crate_id = CrateId::Root(self.arena.len());
131146
let prev = self.arena.insert(crate_id, data);
132147
assert!(prev.is_none());
133148
crate_id
134149
}
135150

151+
pub fn add_crate(&mut self, file_id: FileId) -> CrateId {
152+
let mut crates_with_file_id = self
153+
.arena
154+
.iter()
155+
.filter(|(_, crate_data)| crate_data.root_file_id == file_id)
156+
.peekable();
157+
158+
let matching_id = crates_with_file_id.next();
159+
if crates_with_file_id.peek().is_some() {
160+
panic!("ICE: Found multiple crates with the same FileId");
161+
}
162+
163+
match matching_id {
164+
Some((crate_id @ CrateId::Crate(_), _)) => *crate_id,
165+
Some((CrateId::Root(_), _)) => {
166+
panic!("ICE: Tried to re-add the root crate as a regular crate")
167+
}
168+
Some((CrateId::Stdlib(_), _)) => {
169+
panic!("ICE: Tried to re-add the stdlib crate as a regular crate")
170+
}
171+
Some((CrateId::Dummy, _)) => {
172+
panic!("ICE: A dummy CrateId should not exist in the CrateGraph")
173+
}
174+
None => {
175+
let data = CrateData { root_file_id: file_id, dependencies: Vec::new() };
176+
let crate_id = CrateId::Crate(self.arena.len());
177+
let prev = self.arena.insert(crate_id, data);
178+
assert!(prev.is_none());
179+
crate_id
180+
}
181+
}
182+
}
183+
136184
pub fn add_stdlib(&mut self, file_id: FileId) -> CrateId {
137-
let mut roots_with_file_id =
138-
self.arena.iter().filter(|(_, crate_data)| crate_data.root_file_id == file_id);
185+
for (crate_id, crate_data) in self.arena.iter() {
186+
if crate_id.is_stdlib() {
187+
panic!("ICE: Cannot add two stdlib crates to a graph - use `add_crate` instead");
188+
}
139189

140-
let next_file_id = roots_with_file_id.next();
141-
if let Some(file_id) = next_file_id {
142-
return *file_id.0;
190+
if crate_data.root_file_id == file_id {
191+
panic!("ICE: This FileId was already added to the CrateGraph")
192+
}
143193
}
144194

145195
let data = CrateData { root_file_id: file_id, dependencies: Vec::new() };
@@ -263,8 +313,8 @@ mod tests {
263313

264314
let mut graph = CrateGraph::default();
265315
let crate1 = graph.add_crate_root(file_ids[0]);
266-
let crate2 = graph.add_crate_root(file_ids[1]);
267-
let crate3 = graph.add_crate_root(file_ids[2]);
316+
let crate2 = graph.add_crate(file_ids[1]);
317+
let crate3 = graph.add_crate(file_ids[2]);
268318

269319
assert!(graph.add_dep(crate1, "crate2".parse().unwrap(), crate2).is_ok());
270320
assert!(graph.add_dep(crate2, "crate3".parse().unwrap(), crate3).is_ok());
@@ -279,8 +329,8 @@ mod tests {
279329
let file_id_2 = file_ids[2];
280330
let mut graph = CrateGraph::default();
281331
let crate1 = graph.add_crate_root(file_id_0);
282-
let crate2 = graph.add_crate_root(file_id_1);
283-
let crate3 = graph.add_crate_root(file_id_2);
332+
let crate2 = graph.add_crate(file_id_1);
333+
let crate3 = graph.add_crate(file_id_2);
284334
assert!(graph.add_dep(crate1, "crate2".parse().unwrap(), crate2).is_ok());
285335
assert!(graph.add_dep(crate2, "crate3".parse().unwrap(), crate3).is_ok());
286336
}
@@ -292,11 +342,68 @@ mod tests {
292342
let file_id_2 = file_ids[2];
293343
let mut graph = CrateGraph::default();
294344
let _crate1 = graph.add_crate_root(file_id_0);
295-
let _crate2 = graph.add_crate_root(file_id_1);
345+
let _crate2 = graph.add_crate(file_id_1);
296346

297347
// Adding the same file, so the crate should be the same.
298-
let crate3 = graph.add_crate_root(file_id_2);
299-
let crate3_2 = graph.add_crate_root(file_id_2);
348+
let crate3 = graph.add_crate(file_id_2);
349+
let crate3_2 = graph.add_crate(file_id_2);
300350
assert_eq!(crate3, crate3_2);
301351
}
352+
353+
#[test]
354+
#[should_panic = "ICE: Cannot add two crate roots to a graph - use `add_crate` instead"]
355+
fn panics_if_adding_two_roots() {
356+
let file_ids = dummy_file_ids(2);
357+
let mut graph = CrateGraph::default();
358+
let _ = graph.add_crate_root(file_ids[0]);
359+
let _ = graph.add_crate_root(file_ids[1]);
360+
}
361+
362+
#[test]
363+
#[should_panic = "ICE: This FileId was already added to the CrateGraph"]
364+
fn panics_if_adding_existing_file_as_root() {
365+
let file_ids = dummy_file_ids(1);
366+
let mut graph = CrateGraph::default();
367+
let file_id_0 = file_ids[0];
368+
let _ = graph.add_crate(file_id_0);
369+
let _ = graph.add_crate_root(file_id_0);
370+
}
371+
372+
#[test]
373+
#[should_panic = "ICE: Cannot add two stdlib crates to a graph - use `add_crate` instead"]
374+
fn panics_if_adding_two_stdlib() {
375+
let file_ids = dummy_file_ids(2);
376+
let mut graph = CrateGraph::default();
377+
let _ = graph.add_stdlib(file_ids[0]);
378+
let _ = graph.add_stdlib(file_ids[1]);
379+
}
380+
381+
#[test]
382+
#[should_panic = "ICE: This FileId was already added to the CrateGraph"]
383+
fn panics_if_adding_existing_file_as_stdlib() {
384+
let file_ids = dummy_file_ids(1);
385+
let mut graph = CrateGraph::default();
386+
let file_id_0 = file_ids[0];
387+
let _ = graph.add_crate(file_id_0);
388+
let _ = graph.add_stdlib(file_id_0);
389+
}
390+
391+
#[test]
392+
#[should_panic = "ICE: Tried to re-add the root crate as a regular crate"]
393+
fn panics_if_adding_root_as_regular() {
394+
let file_ids = dummy_file_ids(1);
395+
let mut graph = CrateGraph::default();
396+
let file_id_0 = file_ids[0];
397+
let _ = graph.add_crate_root(file_id_0);
398+
let _ = graph.add_crate(file_id_0);
399+
}
400+
#[test]
401+
#[should_panic = "ICE: Tried to re-add the stdlib crate as a regular crate"]
402+
fn panics_if_adding_stdlib_as_regular() {
403+
let file_ids = dummy_file_ids(1);
404+
let mut graph = CrateGraph::default();
405+
let file_id_0 = file_ids[0];
406+
let _ = graph.add_stdlib(file_id_0);
407+
let _ = graph.add_crate(file_id_0);
408+
}
302409
}

‎crates/noirc_frontend/src/hir/mod.rs

+4
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,10 @@ impl Context {
5959
self.crate_graph.iter_keys()
6060
}
6161

62+
pub fn root_crate_id(&self) -> &CrateId {
63+
self.crate_graph.root_crate_id()
64+
}
65+
6266
// TODO: Decide if we actually need `function_name` and `fully_qualified_function_name`
6367
pub fn function_name(&self, id: &FuncId) -> &str {
6468
self.def_interner.function_name(id)

‎crates/wasm/src/compile.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@ use fm::FileManager;
33
use gloo_utils::format::JsValueSerdeExt;
44
use log::debug;
55
use noirc_driver::{
6-
check_crate, compile_contracts, compile_no_check, prepare_crate, propagate_dep, CompileOptions,
7-
CompiledContract,
6+
check_crate, compile_contracts, compile_no_check, prepare_crate, prepare_dependency,
7+
propagate_dep, CompileOptions, CompiledContract,
88
};
99
use noirc_frontend::{graph::CrateGraph, hir::Context};
1010
use serde::{Deserialize, Serialize};
@@ -60,7 +60,7 @@ impl Default for WASMCompileOptions {
6060

6161
fn add_noir_lib(context: &mut Context, crate_name: &str) {
6262
let path_to_lib = Path::new(&crate_name).join("lib.nr");
63-
let library_crate = prepare_crate(context, &path_to_lib);
63+
let library_crate = prepare_dependency(context, &path_to_lib);
6464

6565
propagate_dep(context, library_crate, &crate_name.parse().unwrap());
6666
}

0 commit comments

Comments
 (0)