Skip to content

chore: Add Send + Sync supertraits to SourceManager trait #1858

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sergerad wants to merge 8 commits into from
Closed

chore: Add Send + Sync supertraits to SourceManager trait #1858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ea4c6c4
Add Send + Sync supertraits to SourceManager trait
sergerad Jun 5, 2025
2f3cdd5
Update changelog
sergerad Jun 5, 2025
a3f1df4
Revert "Add Send + Sync supertraits to SourceManager trait"
sergerad Jun 5, 2025
fe96f77
SourceManagerSync
sergerad Jun 5, 2025
39f9a50
Fixes
sergerad Jun 5, 2025
5f87e70
Mv sync
sergerad Jun 5, 2025
6df3b98
Update changelog
sergerad Jun 5, 2025
6accc44
Lint
sergerad Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
- Improve error messages for some assembler instruction (#1785)
- Remove `idx` column from Kernel ROM chiplet and use chiplet bus for initialization. (#1818)
- [BREAKING] Make `Assembler::source_manager()` be `Send + Sync` (#1822)
- Add `SourceManagerSync` trait and generic implementation. (#1858)
- Simplify and optimize the recursive verifier (#1801).
- Simplify auxiliary randomness generation (#1810).
- Add handling of variable length public inputs to the recursive verifier (#1813).
Expand Down
11 changes: 6 additions & 5 deletions assembly/src/assembler/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ use vm_core::{
};

use crate::{
AssemblyError, Compile, CompileOptions, LibraryNamespace, LibraryPath, SourceManager, Spanned,
AssemblyError, Compile, CompileOptions, LibraryNamespace, LibraryPath, SourceManager,
SourceManagerSync, Spanned,
ast::{self, Export, InvocationTarget, InvokeKind, ModuleKind, QualifiedProcedureName},
diagnostics::Report,
library::{KernelLibrary, Library},
Expand Down Expand Up @@ -64,7 +65,7 @@ pub use self::{
#[derive(Clone)]
pub struct Assembler {
/// The source manager to use for compilation and source location information
source_manager: Arc<dyn SourceManager + Send + Sync>,

@bitwalker bitwalker Jun 5, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I know you didn't change this as part of your PR, so this isn't critiquing your PR, but the change that clearly snuck by me at some point.

I somehow missed that the Assembler was made to require a Send + Sync impl. IMO the assembler should be made generic over this, and type aliases used for ergonomics if needed. Again, the vast majority (all?) Assembler uses aren't going to be in multi-threaded contexts, and it appears that we're primarily doing this so that we can thread the SourceManager impl through to somewhere via the Assembler (hence the source_manager method), and that somewhere needs the impl to be Send + Sync. That just feels totally backwards to me tbh.

A lot of this appears to be justified using the needs of the client, but rather than solving that complexity there, we're pushing it down into miden-core (and thus forcing all downstream crates which depend on it to deal with the effects of such changes as well). I really strongly disagree with the approach of making things in miden-core be biased to specific implementation details - things in this crate should almost universally prefer to punt those sorts of concerns/choices downstream.

@plafer Maybe you have more insight into this? It's not something I think we need to change in this PR, but I'd very much like to change Assembler to support both Sync and non-Sync uses, since as it stands now this effectively makes non-Sync impls of SourceManager borderline useless, even though that is by far the most common context in which they would be used.

@plafer plafer Jun 5, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This was requested by @PhilippGackstatter here, and implemented in #1834. My thinking was: making it Send + Sync will unblock miden-base, and all we're losing is maybe a bit of performance (due to e.g. internal mutexes) on the source manager, which is acceptable. And the DefaultSourceManager was already Send + Sync, so we would literally see no penalty in the current implementation.

I'm all for a cleaner approach if you (eventually) want to propose one, but honestly I just didn't spend too much time thinking about it.

@sergerad sergerad Jun 5, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think that is all fair for Sync but not Send. We simply have no solution downstream if this trait object is not Send.

@bitwalker bitwalker Jun 5, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm all for a cleaner approach if you (eventually) want to propose one, but honestly I just didn't spend too much time thinking about it.

Sure, sounds good!

My thinking was: making it Send + Sync will unblock miden-base, and all we're losing is maybe a bit of performance (due to e.g. internal mutexes) on the source manager, which is acceptable.

On the surface I think that rationale is fine, and ultimately some fix was needed to unblock miden-base, so the simpler one was the quickest way to facilitate that. The thing is that the quick fix was also a breaking change, and so far as I can recall, nobody checked with me if that would break things in the compiler. This has been a recurring problem, so I'm probably extra sensitive to it at this point. I guess my point is, we have to start being a lot more wary of changes that remove functionality - we simply have too many downstream dependents on these core crates to be making changes without careful evaluation of the consequences.

I'm not mad, I'm just disappointed 😛

source_manager: Arc<dyn SourceManagerSync>,
/// The global [ModuleGraph] for this assembler.
module_graph: ModuleGraph,
/// Whether to treat warning diagnostics as errors
Expand Down Expand Up @@ -93,7 +94,7 @@ impl Default for Assembler {
/// Constructors
impl Assembler {
/// Start building an [Assembler]
pub fn new(source_manager: Arc<dyn SourceManager + Send + Sync>) -> Self {
pub fn new(source_manager: Arc<dyn SourceManagerSync>) -> Self {
let module_graph = ModuleGraph::new(source_manager.clone());
Self {
source_manager,
Expand All @@ -106,7 +107,7 @@ impl Assembler {

/// Start building an [`Assembler`] with a kernel defined by the provided [KernelLibrary].
pub fn with_kernel(
source_manager: Arc<dyn SourceManager + Send + Sync>,
source_manager: Arc<dyn SourceManagerSync>,
kernel_lib: KernelLibrary,
) -> Self {
let (kernel, kernel_module, _) = kernel_lib.into_parts();
Expand Down Expand Up @@ -303,7 +304,7 @@ impl Assembler {
}

/// Returns a link to the source manager used by this assembler.
pub fn source_manager(&self) -> Arc<dyn SourceManager + Send + Sync> {
pub fn source_manager(&self) -> Arc<dyn SourceManagerSync> {
self.source_manager.clone()
}

Expand Down
4 changes: 2 additions & 2 deletions assembly/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ pub use self::{
assembler::Assembler,
compile::{Compile, Options as CompileOptions},
diagnostics::{
DefaultSourceManager, Report, SourceFile, SourceId, SourceManager, SourceSpan, Span,
Spanned,
DefaultSourceManager, Report, SourceFile, SourceId, SourceManager, SourceManagerSync,
SourceSpan, Span, Spanned,
},
errors::AssemblyError,
library::{
Expand Down
6 changes: 3 additions & 3 deletions assembly/src/testing.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::{
assembler::Assembler,
ast::{Form, Module, ModuleKind},
diagnostics::{
Report, SourceFile, SourceManager,
Report, SourceFile, SourceManagerSync,
reporting::{ReportHandlerOpts, set_hook},
},
library::Library,
Expand Down Expand Up @@ -195,7 +195,7 @@ macro_rules! parse_module {
///
/// Some of the assertion macros defined above require a [TestContext], so be aware of that.
pub struct TestContext {
source_manager: Arc<dyn SourceManager + Send + Sync>,
source_manager: Arc<dyn SourceManagerSync>,
assembler: Assembler,
}

Expand Down Expand Up @@ -233,7 +233,7 @@ impl TestContext {
}

#[inline(always)]
pub fn source_manager(&self) -> Arc<dyn SourceManager + Send + Sync> {
pub fn source_manager(&self) -> Arc<dyn SourceManagerSync> {
self.source_manager.clone()
}

Expand Down
2 changes: 1 addition & 1 deletion core/src/debuginfo/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ pub use self::{
source_file::{
ByteIndex, ByteOffset, ColumnIndex, LineIndex, SourceContent, SourceFile, SourceFileRef,
},
source_manager::{DefaultSourceManager, SourceId, SourceManager},
source_manager::{DefaultSourceManager, SourceId, SourceManager, SourceManagerSync},
span::{SourceSpan, Span, Spanned},
};
3 changes: 3 additions & 0 deletions core/src/debuginfo/source_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ impl SourceManagerError {
}
}

pub trait SourceManagerSync: SourceManager + Send + Sync {}
impl<T: SourceManager + Send + Sync> SourceManagerSync for T {}

pub trait SourceManager: Debug {
/// Returns true if `file` is managed by this source manager
fn is_manager_of(&self, file: &SourceFile) -> bool {
Expand Down
8 changes: 4 additions & 4 deletions miden/src/cli/data.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use std::{
};

use assembly::{
Assembler, Library, LibraryNamespace, SourceManager,
Assembler, Library, LibraryNamespace, SourceManagerSync,
ast::{Module, ModuleKind},
diagnostics::{Report, WrapErr},
report,
Expand Down Expand Up @@ -109,7 +109,7 @@ impl OutputFile {

pub struct ProgramFile {
ast: Box<Module>,
source_manager: Arc<dyn SourceManager + Send + Sync>,
source_manager: Arc<dyn SourceManagerSync>,
}

/// Helper methods to interact with masm program file.
Expand All @@ -125,7 +125,7 @@ impl ProgramFile {
#[instrument(name = "read_program_file", skip(source_manager), fields(path = %path.as_ref().display()))]
pub fn read_with(
path: impl AsRef<Path>,
source_manager: Arc<dyn SourceManager + Send + Sync>,
source_manager: Arc<dyn SourceManagerSync>,
) -> Result<Self, Report> {
// parse the program into an AST
let path = path.as_ref();
Expand Down Expand Up @@ -160,7 +160,7 @@ impl ProgramFile {
}

/// Returns the source manager for this program file.
pub fn source_manager(&self) -> &Arc<dyn SourceManager + Send + Sync> {
pub fn source_manager(&self) -> &Arc<dyn SourceManagerSync> {
&self.source_manager
}
}
Expand Down
6 changes: 4 additions & 2 deletions test-utils/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ use alloc::{
};

use assembly::{KernelLibrary, Library};
pub use assembly::{LibraryPath, SourceFile, SourceManager, diagnostics::Report};
pub use assembly::{
LibraryPath, SourceFile, SourceManager, SourceManagerSync, diagnostics::Report,
};
pub use pretty_assertions::{assert_eq, assert_ne, assert_str_eq};
pub use processor::{
AdviceInputs, AdviceProvider, ContextId, ExecutionError, ExecutionOptions, ExecutionTrace,
Expand Down Expand Up @@ -171,7 +173,7 @@ macro_rules! assert_assembler_diagnostic {
/// - Execution error test: check that running a program compiled from the given source causes an
/// ExecutionError which contains the specified substring.
pub struct Test {
pub source_manager: Arc<dyn SourceManager + Send + Sync>,
pub source_manager: Arc<dyn SourceManagerSync>,
pub source: Arc<SourceFile>,
pub kernel_source: Option<Arc<SourceFile>>,
pub stack_inputs: StackInputs,
Expand Down
Loading