fix: panic on assembler source manager mismatch by juan518munoz · Pull Request #2987 · 0xMiden/miden-vm

juan518munoz · 2026-04-09T18:49:08Z

When a pre-parsed module is assembled using a different source manager than the one it was parsed with, the assembler used to panic during debug info registration because span byte offsets didn't match the resolved source file.

This PR replaces the panic with proper error handling: missing source ids are skipped silently, while out-of-bounds spans are surfaced as a compilation error with a descriptive message pointing to the source manager mismatch.

Closes #2986

github-actions · 2026-04-09T18:53:28Z

Automated check (CONTRIBUTING.md)

Recommendations:

Consider adding a Test plan or clear review steps.

Next steps:

Ensure issue Assembler::assemble_library panics when given a pre-parsed module from a different source manager #2986 is assigned to you.
See CONTRIBUTING.md for expectations.
If this is a false positive, comment: /quality-review.

huitseeker · 2026-04-10T07:57:56Z

+        // The source id exists, so file_line_col should succeed. If it doesn't, the
+        // span's byte offsets are out of bounds for the file at that source id, which
+        // indicates the module was parsed with a different source manager.
+        let file_line_col = source_manager.file_line_col(span).map_err(|err| {


Could this still attach debug info to the wrong file when the assembler's manager has the same SourceId and that file is long enough? SourceId is only local to one manager, so get plus file_line_col proves the offset fits some file in this manager, but not that the span came from it. In that case the mismatch would still slip through as wrong debug metadata.

Yes, you are right, handled in 1480dee

huitseeker

Please fix the false-negative.

juan518munoz · 2026-04-10T18:41:16Z

@huitseeker

Fixing the false negative required a broader change. As SourceId is just a u32 index with no cross-manager uniqueness, so bounds checks alone can't distinguish "right file at index N" from "wrong file at same index."

The proposed fix in 1480dee uses is_manager_of, which compares Arc<SourceFile> pointer identity, making false negatives impossible. To make the original SourceFile available at debug info registration time, we have to thread it through. now Module stores the Arc<SourceFile>, then LinkModule carries it through linking, and register_procedure_debug_info uses it for the is_manager_of check.

huitseeker · 2026-04-13T08:37:08Z

    /// An alias for creating the default, but empty, `#kernel` [Module].
    pub fn new_kernel() -> Self {
-        Self::new(ModuleKind::Kernel, Path::kernel_path())
+        Self::new(ModuleKind::Kernel, Path::kernel_path(), Self::empty_source_file())


I think this regresses in-memory AST modules.

new_kernel() and new_executable() now attach a synthetic SourceFile, but later register_procedure_debug_info() treats any file not owned by the assembler's SourceManager as an error.

I reproduced this with a programmatically built module containing one nop procedure: Assembler::new(sm).assemble_library([module]) now returns source manager mismatch instead of just skipping debug info.

Could we represent 'no backing source file' as None here?

Forgot to reply here as well, but it's also been handled as of 713e931

huitseeker · 2026-04-13T08:37:16Z

+                    self.debug_info.register_procedure_debug_info(
+                        &procedure,
+                        self.source_manager.as_ref(),
+                        module_source_file.as_deref(),


The source-manager check lands too late here.

compile_procedure() already uses self.source_manager for instruction spans and diagnostics before we reach register_procedure_debug_info(), so a mismatched module can still take the wrong source path first.

I reproduced that with a module parsed in one manager and assembled in another using loc_loadw_be.2: formatting the compile error panicked in SourceFile::location. We should reject the module before lowering starts.

Moved the source-manager check to linking time so it covers this.

713e931

juan518munoz · 2026-04-13T15:26:06Z

Thanks for the re-review @huitseeker, PR is ready to be reviewed again.

I was wondering if it would be preferable to point this to either main or other release branch.

huitseeker

LGTM, thanks for fixing this!

juan518munoz · 2026-04-13T18:33:49Z

Thanks for the review, I've rebased into main so this can be later patch-released @huitseeker

EDIT: As a second though, this has breaking changes (public API change), so it may be better suited for next.

SantiagoPittella · 2026-04-13T18:38:07Z

If this is the only commit that's going to a new patch version, and a maintainer of this repo agrees, maybe you can bump the crate version here too.

huitseeker

Reflecting @bitwalker 's review on #3007:

This PR makes some assumptions about how source files are related to modules which are incorrect, so I don't think it makes sense to merge it (or #2987).

I'm not sure this is even the correct way to handle this in general - if you've somehow gotten your source managers mixed up, you want to know exactly where that happened, and a panic is useful for that. If you instead silently drop the source locations, or try to bubble up an Err, you'll lose valuable context about where the wires got crossed.

IMO, this is really a documentation issue related to how SourceManager impls need to be used, both from the caller perspective, and in terms of threading them through internals of the VM or other components.

huitseeker · 2026-04-14T14:25:58Z

+    /// The source file from which this module was parsed, if any.
+    ///
+    /// This is `None` for programmatically-constructed modules (e.g. `new_kernel()`,
+    /// `new_executable()`) that have no backing source text.


Reflecting @bitwalker 's review on #3007 :

There are two issues with this:

Programmatically-constructed modules can absolutely have a backing source file - it just means that the source language was not Miden Assembly (or that the AST was literally constructed by hand for a test, but that's not the important case).

Source spans within a Module do not have to come from the same source file, and in fact that is the common case when compiling from Rust source code currently.

huitseeker

As above and as reviewed by @bitwalker in #3007.

juan518munoz changed the title ~~fix: panic on sm mismatch~~ fix: panic on assembler source manager mismatch Apr 9, 2026

juan518munoz marked this pull request as ready for review April 9, 2026 18:53

github-actions Bot added the quality-concern label Apr 9, 2026

huitseeker requested changes Apr 10, 2026

View reviewed changes

juan518munoz requested a review from huitseeker April 10, 2026 18:41

huitseeker requested changes Apr 13, 2026

View reviewed changes

github-actions Bot removed the quality-concern label Apr 13, 2026

juan518munoz requested a review from huitseeker April 13, 2026 15:24

huitseeker approved these changes Apr 13, 2026

View reviewed changes

juan518munoz added 4 commits April 13, 2026 15:31

fix: panic on sm mismatch

980b4bf

fix: false negative scenario

0bd7168

docs: add changelog entry

75c9b7c

chore: adress PR comments

e537d12

juan518munoz force-pushed the jmunoz-detect-assembler-source-manager-mismatch branch from 2528505 to e537d12 Compare April 13, 2026 18:31

juan518munoz changed the base branch from next to main April 13, 2026 18:31

chore: fix changelog

31714a0

huitseeker mentioned this pull request Apr 14, 2026

fix: panic on assembler source manager mismatch (semver-compat) #3007

Closed

huitseeker reviewed Apr 14, 2026

View reviewed changes

huitseeker requested changes Apr 14, 2026

View reviewed changes

huitseeker mentioned this pull request Apr 14, 2026

Assembling sometimes panics with "invalid source span: starting byte is out of bounds" (wrong procedure name, missing imports) #2778

Open

juan518munoz mentioned this pull request Apr 14, 2026

Assembler::assemble_library panics when given a pre-parsed module from a different source manager #2986

Open

Conversation

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

Uh oh!

Uh oh!

github-actions Bot commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huitseeker Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

juan518munoz Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

juan518munoz commented Apr 10, 2026

Uh oh!

huitseeker Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

juan518munoz Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

huitseeker Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

juan518munoz Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

juan518munoz commented Apr 13, 2026

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

SantiagoPittella commented Apr 13, 2026

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

huitseeker Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

huitseeker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

juan518munoz commented Apr 9, 2026 •

edited

Loading

github-actions Bot commented Apr 9, 2026 •

edited

Loading

juan518munoz commented Apr 13, 2026 •

edited

Loading