Consider using a feature rather than cfg flag

[KamilaBorowska]

Currently Loom recommends using target specific dependencies.

[target.'cfg(loom)'.dependencies]
loom = "0.5"

This works, however it has certain drawbacks:

• Building with RUSTFLAGS="--cfg loom" requires rebuilding every dependency.

• cargo vendor vendors dependencies of loom.

• Dependencies of loom are included in Cargo.lock when using a crate that depends on loom.

• cargo audit includes dependencies of loom when using a crate that depends on loom.

• Requiring an user to use RUSTFLAGS="--cfg loom" to test crate with loom.

• Requiring an user to import loom or std types depending on whether loom is enabled.

  #[cfg(loom)]
  pub(crate) use loom::sync::atomic::AtomicUsize;
  
  #[cfg(not(loom))]
  pub(crate) use std::sync::atomic::AtomicUsize;

All those issues would be solvable if loom did use a feature rather than a config flag. To clarify what I mean: the user could use loom like this in Cargo.toml.

[dependencies]
loom = "0.6"
[dev-dependencies]
loom = { version = "0.6", features = ["model"] }

Then import whatever they want by simply using use, without having to bother with #[cfg] or cfg_if!.

use loom::sync::atomic::AtomicUsize;

That would work because Loom crate would provide a basic AtomicUsize implementation that directly uses methods from std when model feature is not enabled. This means no performance cost when not using model feature.

if_cfg! {
    if #[cfg(feature = "model")] {
        pub struct AtomicUsize(loom::Atomic<usize>);
        impl AtomicUsize {
            #[track_caller]
            pub fn with_mut<R>(&mut self, f: impl FnOnce(&mut usize) -> R) -> R {
                self.0.with_mut(f)
            }

            // ...
        }
    } else {
        pub struct AtomicUsize(std::sync::atomic::AtomicUsize);
        impl AtomicUsize {
            pub fn with_mut<R>(&mut self, f: impl FnOnce(&mut usize) -> R) -> R {
                f(self.get_mut())
            }

            // ...
        }
    }
}

Some possible concerns:

• Does that mean that crates that use loom have to use loom as a dependency? No, if they choose to they can use #[cfg(test)] themselves to mock loom APIs, just like they have to right now.
• What about running loom tests from another library (see Having loom as dev-dependency incompatible with loom integration tests #134)? Cargo compiles crates once, having a feature turned on turns it on for all users.
• Would that cause a performance cost for programs that use loom in tests due to Cargo merging all features used within a crate? No, assuming new feature resolver is being used (Rust 1.51+, needs to be opted-in with resolver = "2" or edition = "2021"). That said, a note in documentation should be added recommending using new feature resolver for programs that use loom for testing themselves (it's not necessary for libraries or programs that happen to use libraries that use loom).

[taiki-e]

cargo feature needs to be additive. see also crossbeam-rs/crossbeam#638

[KamilaBorowska]

This is additive I believe unless I'm missing something? A model feature would add loom::model function to an API, and that's it. It also would modify a lot of things to interact with model, but the user-visible API wouldn't change, and enabling model wouldn't make changes in other functions visible when not using model function (other than making them slower).

[KamilaBorowska]

Oh, I see, Loom functions expect loom::model to be used first? Yeah, that is an issue. I wonder if it's possible to make them behave like normal functions when not in loom::model scope.

[taiki-e]

This is additive I believe unless I'm missing something? A model feature would add loom::model function to an API, and that's it. It also would modify a lot of things to interact with model, but the user-visible API wouldn't change, and enabling model wouldn't make changes in other functions visible when not using model function (other than making them slower).

Some of the features do not work outside of the loom::model function.
For example, removing this function's loom::model will cause a panic with "cannot access a scoped thread local variable without calling set first".

loom/tests/atomic.rs

Lines 32 to 42 in 0d7f0d5

	#[test]
	fn lazy_static_arc_race() {
	loom::model(|| {
	let jh = thread::spawn(|| {
	assert_eq!(**ARC_WITH_SLOW_CONSTRUCTOR, 0);
	});
	assert_eq!(**ARC_WITH_SLOW_CONSTRUCTOR, 0);
	jh.join().unwrap();
	});
	}

A feature that can break most of the existing tests with panic should not be called "additive", even if it does not seem to break the API.

[taiki-e]

• cargo vendor vendors dependencies of loom.
• Dependencies of loom are included in Cargo.lock when using a crate that depends on loom.
• cargo audit includes dependencies of loom when using a crate that depends on loom.

The workaround for these problems is to make loom an optional dependency: crossbeam-rs/crossbeam#666