`assert_contains` changed in a semantically-observable manner between 9.5 and 9.6

[shepmaster]

fn main() {
    let s = String::new();
    assertables::assert_contains!(s, "");
    assertables::assert_contains!(s, "");
}

% cargo update -p assertables --precise 9.5.0
    Updating crates.io index

% cargo run
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/repro`

% cargo update -p assertables --precise 9.6.0
    Updating crates.io index
    Updating assertables v9.5.0 -> v9.6.0

% cargo run
   Compiling repro v0.1.0 (/private/tmp/repro)
error[E0382]: use of moved value: `s`
 --> src/main.rs:4:35
  |
2 |     let s = String::new();
  |         - move occurs because `s` has type `String`, which does not implement the `Copy` trait
3 |     assertables::assert_contains!(s, "");
  |                                   - value moved here
4 |     assertables::assert_contains!(s, "");
  |                                   ^ value used here after move
  |
help: consider cloning the value if the performance cost is acceptable
  |
3 |     assertables::assert_contains!(s.clone(), "");
  |                                    ++++++++

For more information about this error, try `rustc --explain E0382`.
error: could not compile `repro` (bin "repro") due to 1 previous error

[joelparkerhenderson]

Excellent catch, thank you.

[I'm sunsetting much of this comment because my info is no longer correct-- the fix is now ready]

[shepmaster]

The fix in 9.5.6

Note that the code compiles in 9.5.6:

% cargo update -p assertables --precise 9.5.6
    Updating crates.io index
 Downgrading assertables v9.6.0 -> v9.5.6

% cargo check
    Checking assertables v9.5.6
    Checking repro v0.1.0 (/private/tmp/repro)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.16s

Is it possible for you to upgrade to 9.6.1

Yes, but I'm not sure I understand the suggestion — the code continues to fail with that version.

% cargo update -p assertables --precise 9.6.1
    Updating crates.io index
    Updating assertables v9.5.6 -> v9.6.1

% cargo check
  Downloaded assertables v9.6.1
  Downloaded 1 crate (178.3KiB) in 0.21s
    Checking assertables v9.6.1
    Checking repro v0.1.0 (/private/tmp/repro)
error[E0382]: use of moved value: `s`
 --> src/main.rs:4:35
  |
2 |     let s = String::new();
  |         - move occurs because `s` has type `String`, which does not implement the `Copy` trait
3 |     assertables::assert_contains!(s, "");
  |                                   - value moved here
4 |     assertables::assert_contains!(s, "");
  |                                   ^ value used here after move
  |
help: consider cloning the value if the performance cost is acceptable
  |
3 |     assertables::assert_contains!(s.clone(), "");
  |                                    ++++++++

creating a regression test

I would normally suggest cargo semver-checks, but it does not seem to catch this issue. I'm guessing because it's in a macro:

% cargo semver-checks --baseline-version 9.5.6
    Building assertables v9.6.0 (current)
       Built [   0.423s] (current)
     Parsing assertables v9.6.0 (current)
      Parsed [   0.004s] (current)
     Parsing assertables v9.5.6 (baseline, cached)
      Parsed [   0.013s] (baseline)
    Checking assertables v9.5.6 -> v9.6.0 (minor change)
     Checked [   0.011s] 128 checks: 128 pass, 36 skip
     Summary no semver update required
    Finished [   0.703s] assertables

I'll try to think a bit on if there's a way out in a semver compatible manner.

[joelparkerhenderson]

Thank you. Yes you're exactly right, cargo semver-checks doesn't seem to catch this. What I'm seeing other macro-based projects do is have a large test suite of semantic observations for testing regressions.

Yes, but I'm not sure I understand the suggestion — the code continues to fail with that version.

[I'm sunsetting much of this comment because my info is no longer correct-- the fix is now ready]

A solution is to pin the exact version that works for you, meaning 9.5.0.

Thanks again for you help and report-- this is the first hiccup in the project overall so I'm hoping to have more Rust people helping with advice and code.

[shepmaster]

Are you able to edit your own source code?

Yes, but that goes against the point of semver — I should not need to change my source code when upgrading from one semantic version to another.

pin the exact version that works for you

Yes, that's more-or-less what I've done, but that's only a temporary bandaid while this issue is worked on.

---

However, you may not need to make this an incompatible change. For example, this Rust macro evaluates the argument once and doesn't require ownership:

use std::cell::Cell;

fn main() {
    // One evaluation
    let counter = Cell::new(0_u8);
    let get_value_and_increment = || {
        counter.set(counter.get() + 1);
        "container"
    };

    println!("{}", get_value_and_increment());

    assert_eq!(1, counter.get());

    // Ownership not required
    let s = String::from("container");
    println!("{}", s);
    assert!(!s.is_empty());
}

If we focus on how println is implemented (on Rust 1.56 because this has changed more recently):

println!("{}", String::new());

Expands to (cleaned a bit)

::std::io::_print(
    match match (&String::new(),) {
        (arg0,) => [::core::fmt::ArgumentV1::new(
            arg0,
            ::core::fmt::Display::fmt,
        )],
    } {
        ref args => unsafe { ::core::fmt::Arguments::new_v1(&["", "\n"], args) },
    },
)

You could use similar constructs in your macros:

use std::cell::Cell;

macro_rules! once_by_reference {
    ($v:expr) => {
        match $v {
            ref x => {
                assert!(x.contains("a"));
                assert!(x.contains("e"));
                assert!(x.contains("i"));
                assert!(x.contains("o"));
            }
        }
    };
}

fn main() {
    // One evaluation
    let counter = Cell::new(0_u8);
    let get_value_and_increment = || {
        counter.set(counter.get() + 1);
        "container"
    };

    once_by_reference!(get_value_and_increment());

    assert_eq!(1, counter.get());

    // Ownership not required
    let s = String::from("container");
    once_by_reference!(s);
    assert!(!s.is_empty());
}

Note that the println macro is a very old example, and so there might be simpler ways of writing something equivalent. For example, this also passes the quick test above:

macro_rules! once_by_reference {
    ($v:expr) => {
        let x = &$v;
        assert!(x.contains("a"));
        assert!(x.contains("e"));
        assert!(x.contains("i"));
        assert!(x.contains("o"));
    };
}

[joelparkerhenderson]

Oooh, that's nifty. First I've seen of match ref. You're right, that could solve things. I'll experiment with your approach today & tomorrow.

Yes, but that goes against the point of semver — I should not need to change my source code when upgrading from one semantic version to another.

Are you familiar with the emacs spacebar heater? :-) For this issue, there are multiple levels of semantics, meaning the higher level of compare two items, and the lower level of doing a comparison by a specific syntax that uses reference or value ownership.

Broadly I consider this to be a challenging point with Rust macros, in general, because as far as I know there's no way to write a macro that states that it won't alter its parameters and won't access a parameter more than once. For example pseudocode "macro(read once a:expr, read once b:expr) …".

[pdh11]

Another example of code which works with earlier 9.x versions but fails to compile with 9.6.1:

struct Foo {
    s: String,
}

fn fnord() {
    let s = Mutex::new(Foo { s: "fnord".to_string() } );
    let inner = s.lock().unwrap();
    assert_contains!(inner.s, "nor");
}

The error is "error[E0507]: cannot move out of dereference of MutexGuard<'_, Foo>". Here it's not a question of picking the unexpected one out of pass-by-reference and pass-by-value: here, pass-by-value is not available at all.

[joelparkerhenderson]

Thank you for the reports and for your patience too.

• I've published fixes in three upgrades: 9.5.7, 9.6.2 (which adds a couple of email address macros), 9.7.0 (which adds a couple of floating-point approximate equality macros).

• I yanked the problematic versions 9.5.3-9.5.6.

• For @pdh11 there's a new file ./tests/regressions/mutex.rs with a struct mutex test, to ensure it compiles and succeeds.

Can you do me a favor and give it try, and let me know here if this solves your issues?

[pdh11]

Can confirm, 9.7.0 works fine with the existing client code, I can remove the "<9.6" from my Cargo.toml now, thanks for sorting this out!

[pdh11]

Ah. Sorry to come back again, but my CI has found a new one:

#[derive(Default)]
struct Foo {
    seen: HashSet<String>
}

fn fnord(needle: &str) {
    let mut f = Foo::default();
    f.seen.insert("Fnord".to_string());
    assert_contains!(f.seen, needle);
}

This compiles and works with 9.4.0. but not 9.5.7 or 9.7.0 :(

If I change the assert_contains to use *needle, it works in 9.5.7 and 9.7.0 but not 9.4.0 :( :(

[joelparkerhenderson]

Thank you for the follow up. [I'm deleting the rest of this in favor of better newer information; please see below].

[shepmaster]

This may be the same issue as reported earlier? However, the regression now also appears in a smaller version range (9.5.2 works, 9.5.7 fails).

fn main() {
    let s = String::new();
    let n = String::new();
    assertables::assert_contains!(s, &n);
    assertables::assert_contains!(s, &n) ;
}

% cargo update -p assertables --precise 9.5.2
    Updating crates.io index
 Downgrading assertables v9.5.3 -> v9.5.2

% cargo check
    Checking assertables v9.5.2
    Checking repro v0.1.0 (/private/tmp/repro)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.23s

% cargo update -p assertables --precise 9.5.7
    Updating crates.io index
    Updating assertables v9.5.2 -> v9.5.7

% cargo check
    Checking repro v0.1.0 (/private/tmp/repro)
error[E0277]: the trait bound `&&String: Pattern` is not satisfied
    --> src/main.rs:4:5
     |
4    |     assertables::assert_contains!(s, &n);
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |     |
     |     the trait `Fn(char)` is not implemented for `String`
     |     required by a bound introduced by this call
     |
     = note: required for `&String` to implement `FnOnce(char)`
     = note: required for `&&String` to implement `Pattern`
note: required by a bound in `core::str::<impl str>::contains`
    --> /Users/shep/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/str/mod.rs:1320:24
     |
1320 |     pub fn contains<P: Pattern>(&self, pat: P) -> bool {
     |                        ^^^^^^^ required by this bound in `core::str::<impl str>::contains`
     = note: this error originates in the macro `$crate::assert_contains_as_result` which comes from the expansion of the macro `assertables::assert_contains` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider dereferencing here
    -->  /Users/shep/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/assertables-9.5.7/src/assert_contains/assert_contains.rs:56:39
     |
56   |                 if container.contains(*containee) {
     |                                       +

error[E0277]: the trait bound `&&String: Pattern` is not satisfied
    --> src/main.rs:5:5
     |
5    |     assertables::assert_contains!(s, &n) ;
     |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |     |
     |     the trait `Fn(char)` is not implemented for `String`
     |     required by a bound introduced by this call
     |
     = note: required for `&String` to implement `FnOnce(char)`
     = note: required for `&&String` to implement `Pattern`
note: required by a bound in `core::str::<impl str>::contains`
    --> /Users/shep/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/str/mod.rs:1320:24
     |
1320 |     pub fn contains<P: Pattern>(&self, pat: P) -> bool {
     |                        ^^^^^^^ required by this bound in `core::str::<impl str>::contains`
     = note: this error originates in the macro `$crate::assert_contains_as_result` which comes from the expansion of the macro `assertables::assert_contains` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider dereferencing here
    -->  /Users/shep/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/assertables-9.5.7/src/assert_contains/assert_contains.rs:56:39
     |
56   |                 if container.contains(*containee) {
     |                                       +

[shepmaster]

Note that I don't think there's anything wrong with cutting losses and releasing a semver-incompatible version. If 9.6 had been called 10.0, I wouldn't have blinked at having to make changes to my code. I only opened the issue because the version number indicated that there were no API changes required.

[joelparkerhenderson]

Thanks to your code sample @pdh11 I'm able to replicate the error message.

Here's what I'm learning, to the best of my knowledge today...

The problem is that the macro asserts_contains is simply calling the method contains which Rust uses differently in different types, and the HashSet type has some special handling using the trait Borrow.

HashSet.contains means "contains value" which can be "contains exact element" or "contains borrowed optimized type"

pub fn contains<Q: ?Sized>(&self, value: &Q) -> bool
where
    T: Borrow<Q>,
    Q: Hash + Eq,

Read about the trait Borrow and the special handling for HashSet here: https://doc.rust-lang.org/std/borrow/trait.Borrow.html

let haystack: HashSet<String> = [String::from("a")].into();
let needle: &str = "a";
assert!(haystack.contains(needle)); // This code works because it sends &str, the method gets Q str, which Rust knows how to converts into String.
assert_contains!(haystack, needle); // This code fails because the macro matches on &needle, thus receives &&str, thus the method gets &str, which Rust doesn't know how to borrow into String.

Here's what I'm thinking of for a sort-of solution sort-of tradeoff, below. Your suggestions are much appreciated.

The best compromise that I can suggest-- given what I'm learning so far-- is to make the macro use the same semantics and syntax as the Rust documentation.

Examples:

// Contains a substring
let string: String = String::from("alfa");
let pattern = "lf"
assert_contains!(string, pattern); // The macro calls string.contains(pattern) thus doesn't need ampersand.

// Contains a value with automatic borrow optimization
let haystack: HashSet<String> = [String::from("alfa")].into();
let needle = "alfa";
assert_contains!(haystack, needle); // The macro calls haystack.contains(needle) and needle is already a reference type &str thus no need for ampersand.

// Contains a value with exact type, no automatic borrow optimization
let haystack: HashSet<Foo> = …
let needle: Foo = …
assert_contains!(haystack, &needle); // The macro calls haystack.contains(&needle) and the ampersand is necessary because Range.contains method signature needs the value to be a reference.

This compromise means that some of the semantics-syntax must change, such as for assert_contains with a Range or a Vec:

let haystack = 1..3;
let needle = 2;
assert_contains!(haystack, &needle); // The macro calls Range.contains which requires a reference.

Thoughts? Better ideas? Alternatives?

[joelparkerhenderson]

FYI I'm publishing updates 9.5.8, 9.6.3., 9.7.1, with the fixes to the macro assert_contains to align it with the Rust standard libraries as above.

I appreciate all your help and patience! <3

I've updated the code, the tests, and the docs with examples of assert_contains working with

• A String without a borrow i.e. the second arg doesn't have an ampersand

• A Vec with a borrow i.e. the second arg needs an ampersand

• A Range with a borrow i.e. the second arg needs an ampersand

• A HashSet with two examples:

  • One example uses a &str. This doesn't need an ampersand (because the &str type already has one) which triggers the Rust automatic compiler Borrow optimization. This is the example that @pdh11 is reporting.

  • One example uses a String. This does need an ampersand (because String type doesn't already have one) which makes the method do a comparison without any Borrow optimization.