feat!: add opt-in interior mutability via a Ref type parameter

Shared `Variable` and `Array` declarations were reachable through many
`Literal::Variable` references but could not be edited, since `Arc<T>`
hands out no `&mut`. Adding/removing variables and constraints already
worked (those are plain `Vec`s the caller owns); mutating the interior of
a shared declaration did not, which is what a presolve or rewriting pass
needs.

Every public generic type now carries a trailing `Ref` parameter,
defaulted to `Immutable`:

    helpers::Immutable  ->  Of<T> = Arc<T>            (today's behaviour)
    helpers::Mutable    ->  Of<T> = Arc<RwLock<T>>

    let fzn: FlatZinc<String, Mutable> = FlatZinc::from_fzn(rdr)?;
    var.write().unwrap().ty = Type::Int(Some(RangeList::from(0..=1)));

A defaulted type parameter is the only opt-in that stays source
compatible; a cargo feature would be unification-unsafe for a published
library, silently changing the public API when two dependents disagree.

`Mutable::Of<T>` is just `Arc<RwLock<T>>`, so edits go through the normal
lock API and no `with_mut` helper is needed. `ArcKey` and `cloned_key`
are untouched: `ArcKey<T>` is generic over `T` and already serves both
markers as `ArcKey<RwLock<Variable<..>>>`.

BREAKING CHANGE: `NamedRef::name` returns `Cow<'_, str>` rather than
`&str`; the name lives behind a lock under `Mutable`, so no borrow can
escape. `Cow` derefs to `str`, so most call sites are unaffected.

BREAKING CHANGE: `Argument`'s parameters are now `<Identifier, Ref, L>`.
`L` defaults to `Literal<Identifier, Ref>`, and a default cannot forward
-reference a later parameter, so `Ref` has to precede `L` here even
though it trails on the other eleven types.

BREAKING CHANGE: the `From<Arc<..>> for NamedRef` conversions are now
implemented per marker rather than generically. An associated type is
opaque, so the compiler cannot rule out `Ref::Of<Array<..>>` and
`Ref::Of<Variable<..>>` naming the same type, and the generic impls
overlap. Generic code should construct the variants directly.

Corpus fixtures are unchanged: `Immutable` is the default and its `Debug`
and `Display` output forwards exactly as `Arc` did. The `Mutable`
deadlock hazard — `NamedRef`'s `Hash`, `Ord`, and `PartialEq` take a read
lock to reach the name — is documented on both types.

Author: Dekker1

src/fzn.rs

@@ -16,6 +16,7 @@ use winnow::{
 use crate::{
 	FlatZinc, Type,
 	error::FznParseError,
+	helpers::FznRef,
 	intermediate::{
 		self, Argument, Array, Constraint, Declaration, Literal, Method, NameId, ParserState,
 		SolveObjective, Variable,
@@ -231,7 +232,9 @@ fn map_parse_error<Identifier, F>(
 ///
 /// This is used by [`crate::FlatZinc::from_fzn`], which is the public entry
 /// point for `.fzn` parsing.
-pub(crate) fn parse<I, E>(source: impl BufRead) -> Result<FlatZinc<I>, FznParseError>
+pub(crate) fn parse<I, E, Ref: FznRef>(
+	source: impl BufRead,
+) -> Result<FlatZinc<I, Ref>, FznParseError>
 where
 	I: Clone + for<'a> TryFrom<&'a str, Error = E>,
 	E: Display,
@@ -241,10 +244,10 @@ where
 
 /// Parse the `.fzn` source to a [`FlatZinc`] instance using a custom
 /// identifier interner.
-pub(crate) fn parse_with_interner<I, F, E>(
+pub(crate) fn parse_with_interner<I, F, E, Ref: FznRef>(
 	mut source: impl BufRead,
 	mut interner: F,
-) -> Result<FlatZinc<I>, FznParseError>
+) -> Result<FlatZinc<I, Ref>, FznParseError>
 where
 	I: Clone,
 	F: FnMut(&str) -> Result<I, E>,
@@ -608,6 +611,7 @@ mod tests {
 	}
 
 	use std::{
+		borrow::Cow,
 		convert::Infallible,
 		fmt::Debug,
 		fs::File,
@@ -811,7 +815,7 @@ mod tests {
 			.unwrap_or_else(|| panic!("expected interned name `{expected}`"))
 	}
 
-	fn output_names<Identifier>(fzn: &FlatZinc<Identifier>) -> Vec<&str> {
+	fn output_names<Identifier>(fzn: &FlatZinc<Identifier>) -> Vec<Cow<'_, str>> {
 		fzn.output.iter().map(NamedRef::name).collect()
 	}
 

src/helpers.rs

@@ -2,11 +2,139 @@
 //! deserialization of FlatZinc data.
 
 use std::{
+	borrow::Cow,
+	fmt::Debug,
 	hash::{Hash, Hasher},
 	ops::Deref,
-	sync::Arc,
+	sync::{Arc, PoisonError, RwLock},
 };
 
+/// A family of smart pointers used to share the [`Variable`](crate::Variable)
+/// and [`Array`](crate::Array) declarations of a
+/// [`FlatZinc`](crate::FlatZinc) instance.
+///
+/// This is the trait behind the `Ref` type parameter of the public types. It
+/// selects whether the shared declarations are plain [`Immutable`] `Arc<T>`
+/// values, as produced by parsing, or [`Mutable`] `Arc<RwLock<T>>` values that
+/// can be edited in place after the instance has been constructed.
+///
+/// The supertraits are free — implementors are zero-sized markers — and let the
+/// derived `Clone`, `Debug`, and `PartialEq` implementations on the public
+/// types, which bound every type parameter, be satisfied by `Ref: FznRef`
+/// alone.
+pub trait FznRef: Copy + Debug + Eq {
+	/// The reference type used to share a `T`.
+	type Of<T>: Clone;
+
+	/// Allocate a new shared declaration.
+	fn new<T>(value: T) -> Self::Of<T>;
+
+	/// Borrow the shared declaration for reading, and apply `f` to it.
+	fn with<T, R>(node: &Self::Of<T>, f: impl FnOnce(&T) -> R) -> R;
+
+	/// The address of the shared allocation, used to test pointer identity.
+	///
+	/// Comparing two references with [`FznRef::with`] would take two read locks
+	/// under [`Mutable`], which deadlocks if both name the same declaration and
+	/// a writer is waiting. Testing this first avoids that.
+	///
+	/// This returns a bare address rather than a pointer, matching the
+	/// `addr` method on raw pointers, because the result is only ever compared:
+	/// keeping it a pointer would carry provenance that no caller is entitled
+	/// to use.
+	fn addr<T>(node: &Self::Of<T>) -> usize;
+
+	/// Project a string field out of the shared declaration.
+	///
+	/// The result borrows from `node` for [`Immutable`], but must be cloned for
+	/// [`Mutable`], where no borrow can escape the lock guard.
+	///
+	/// This exists solely for [`NamedRef::name`](crate::NamedRef::name), which
+	/// must hand back a value that outlives the call. Anything that consumes
+	/// the name in place should use [`FznRef::with`] instead, which allocates
+	/// under neither marker.
+	fn map_str<'a, T>(node: &'a Self::Of<T>, f: impl FnOnce(&T) -> &str) -> Cow<'a, str>;
+}
+
+/// Marker selecting immutable shared declarations, represented as `Arc<T>`.
+///
+/// This is the default, and the representation produced by parsing. Reading a
+/// declaration is a plain pointer dereference.
+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+pub struct Immutable;
+
+/// Marker selecting interior-mutable shared declarations, represented as
+/// `Arc<RwLock<T>>`.
+///
+/// This allows a [`Variable`](crate::Variable) or [`Array`](crate::Array) to be
+/// edited after the instance has been constructed, with every reference to that
+/// declaration observing the change. Since `Mutable::Of<T>` is simply
+/// `Arc<RwLock<T>>`, edits are made through the normal lock API:
+///
+/// ```
+/// # use std::sync::{Arc, RwLock};
+/// # use flatzinc_serde::{Type, Variable, helpers::Mutable};
+/// # let var: Arc<RwLock<Variable<String, Mutable>>> = Arc::new(RwLock::new(Variable {
+/// #     name: "x".to_owned(),
+/// #     ty: Type::Int(None),
+/// #     ann: Vec::new(),
+/// #     defined: false,
+/// #     introduced: false,
+/// # }));
+/// var.write().unwrap().ty = Type::Bool;
+/// ```
+///
+/// ### Warning
+///
+/// Reading a declaration takes a read lock, and several trait implementations
+/// do so implicitly: [`Display`](std::fmt::Display) and `Serialize` on any type
+/// that reaches a declaration, and [`Hash`], [`Ord`], and [`PartialEq`] on
+/// [`NamedRef`](crate::NamedRef), which read the declaration name. Holding a
+/// write guard on a declaration while invoking any of these on a value that
+/// reaches the same declaration will deadlock.
+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+pub struct Mutable;
+
+impl FznRef for Immutable {
+	type Of<T> = Arc<T>;
+
+	fn new<T>(value: T) -> Self::Of<T> {
+		Arc::new(value)
+	}
+
+	fn with<T, R>(node: &Self::Of<T>, f: impl FnOnce(&T) -> R) -> R {
+		f(node)
+	}
+
+	fn addr<T>(node: &Self::Of<T>) -> usize {
+		Arc::as_ptr(node).addr()
+	}
+
+	fn map_str<'a, T>(node: &'a Self::Of<T>, f: impl FnOnce(&T) -> &str) -> Cow<'a, str> {
+		Cow::Borrowed(f(node))
+	}
+}
+
+impl FznRef for Mutable {
+	type Of<T> = Arc<RwLock<T>>;
+
+	fn new<T>(value: T) -> Self::Of<T> {
+		Arc::new(RwLock::new(value))
+	}
+
+	fn with<T, R>(node: &Self::Of<T>, f: impl FnOnce(&T) -> R) -> R {
+		f(&node.read().unwrap_or_else(PoisonError::into_inner))
+	}
+
+	fn addr<T>(node: &Self::Of<T>) -> usize {
+		Arc::as_ptr(node).addr()
+	}
+
+	fn map_str<'a, T>(node: &'a Self::Of<T>, f: impl FnOnce(&T) -> &str) -> Cow<'a, str> {
+		Cow::Owned(f(&node.read().unwrap_or_else(PoisonError::into_inner)).to_owned())
+	}
+}
+
 /// A wrapper around an [`Arc`] that can be used as a key for collections, such
 /// as [`BTreeMap`](std::collections::BTreeMap),
 /// [`HashMap`](std::collections::HashMap), and
@@ -47,13 +175,15 @@ impl<T> Eq for ArcKey<T> {}
 
 impl<T> Hash for ArcKey<T> {
 	fn hash<H: Hasher>(&self, state: &mut H) {
-		Arc::as_ptr(&self.key).hash(state);
+		Arc::as_ptr(&self.key).addr().hash(state);
 	}
 }
 
 impl<T> Ord for ArcKey<T> {
 	fn cmp(&self, other: &Self) -> std::cmp::Ordering {
-		Arc::as_ptr(&self.key).cmp(&Arc::as_ptr(&other.key))
+		Arc::as_ptr(&self.key)
+			.addr()
+			.cmp(&Arc::as_ptr(&other.key).addr())
 	}
 }