Shift Module Resolution to AST and Introduce Scope Analysis by LesterEvSe · Pull Request #345 · BlockstreamResearch/SimplicityHL

LesterEvSe · 2026-06-02T14:53:13Z

Motivation

This PR is deliberately monolithic because the driver and AST resolution phases are tightly coupled. Updating driver/resolve_order.rs without ast.rs (or vice versa) breaks the test suite across both files, as well as lib.rs. The new driver outputs multi-file programs that the old AST cannot process, requiring them to be implemented and tested together.

Changes

driver/resolve_order.rs: Stripped out alias and visibility resolution. The driver now strictly focuses on resolving multiple files and packing them into Item::Module blocks.
ast.rs: Implemented native scopes, alias resolution, and visibility checks. The analyze stage can now properly process Item::Use and Item::Module blocks.
Testing: Added full test coverage for the new scopes and driver logic, logically separated into 3 commits (driver/resolve_order.rs, ast.rs, and global lib.rs).

KyrylR · 2026-06-03T08:07:01Z

        let source = SourceFile::anonymous(file.clone());
        let mut error_handler = ErrorCollector::new();
        let parse_program = parse::Program::parse_from_str_with_errors(source, &mut error_handler);
+        dbg!(&file);


Oh, I forgot about that. I delete it during the refactoring

KyrylR · 2026-06-03T08:15:00Z

@@ -13,36 +13,30 @@
 //! to build a Directed Acyclic Graph (DAG) of the project's dependencies. Because
 //! the final AST requires a flat array of items, the driver applies a deterministic


because import cache is keyed only by UseDecl, identical use crate::... in different package roots can rewrite to the wrong file

Fixed and added more test cases

LesterEvSe · 2026-06-03T15:50:36Z

@apoelstra, I’d like to get your thoughts on our commit and PR structure. I'm trying my best to follow the CONTRIBUTING.md rules for atomic PRs, but for massive, tightly coupled features like the current Modules (or recent imports in PR #264), it’s becoming a huge bottleneck.

My current workflow is: brainstorm the architecture -> implement the entire feature on a single branch -> painstakingly divide it into separate PRs. I have to do it this way because I frequently discover bugs in my earlier assumptions only after I've written the later commits.

However, this means if a reviewer catches an issue in PR 3 that requires a change in the code from PR 1, keeping the commit history clean and linear becomes incredibly difficult. I feel like I am spending 50% of my time just organizing Git history instead of building the new features.

Is there a better or accepted way to handle these large, monolithic architectural changes? Should we be relaxing the commit rules for these specific cases? Let me know what you think!

KyrylR · 2026-06-04T11:53:22Z

+            dbg!(curr_id);
+            dbg!(&queue);


KyrylR · 2026-06-04T11:54:57Z

                Self::parse_and_get_source_module(&path, current.source, import_span, ctx.handler)
            else {
-                ctx.invalid_imports.push(path);
+                ctx.invalid_imports.insert(path);


Suggested change

ctx.invalid_imports.insert(path);

let _ = ctx.invalid_imports.insert(path);

Why it is ok to ignore?

It checked before in .contains method. Added comment for this

KyrylR · 2026-06-04T11:57:17Z

 /// through every recursive call. Lives only for the duration of [`resolve_imports`].
 struct ImportContext<'a> {
-    importer_source: &'a CanonSourceFile,
+    current: &'a CurrentModule<'a>,


Is this lifetime really needed?

No, deleted it

KyrylR · 2026-06-04T12:00:19Z

+        // might resolve to completely different implementations of the `foo` function.
+        let mut use_decl = use_decl.clone();
+        use_decl.set_file_id(self.current.id);
+


nit: you can create span here to prevent second clone

KyrylR · 2026-06-04T12:01:17Z

+        let mut use_decl = use_decl.clone();
+        use_decl.set_file_id(self.current.id);
+
+        let result = (resolved.path.clone(), *use_decl.span());


Suggested change

let result = (resolved.path.clone(), *use_decl.span());

let result: <add explicit type> = (resolved.path.clone(), *use_decl.span());

KyrylR · 2026-06-04T12:05:10Z

-                    parse::Item::Module(_) | parse::Item::Use(_) | parse::Item::Ignored => continue,
-                }
-                items.push(new_elem);
+            /*


I have already deleted this and the other unnecessary comments

KyrylR · 2026-06-04T12:14:02Z


-    let name = item.name();
-    let local_id = (name.clone(), source_id);
+        //if target_id != MAIN_MODULE {


KyrylR · 2026-06-04T12:20:03Z

+    /// * May also return errors propagated from item collection and insertion, such as [`Error::PrivateItem`] or [`Error::RedefinedItem`].
+    pub fn resolve_use(&mut self, use_decl: &UseDecl) -> Result<(), Error> {
+        let path = use_decl.path();
+        if path[0].as_inner() != CRATE_STR {


panic check?

KyrylR · 2026-06-04T12:21:22Z

+        };
+
+        // Phase 1: navigate to target and collect items. Immutable borrow, dropped at end of block
+        let collected: Vec<_> = {


Let's expand the type

KyrylR · 2026-06-04T12:22:19Z

+    ///
+    /// * Returns a specific error (e.g., [`Error::PrivateItem`], [`Error::RedefinedItem`]) if one occurred.
+    /// * Returns a fallback [`Error::UnresolvedItem`] if the item could not be found in any of the checked namespaces.
+    fn resolve_processing_use_items_error(results: &[Result<(), Error>]) -> Result<(), Error> {


Let's have GH issue

KyrylR · 2026-06-04T12:23:04Z

+        /*
+        if !scope.module_path.is_empty() {
            return Err(Error::MainOutOfEntryFile).with_span(from);
        }
+        */
+


KyrylR · 2026-06-04T12:24:13Z

+    /// THE DEFAULT HELPER
+    pub(crate) fn flatten_dependency_test(root_path: &str, lib_alias: &str) {
+        let root_path = PathBuf::from(root_path);
+        let lib_path = root_path.join(lib_alias);
+        let main_path = root_path.join(MAIN);
+        let flattened_path = root_path.join(FLATTENED);
+
+        let dependency_map = build_dependency_map(&main_path, [(&root_path, lib_alias, &lib_path)]);
+
+        let expected = format_program_file(&flattened_path);
+        let actual = flatten_template_file(&main_path, &dependency_map);
+
+        println!("Expected:\n{}", expected);
+        println!("----------");
+        println!("Actual:\n{}", actual);
+
+        assert_eq!(expected, actual);
+    }


Why do we need printlns?

No need at all

KyrylR · 2026-06-04T12:24:21Z

            .assert_run_success();
    }

+    /// THE ADVANCED HELPER


Suggested change

/// THE ADVANCED HELPER

KyrylR · 2026-06-04T12:24:43Z

            .assert_run_success();
    }

+    /// THE DEFAULT HELPER


Suggested change

/// THE DEFAULT HELPER

KyrylR · 2026-06-04T12:25:47Z

 }

-impl_eq_hash!(UseDecl; visibility, path, items);
+impl_eq_hash!(UseDecl; file_id, visibility, path, items, span);


explain why those two new args are needed

KyrylR

ACK 03e782d; successfully ran local tests

LesterEvSe requested a review from KyrylR June 2, 2026 14:53

LesterEvSe self-assigned this Jun 2, 2026

LesterEvSe requested a review from delta1 as a code owner June 2, 2026 14:53

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

LesterEvSe added 3 commits June 2, 2026 17:55

update: driver resolution strategy, ast analyze. Add scopes

49414bb

feat: add new tests for driver/resolve_order.rs file

5e34ddc

feat: add module, alias and scope tests for ast.rs

fcb6004

LesterEvSe force-pushed the update/driver-resolution-strategy branch from 3386db6 to 415a69d Compare June 2, 2026 14:55

KyrylR reviewed Jun 3, 2026

View reviewed changes

LesterEvSe force-pushed the update/driver-resolution-strategy branch 3 times, most recently from e81b73e to ce3f943 Compare June 3, 2026 15:25

KyrylR reviewed Jun 4, 2026

View reviewed changes

LesterEvSe force-pushed the update/driver-resolution-strategy branch from ce3f943 to 693f2a2 Compare June 4, 2026 13:53

LesterEvSe force-pushed the update/driver-resolution-strategy branch 2 times, most recently from b619e5c to ec6cbb7 Compare June 5, 2026 09:43

LesterEvSe added 2 commits June 5, 2026 15:34

feat: add examples and flattening tests. Fix bugs in driver

d465df1

docs: clarify imports and flattening have no effect on CMR

03e782d

LesterEvSe force-pushed the update/driver-resolution-strategy branch from ec6cbb7 to 03e782d Compare June 5, 2026 12:35

KyrylR approved these changes Jun 5, 2026

View reviewed changes

KyrylR merged commit 03e782d into BlockstreamResearch:dev/modules Jun 5, 2026
11 checks passed

LesterEvSe mentioned this pull request Jun 5, 2026

Implement module system and program flattening #337

Open

2 tasks

		@@ -13,36 +13,30 @@
		//! to build a Directed Acyclic Graph (DAG) of the project's dependencies. Because
		//! the final AST requires a flat array of items, the driver applies a deterministic

	ctx.invalid_imports.insert(path);
	let _ = ctx.invalid_imports.insert(path);

	let result = (resolved.path.clone(), *use_decl.span());
	let result: <add explicit type> = (resolved.path.clone(), *use_decl.span());

Conversation

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

Uh oh!

Motivation

Changes

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

LesterEvSe Jun 5, 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!

Choose a reason for hiding this comment

Uh oh!

KyrylR Jun 4, 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!

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!

2 participants

LesterEvSe commented Jun 2, 2026 •

edited

Loading

LesterEvSe Jun 5, 2026 •

edited

Loading

KyrylR Jun 4, 2026 •

edited

Loading