Implement `DependencyGraph` and foundational driver architecture by LesterEvSe · Pull Request #271 · BlockstreamResearch/SimplicityHL

LesterEvSe · 2026-04-03T13:48:38Z

Dependency Graph: Added driver.rs featuring the DependencyGraph implementation to build a Directed Acyclic Graph of project dependencies.
Trait Refactoring: Updated the ParseFromStrWithErrors trait to accept a full SourceFile (containing both metadata and source text) instead of just the raw string content.

metalurgical · 2026-04-03T22:42:14Z

src/driver.rs

+pub struct ProjectGraph {
+    /// Arena Pattern: the data itself lives here.
+    /// A flat vector guarantees that module data is stored contiguously in memory.
+    #[expect(dead_code)]


Suggested change

#[expect(dead_code)]

#[allow(dead_code)]

KyrylR · 2026-04-06T11:35:41Z

src/driver/mod.rs

@@ -0,0 +1,327 @@
+use std::collections::{HashMap, VecDeque};


Could we move it to driver/mod.rs?
The reasoning is that we gonna introduce a few independent algorithms here, so we could have them under same module, as they are just related to the driver itself

KyrylR · 2026-04-06T11:36:37Z

src/driver.rs

+    ///
+    /// This function will return an `Err(String)` only for critical internal compiler errors
+    /// (e.g., if a provided `SourceFile` is unexpectedly missing its underlying file path).
+    pub fn new(


Let's have pub functions first then private

KyrylR · 2026-04-06T11:41:38Z

src/driver.rs

+
+        while let Some(curr_id) = queue.pop_front() {
+            // We need this to report errors inside THIS file.
+            let importer_source = graph.modules[curr_id].source.clone();


Let's handle modules[curr_id], via let statement to avoid panic

KyrylR · 2026-04-06T13:39:49Z

src/driver/mod.rs

+            );
+        }
+
+        Ok((!handler.has_errors()).then_some(graph))


Is it expected that ParseFromStrWithErrors now returns None whenever the shared ErrorCollector already contains any earlier error, and DependencyGraph::new reuses one collector across the entire BFS?

This was not expected. I could add a local ErrorCollector inside the parse_and_get_program function to parse more files and collect more errors. I think this would be better than the current approach

Yes, then you could return Result<Self, ErrorCollector>

That won't work with the collector because the Strings are not bound to the span elements that the collector requires. I took a slightly different approach and created the collector in the parse_and_get_program function to collect more errors.

I think it will be fixed once the #270 issue has been resolved.

Add a TODO here, and mention in the comment the issue

KyrylR · 2026-04-06T13:42:21Z

src/driver/mod.rs

+            if let Some(&existing_id) = self.lookup.get(&path) {
+                let deps = self.dependencies.entry(curr_id).or_default();
+                if !deps.contains(&existing_id) {
+                    deps.push(existing_id);
+                }


Does it actually detect import cycles? As it is written it treats any already-seen path as a normal resolved dependency and continues. For A -> B -> A, B’s import of A hits the existing_id branch, and in Ok((!handler.has_errors()).then_some(graph)) still returns Some(graph) with no diagnostic

No, it will be the responsibility of C3 and the other algorithms in the following PR

KyrylR · 2026-04-07T12:39:27Z

src/driver/mod.rs

+            let Some(module) =
+                Self::parse_and_get_program(&path, importer_source.clone(), import_span, handler)
+            else {
+                continue;
+            };


If parse_and_get_program fails here, the path is never recorded in lookup, so another parent importing the same file will try to parse it again and emit the same lex or syntax errors again

We could try to fix it here, or have a GitHub issue for it

Tried to resolve it here

KyrylR

ACK ae3ed68; code review, comments

apoelstra · 2026-04-07T14:10:53Z

ae3ed68 needs rebase

KyrylR · 2026-04-07T14:15:51Z

ae3ed68 needs rebase

Resolved locally

LesterEvSe · 2026-04-07T15:38:30Z

I have reopened the PR for fixes after all. #278

LesterEvSe requested a review from KyrylR April 3, 2026 13:48

LesterEvSe self-assigned this Apr 3, 2026

LesterEvSe requested a review from delta1 as a code owner April 3, 2026 13:48

LesterEvSe added the enhancement New feature or request label Apr 3, 2026

metalurgical reviewed Apr 3, 2026

View reviewed changes

LesterEvSe force-pushed the feature/simple-driver branch from 874e4a5 to 77e449a Compare April 6, 2026 09:27

LesterEvSe changed the title ~~Implement ProjectGraph and foundational driver architecture~~ Implement DependencyGraph and foundational driver architecture Apr 6, 2026

KyrylR reviewed Apr 6, 2026

View reviewed changes

LesterEvSe force-pushed the feature/simple-driver branch from 77e449a to 006099a Compare April 6, 2026 13:23

KyrylR reviewed Apr 6, 2026

View reviewed changes

LesterEvSe force-pushed the feature/simple-driver branch from 006099a to 571453c Compare April 6, 2026 14:56

LesterEvSe requested a review from KyrylR April 6, 2026 16:14

KyrylR reviewed Apr 7, 2026

View reviewed changes

LesterEvSe force-pushed the feature/simple-driver branch from 571453c to f04cf55 Compare April 7, 2026 13:34

LesterEvSe requested a review from KyrylR April 7, 2026 13:40

LesterEvSe force-pushed the feature/simple-driver branch from f04cf55 to ae3ed68 Compare April 7, 2026 13:53

KyrylR approved these changes Apr 7, 2026

View reviewed changes

KyrylR merged commit ae3ed68 into BlockstreamResearch:dev/imports Apr 7, 2026
11 checks passed

KyrylR mentioned this pull request Apr 7, 2026

Add imports functionality #264

Draft

6 tasks

LesterEvSe mentioned this pull request Apr 7, 2026

[Reopen] Implement DependencyGraph and foundational driver architecture #278

Open

Conversation

LesterEvSe commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LesterEvSe Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KyrylR left a comment

Choose a reason for hiding this comment

Uh oh!

apoelstra commented Apr 7, 2026

Uh oh!

Uh oh!

KyrylR commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LesterEvSe commented Apr 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

LesterEvSe commented Apr 3, 2026 •

edited

Loading

LesterEvSe Apr 6, 2026 •

edited

Loading

KyrylR commented Apr 7, 2026 •

edited

Loading