fix: centralize unsafe MLIR access and deny unsafe elsewhere (#415)

Cargo.toml

@@ -65,7 +65,10 @@ license-file = "LICENSE"
 repository = "https://github.com/arniber21/quon"
 
 [workspace.lints.rust]
-unsafe_code = "allow"
+unsafe_code = "deny"
+
+[workspace.lints.clippy]
+undocumented_unsafe_blocks = "warn"
 
 [profile.release]
 lto = true

docs/adr/0004-quantum-circ-as-unregistered-dialect.md

@@ -36,7 +36,10 @@ IRDL, and it would still not give us Rust verifier callbacks.
   every op builder, so a malformed op cannot be constructed through the public API, and by
   exposing `verify` for callers that build ops by hand.
 - Diagnostic *emission* is not wrapped by Melior, so all error reporting flows through the
-  `diagnostics` module, which isolates the single `unsafe` `mlirEmitError` boundary behind a
-  `Result`/Writer-style abstraction.
+  `diagnostics` module, which delegates to the `ffi` module's safe `emit_error` wrapper. The
+  `ffi` module is the sole audited unsafe boundary in `mlir_bridge`: it owns
+  `mlirEmitError`, `mlirOperationSetAttributeByName`, `mlirOperationSetOperand`, and
+  external-pass context lifetime erasure. The workspace denies `unsafe_code` everywhere
+  else.
 - Custom types are opaque strings (`!quantum.qubit`, `!quantum.circ`); type checks compare the
   printed form rather than a registered `TypeID`.

docs/agents/code-quality.md

@@ -76,10 +76,10 @@ When adding fallible parsing or serialization, add at least: happy-path unit tes
 | **quonc** | `anyhow::Result` | CLI driver; may aggregate errors from the pipeline (`quonc/src/main.rs`). |
 | **frontend** | `thiserror` (`TypeError`, etc.) | Library API returns typed errors; span-aware reporting via `ariadne`. |
 | **backend** | `thiserror` (`BackendError`) | Descriptor JSON → domain conversion; every fallible path returns `Result` (`backend/src/error.rs`). |
-| **mlir_bridge** | `thiserror` per module + **`Diagnostics` monad** | Verifiers return `Result<(), E>`; passes accumulate with `Diagnostics::report` and flush once at the FFI boundary (`mlir_bridge/src/diagnostics.rs`). |
+| **mlir_bridge** | `thiserror` per module + **`Diagnostics` monad** | Verifiers return `Result<(), E>`; passes accumulate with `Diagnostics::report` and flush once at the FFI boundary (`mlir_bridge/src/ffi.rs`). |
 | **zx** | Typed errors (follow workspace convention) | Graph transforms; no `anyhow` in library code. |
 
-**Diagnostics monad (mlir_bridge):** Dialect verifiers and passes stay pure Rust. They build a `Diagnostics` accumulator, fold `Result` values with `.report(location, result)`, and only `Diagnostics::emit` crosses into unsafe MLIR C API. Do not call `mlirEmitError` outside `diagnostics.rs`.
+**Diagnostics monad (mlir_bridge):** Dialect verifiers and passes stay pure Rust. They build a `Diagnostics` accumulator, fold `Result` values with `.report(location, result)`, and only `Diagnostics::emit` flushes to MLIR — via the safe `ffi::emit_error` wrapper. The `ffi` module (`mlir_bridge/src/ffi.rs`) is the sole audited unsafe boundary: it owns `mlirEmitError`, `mlirOperationSetAttributeByName`, `mlirOperationSetOperand`, and external-pass context lifetime erasure (`PassContext`). The workspace denies `unsafe_code` everywhere else. Do not call `mlir-sys` functions outside `ffi.rs`.
 
 **anyhow:** Reserved for the **quonc** binary. Library crates should use `thiserror` enums (or plain `Result<T, E>` with a small `E`). Taskless rule `no-anyhow-in-lib-src` enforces this on new code.
 

mlir_bridge/src/diagnostics.rs

@@ -9,19 +9,19 @@
 //!     accumulator (a Writer-style "diagnostic monad") and return it, or fold a
 //!     typed [`Result`] into it with [`Diagnostics::report`]. None of that code
 //!     touches the FFI boundary.
-//!   * [`Diagnostics::emit`] is the *single* place in the crate that crosses
-//!     into the unsafe MLIR C API. It is `safe` to call: the one `unsafe` block
-//!     it contains is self-contained and sound (a valid location plus a
-//!     NUL-free C string).
+//!   * [`Diagnostics::emit`] flushes the accumulator to MLIR via the safe
+//!     [`crate::ffi::emit_error`] wrapper. The `unsafe` `mlirEmitError` call
+//!     lives solely in [`crate::ffi`], alongside all other unsafe MLIR FFI in
+//!     the crate.
 //!
-//! This keeps the unsafe surface to one auditable function while the rest of
-//! the bridge composes diagnostics with ordinary `Result`/iterator combinators.
+//! This keeps the unsafe surface to one audited module ([`crate::ffi`]) while
+//! the rest of the bridge composes diagnostics with ordinary
+//! `Result`/iterator combinators.
 
 use std::ffi::CString;
 use std::fmt;
 
 use melior::ir::Location;
-use mlir_sys::mlirEmitError;
 
 /// A single error diagnostic anchored at an IR [`Location`].
 #[derive(Clone)]
@@ -51,21 +51,17 @@ impl<'c> Diagnostic<'c> {
 
     /// Emits this diagnostic into MLIR's diagnostic engine.
     ///
-    /// This is the only function in the crate that calls into the MLIR C
-    /// diagnostic API. The message is sanitized of interior NUL bytes so the
-    /// `CString` conversion is infallible.
+    /// The message is sanitized of interior NUL bytes so the `CString`
+    /// conversion is infallible. The actual FFI call is delegated to the safe
+    /// [`crate::ffi::emit_error`] wrapper — this module contains no `unsafe` code.
     fn emit(&self) {
         let sanitized: String = self
             .message
             .chars()
             .map(|c| if c == '\0' { '?' } else { c })
             .collect();
         let message = CString::new(sanitized).unwrap_or_default();
-        // SAFETY: `self.location` is a live MLIR location owned by the context,
-        // and `message` is a valid NUL-terminated C string that outlives the
-        // call. `mlirEmitError` copies the message and does not retain the
-        // pointer.
-        unsafe { mlirEmitError(self.location.to_raw(), message.as_ptr()) };
+        crate::ffi::emit_error(&self.location, &message);
     }
 }
 

mlir_bridge/src/ffi.rs

@@ -0,0 +1,173 @@
+//! Centralized unsafe FFI boundary for the MLIR bridge.
+//!
+//! All `unsafe` code in `mlir_bridge` is confined to this module. Pass
+//! implementations, verifiers, and the [`crate::diagnostics`] accumulator
+//! compose the safe wrappers exported here and never touch raw `mlir-sys`
+//! pointers directly.
+//!
+//! Three concerns are centralized:
+//!   * Error emission (`mlirEmitError`) — called by [`crate::diagnostics`].
+//!   * Raw operation mutation (`mlirOperationSetAttributeByName`,
+//!     `mlirOperationSetOperand`).
+//!   * External-pass context lifetime erasure ([`PassContext`] +
+//!     [`with_context`]).
+//!
+//! Every `unsafe` block below carries a `SAFETY` comment tied to the upstream
+//! FFI contract or the MLIR pass-framework lifetime guarantee.
+
+#![allow(unsafe_code)]
+
+use std::ffi::CString;
+
+use melior::StringRef;
+use melior::ir::{Attribute, AttributeLike, Location, OperationRef, Value, ValueLike};
+use melior::{Context, ContextRef};
+use mlir_sys::{
+    MlirContext, mlirEmitError, mlirOperationSetAttributeByName, mlirOperationSetOperand,
+};
+
+// ─── Error emission ─────────────────────────────────────────────────────────
+
+/// Emits an error diagnostic at `location` with the given C string message.
+///
+/// This is the sole call site for `mlirEmitError` in the crate. The message
+/// must already be a valid NUL-terminated C string (the [`crate::diagnostics`]
+/// module sanitizes interior NULs before calling).
+pub(crate) fn emit_error(location: &Location<'_>, message: &CString) {
+    // SAFETY: `location` is a live MLIR location backed by the context that
+    // owns it. `message` is a valid NUL-terminated C string whose backing
+    // buffer outlives the call. `mlirEmitError` copies the message internally
+    // and does not retain the pointer after returning.
+    unsafe { mlirEmitError(location.to_raw(), message.as_ptr()) };
+}
+
+// ─── Raw operation mutation ─────────────────────────────────────────────────
+
+/// Sets a named attribute on an MLIR operation.
+///
+/// Safe wrapper for `mlirOperationSetAttributeByName`. The operation and
+/// attribute must belong to the same context.
+pub(crate) fn set_operation_attribute<'c>(
+    op: OperationRef<'c, '_>,
+    name: &str,
+    attribute: &Attribute<'c>,
+) {
+    // SAFETY: `op` is a live operation reference. `name` is borrowed as a
+    // `StringRef` for the duration of the call; the C function copies it
+    // internally via `MlirStringRef` and does not retain the pointer.
+    // `attribute` is a live attribute owned by the same context. The function
+    // performs an in-place attribute update and does not retain any pointer
+    // after returning.
+    unsafe {
+        mlirOperationSetAttributeByName(
+            op.to_raw(),
+            StringRef::new(name).to_raw(),
+            attribute.to_raw(),
+        );
+    }
+}
+
+/// Replaces a single operand of an MLIR operation.
+///
+/// Safe wrapper for `mlirOperationSetOperand`. The operation and value must
+/// belong to the same context. The caller is responsible for ensuring `index`
+/// is a valid operand slot.
+pub(crate) fn set_operation_operand<'c, 'a>(
+    op: OperationRef<'c, 'a>,
+    index: isize,
+    value: &Value<'c, 'a>,
+) {
+    // SAFETY: `op` is a live operation reference. `index` is a valid operand
+    // index validated by the caller. `value` is a live SSA value belonging to
+    // the same context. The function updates the operand slot in place and
+    // does not retain the pointers after returning.
+    unsafe {
+        mlirOperationSetOperand(op.to_raw(), index, value.to_raw());
+    }
+}
+
+// ─── External-pass context lifetime erasure ────────────────────────────────
+
+/// Erases the external-pass context lifetime so that a `'static` pass struct
+/// can hold a context reference between `initialize` and `run`.
+///
+/// MLIR's `RunExternalPass` trait hands passes a `ContextRef<'c>` in
+/// `initialize`, but the pass struct must be `'static` and cannot hold a
+/// `&'c Context` directly. The pass framework guarantees that the context
+/// remains valid for the entire lifetime of the pass.
+///
+/// `PassContext` stores the raw `MlirContext` handle (a `Copy` value, not a
+/// pointer to stack memory) extracted in `initialize`. In `run`, the handle is
+/// reconstructed into a `ContextRef` whose lifetime is scoped to the
+/// [`with_context`] closure, ensuring the `&Context` is always valid.
+///
+/// # Why not store a `&Context` pointer?
+///
+/// `ContextRef::to_ref` uses `transmute` to return a `&Context` that points to
+/// the `ContextRef` itself (they share the same layout: a single
+/// `MlirContext`). Storing that pointer for later use is undefined behaviour
+/// because the `ContextRef` is a stack local. Storing the `MlirContext` handle
+/// by value and reconstructing the `ContextRef` in `run` avoids this pitfall.
+#[derive(Clone, Copy, Default)]
+pub(crate) struct PassContext {
+    raw: Option<MlirContext>,
+}
+
+impl PassContext {
+    /// Creates an empty context store (no context captured yet).
+    pub(crate) fn new() -> Self {
+        Self { raw: None }
+    }
+
+    /// Captures the MLIR context handle for later retrieval.
+    ///
+    /// The `MlirContext` handle is extracted by value from the `ContextRef`.
+    /// No reference to the `ContextRef` (which is a stack local) is retained.
+    pub(crate) fn capture<'c>(&mut self, context: ContextRef<'c>) {
+        // SAFETY: `to_ref` transmutes `&ContextRef` into `&Context` (same
+        // layout), and `to_raw` copies the `MlirContext` handle out by value.
+        // The `&Context` is alive only for this expression; the stored
+        // `MlirContext` is an independent value, not a dangling pointer.
+        self.raw = Some(unsafe { context.to_ref().to_raw() });
+    }
+
+    /// Whether a context has been captured.
+    pub(crate) fn is_captured(&self) -> bool {
+        self.raw.is_some()
+    }
+
+    /// Returns the raw `MlirContext` handle, or `None` if [`capture`](Self::capture)
+    /// was never called.
+    ///
+    /// The handle is safe to pass to [`with_context`].
+    pub(crate) fn raw(&self) -> Option<MlirContext> {
+        self.raw
+    }
+}
+
+/// Runs a closure with a borrowed `&'c Context` reconstructed from a stored
+/// `MlirContext` handle.
+///
+/// The `ContextRef` is created as a local variable inside this function, so
+/// the `&Context` returned by `to_ref` (which `transmute`s `&ContextRef` into
+/// `&Context`) is valid for the duration of the closure call. After the
+/// closure returns, both are dropped — no dangling pointer escapes.
+///
+/// # Panics
+///
+/// Panics if `raw` is a null/invalid handle. The pass framework guarantees
+/// validity, so this should never happen in practice.
+pub(crate) fn with_context<'c, R>(raw: MlirContext, f: impl FnOnce(&'c Context) -> R) -> R {
+    // SAFETY: The `MlirContext` handle was obtained in `PassContext::capture`
+    // from a `ContextRef` provided by `RunExternalPass::initialize`. The pass
+    // framework guarantees the context outlives every `run` call, so the
+    // handle is still valid here.
+    let context_ref = unsafe { ContextRef::from_raw(raw) };
+    // SAFETY: `context_ref` is a local variable alive for the duration of this
+    // function. `to_ref` transmutes `&context_ref` into `&'c Context` — the
+    // reference points to `context_ref`'s stack slot, which is valid until
+    // this function returns. The closure consumes the reference before that,
+    // so no dangling pointer escapes.
+    let context = unsafe { context_ref.to_ref() };
+    f(context)
+}

mlir_bridge/src/fixed_physical.rs

@@ -29,15 +29,14 @@
 
 use backend::BackendTarget;
 use melior::Context;
-use melior::StringRef;
 use melior::ir::Module;
 use melior::ir::attribute::IntegerAttribute;
 use melior::ir::operation::OperationLike;
 use melior::ir::r#type::IntegerType;
-use melior::ir::{AttributeLike, BlockLike, OperationRef, RegionLike};
-use mlir_sys::mlirOperationSetAttributeByName;
+use melior::ir::{BlockLike, OperationRef, RegionLike};
 
 use crate::dialect::quantum_dynamic;
+use crate::ffi;
 use crate::metrics;
 use crate::passes::{
     depth_scheduling, native_gate_decomp, sabre_routing, sabre_routing::SabreCost,
@@ -85,8 +84,7 @@ pub fn run_fixed_physical(
 pub fn corrupt_phys_qubit_attrs(context: &Context, module: &Module<'_>, bogus: i32) {
     let attr: melior::ir::Attribute<'_> =
         IntegerAttribute::new(IntegerType::new(context, 32).into(), i64::from(bogus)).into();
-    let raw = attr.to_raw();
-    let name = StringRef::new(quantum_dynamic::attr::PHYS_QUBIT).to_raw();
+    let name = quantum_dynamic::attr::PHYS_QUBIT;
     let Some(body) = module
         .as_operation()
         .region(0)
@@ -95,13 +93,13 @@ pub fn corrupt_phys_qubit_attrs(context: &Context, module: &Module<'_>, bogus: i
     else {
         return;
     };
-    corrupt_block(body, name, raw);
+    corrupt_block(body, name, &attr);
 }
 
 fn corrupt_block<'c, 'a>(
     block: melior::ir::BlockRef<'c, 'a>,
-    name: mlir_sys::MlirStringRef,
-    attr: mlir_sys::MlirAttribute,
+    name: &str,
+    attr: &melior::ir::Attribute<'c>,
 ) {
     let mut op = block.first_operation();
     while let Some(current) = op {
@@ -112,15 +110,13 @@ fn corrupt_block<'c, 'a>(
 
 fn corrupt_op<'c, 'a>(
     op: OperationRef<'c, 'a>,
-    name: mlir_sys::MlirStringRef,
-    attr: mlir_sys::MlirAttribute,
+    name: &str,
+    attr: &melior::ir::Attribute<'c>,
 ) {
     // Only ops that already have a phys_qubit attr are touched — overwriting
     // a non-existent attr would *add* one, which is not the test's intent.
     if op.attribute(quantum_dynamic::attr::PHYS_QUBIT).is_ok() {
-        unsafe {
-            mlirOperationSetAttributeByName(op.to_raw(), name, attr);
-        }
+        ffi::set_operation_attribute(op, name, attr);
     }
     let count = op.region_count();
     for index in 0..count {

mlir_bridge/src/lib.rs

@@ -2,6 +2,7 @@
 
 pub mod circ_extract;
 pub mod diagnostics;
+pub mod ffi;
 pub mod dialect;
 pub mod dynamic_walk;
 pub mod emit;

mlir_bridge/src/passes/classical_region_fusion.rs

@@ -30,6 +30,7 @@ use melior::pass::{ExternalPass, Pass, RunExternalPass, create_external};
 use melior::{Context, ContextRef, IrRewriter};
 use thiserror::Error;
 
+use crate::ffi::PassContext;
 use crate::diagnostics::Diagnostics;
 use crate::dialect::{quantum_circ, quantum_dynamic};
 use crate::passes::qubit_wiring::{self, WireTracker};
@@ -688,29 +689,31 @@ static CLASSICAL_REGION_FUSION_PASS_ID: PassId = PassId;
 
 #[derive(Clone)]
 struct ClassicalRegionFusion {
-    context: usize,
+    context: PassContext,
 }
 
 impl ClassicalRegionFusion {
     fn new() -> Self {
-        Self { context: 0 }
+        Self { context: PassContext::new() }
     }
 }
 
 impl<'c> RunExternalPass<'c> for ClassicalRegionFusion {
     fn initialize(&mut self, context: ContextRef<'c>) {
-        self.context = unsafe { context.to_ref() as *const Context as usize };
+        self.context.capture(context);
     }
 
     fn run(&mut self, operation: OperationRef<'c, '_>, pass: ExternalPass<'_>) {
-        if self.context == 0 {
+        let Some(raw) = self.context.raw() else {
             pass.signal_failure();
             return;
-        }
-        let context = unsafe { &*(self.context as *const Context) };
-        let mut diagnostics = Diagnostics::new();
-        fuse_module(context, operation, &mut diagnostics);
-        if !diagnostics.emit() {
+        };
+        let success = crate::ffi::with_context(raw, |context| {
+            let mut diagnostics = Diagnostics::new();
+            fuse_module(context, operation, &mut diagnostics);
+            diagnostics.emit()
+        });
+        if !success {
             pass.signal_failure();
         }
     }