Introduce `DependencyMap` for future import routing by LesterEvSe · Pull Request #269 · BlockstreamResearch/SimplicityHL

LesterEvSe · 2026-04-02T09:46:04Z

Adds the DependencyMap and Remapping data structures.
Implements longest-prefix matching to correctly resolve paths.

KyrylR · 2026-04-02T10:30:19Z

src/resolution.rs

+    pub fn test_insert_without_canonicalize(
+        &mut self,
+        context: &Path,
+        dependency_root_path: String,
+        path: &Path,
+    ) {
+        self.inner.push(Remapping {
+            context_prefix: context.to_path_buf(),
+            dependency_root_path,
+            target: path.to_path_buf(),
+        });
+        self.sort_mappings();
+    }


Should go to the test mod

KyrylR · 2026-04-02T10:30:32Z

src/resolution.rs

+        // No matches found
+        Err(Error::UnknownLibrary(first_segment.to_string())).with_span(*use_decl.span())
+    }
+}


Let's have at least a few unit tests

KyrylR · 2026-04-02T11:34:19Z

src/resolution.rs

+        // If your macro generates a different constructor, just swap `From::from`
+        // with `Identifier::new(s)` or `Identifier(Arc::from(s))`!


Looks like AI generated comment, let's not include those

KyrylR · 2026-04-02T11:35:02Z

src/resolution.rs

+    // --- TEST HELPERS ---
+


Suggested change

// --- TEST HELPERS ---

KyrylR · 2026-04-02T11:36:19Z

src/resolution.rs

+    /// The base directory that owns this dependency mapping.
+    pub context_prefix: PathBuf,
+    /// The name used in the `use` statement (e.g., "math").
+    pub dependency_root_path: String,


Suggested change

pub dependency_root_path: String,

pub drp_name: String,

KyrylR · 2026-04-02T11:37:34Z

src/resolution.rs

+    // --- TESTS ---
+


Suggested change

// --- TESTS ---

KyrylR · 2026-04-03T07:49:19Z

src/resolution.rs

+#[derive(Debug, Clone)]
+pub struct Remapping {
+    /// The base directory that owns this dependency mapping.
+    pub context_prefix: PathBuf,


Suggested change

pub context_prefix: PathBuf,

pub context_prefix: CanonPath,

KyrylR · 2026-04-03T07:51:36Z

src/resolution.rs

+    pub fn insert(
+        &mut self,
+        context: CanonPath,
+        dependency_root_path: String,


Suggested change

dependency_root_path: String,

drp_name: String,

KyrylR · 2026-04-03T07:51:56Z

src/resolution.rs

+        Ok(())
+    }
+
+    /// Resolve `use dependency_root_path::...` into a physical file path by finding the


Suggested change

/// Resolve `use dependency_root_path::...` into a physical file path by finding the

/// Resolve `use dependency_root_path_name::...` into a physical file path by finding the

KyrylR · 2026-04-03T11:28:43Z

src/parse.rs


+    impl UseDecl {
+        /// Creates a dummy `UseDecl` specifically for testing `DependencyMap` resolution.
+        /// It automatically builds the internal Identifier path.


Suggested change

/// It automatically builds the internal Identifier path.

KyrylR · 2026-04-03T11:31:04Z

src/resolution.rs

+    /// Returns a `String` containing the OS error if the path does not exist or
+    /// cannot be accessed. The caller is expected to map this into a more specific
+    /// compiler diagnostic (e.g., `RichError`).


Not to block this PR, but the String as an error is too generic, so we will need a follow up PR (after modules are merged into master that would take care of it)

@LesterEvSe could you create a GitHub issue, and link this comment?

KyrylR · 2026-04-03T11:31:24Z

src/resolution.rs

+        // We use `map_err` here to intercept the generic OS error and enrich
+        // it with the specific path that failed


Suggested change

// We use `map_err` here to intercept the generic OS error and enrich

// it with the specific path that failed

Outdated?

No, I think it's still necessary

KyrylR · 2026-04-03T11:34:12Z

src/resolution.rs

+            // Check if the current file is executing inside the context's directory tree.
+            // This prevents a file in `/project_a/` from using a dependency meant for `/project_b/`
+            if !current_file
+                .as_path()
+                .starts_with(remapping.context_prefix.as_path())
+            {
+                continue;
+            }


Could we create a method inside the CanonPath and add rust doc there?

KyrylR · 2026-04-03T11:35:54Z

src/resolution.rs

+        // Safely extract the first segment (the dependency root path)
+        let parts: Vec<&str> = use_decl.path().iter().map(|s| s.as_inner()).collect();
+        let first_segment = parts.first().copied().ok_or_else(|| {
+            Error::CannotParse("Empty use path".to_string()).with_span(*use_decl.span())
+        })?;


Move this to the method of UseDecl

KyrylR · 2026-04-03T11:36:26Z

src/resolution.rs

+            }
+        }
+
+        // No matches found


Suggested change

// No matches found

KyrylR

ACK f609987

LesterEvSe requested a review from KyrylR April 2, 2026 09:46

LesterEvSe self-assigned this Apr 2, 2026

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

LesterEvSe requested a review from delta1 as a code owner April 2, 2026 09:46

LesterEvSe mentioned this pull request Apr 2, 2026

Add DependencyMap data structure for imports #268

Merged

KyrylR requested changes Apr 2, 2026

View reviewed changes

LesterEvSe force-pushed the feature/dependency-data-structure branch from 72a8241 to e21af35 Compare April 2, 2026 11:29

LesterEvSe requested a review from KyrylR April 2, 2026 11:29

LesterEvSe force-pushed the feature/dependency-data-structure branch from e21af35 to 3355d31 Compare April 2, 2026 11:31

KyrylR requested changes Apr 2, 2026

View reviewed changes

LesterEvSe force-pushed the feature/dependency-data-structure branch 2 times, most recently from b455b6f to 4199eed Compare April 2, 2026 13:45

LesterEvSe requested a review from KyrylR April 2, 2026 13:45

KyrylR reviewed Apr 3, 2026

View reviewed changes

LesterEvSe force-pushed the feature/dependency-data-structure branch from 4199eed to 683082b Compare April 3, 2026 11:22

LesterEvSe requested a review from KyrylR April 3, 2026 11:22

LesterEvSe force-pushed the feature/dependency-data-structure branch 2 times, most recently from 37f7b01 to 3695b8a Compare April 3, 2026 11:27

KyrylR reviewed Apr 3, 2026

View reviewed changes

src/resolution.rs Outdated

}

}

// No matches found

Copy link
Copy Markdown

Collaborator

KyrylR Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

// No matches found

LesterEvSe mentioned this pull request Apr 3, 2026

Refactor(ErrorHandler): Consolidate generic String errors into structured type #270

Open

LesterEvSe force-pushed the feature/dependency-data-structure branch from 3695b8a to fb99a23 Compare April 3, 2026 12:19

LesterEvSe requested a review from KyrylR April 3, 2026 12:19

feat: add data structure for dependency resolution

f609987

LesterEvSe force-pushed the feature/dependency-data-structure branch from fb99a23 to f609987 Compare April 3, 2026 12:24

KyrylR approved these changes Apr 3, 2026

View reviewed changes

KyrylR merged commit f609987 into BlockstreamResearch:dev/imports Apr 3, 2026
11 checks passed

KyrylR mentioned this pull request Apr 3, 2026

Add imports functionality #264

Draft

6 tasks

LesterEvSe deleted the feature/dependency-data-structure branch April 3, 2026 12:33

Ari4ka approved these changes Apr 3, 2026

View reviewed changes

		// If your macro generates a different constructor, just swap `From::from`
		// with `Identifier::new(s)` or `Identifier(Arc::from(s))`!

	/// Resolve `use dependency_root_path::...` into a physical file path by finding the
	/// Resolve `use dependency_root_path_name::...` into a physical file path by finding the

		// We use `map_err` here to intercept the generic OS error and enrich
		// it with the specific path that failed

Conversation

LesterEvSe commented Apr 2, 2026

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!

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 3, 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!

Choose a reason for hiding this comment

Uh oh!

KyrylR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LesterEvSe Apr 3, 2026 •

edited

Loading