fix: panic on assembler source manager mismatch (semver-compat)#3007
fix: panic on assembler source manager mismatch (semver-compat)#3007huitseeker wants to merge 6 commits intomainfrom
Conversation
Al-Kindi-0
left a comment
There was a problem hiding this comment.
Assuming only the semver change is different from #2987
Al-Kindi-0
left a comment
There was a problem hiding this comment.
Seems this will need a deeper look by @bitwalker
bitwalker
left a comment
There was a problem hiding this comment.
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.
| /// The source file from which this module was parsed, if any. | ||
| /// | ||
| /// Programmatically-constructed modules do not have a backing source file. | ||
| source_file: Option<Arc<SourceFile>>, |
There was a problem hiding this comment.
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
Moduledo not have to come from the same source file, and in fact that is the common case when compiling from Rust source code currently.
|
@bitwalker OK, let's continue the discussion in #2987. |
This is a version of #2987 that is semver-compatible.