Tracking Issue for assert_matches

[m-ou-se]

Feature gate: #![feature(assert_matches)]

This is a tracking issue for the assert_matches!() and debug_assert_matches!() macros.

Public API

// core

macro_rules! assert_matches { .. }
macro_rules! debug_assert_matches { .. }

Steps / History

• [x] Implementation: #82770
• [x] Temporarily move the macro into std::assert_matches::assert_matches to avoid breakage: https://github.com/rust-lang/rust/issues/82913
• [x] Final commenting period (FCP)
• [ ] Stabilization PR
  • [ ] Update the must_use message on is_none (per https://github.com/rust-lang/rust/pull/62431#pullrequestreview-258576936)
  • [ ] Mention this macro in the documentation of matches!()

Unresolved Questions

• [x] Add => expr syntax?
  • Nope, to confusing. (See discussion below.)

[m-ou-se]

Several implementations of assert_matches!() in the ecosystem also accept an => expr syntax, to do something with the matched values or even return something from the macro.

• https://docs.rs/assert_matches/1.5.0/assert_matches/macro.assert_matches.html
• https://docs.rs/cool_asserts/1.0.3/cool_asserts/macro.assert_matches.html

This is currently not supported by std::assert_matches!() as implemented in #82770.

Adding it would allow things like:

let thing = assert_matches!(thing, Some(x) => x);

and

assert_matches!(thing, Some((x, y)) => {
    assert_eq!(x.a, 10);
    assert_eq!(y.b(), 20);
});

[drahnr]

As a (almost) daily user of the assert_matches crate, I have to admit that without the additional $pat => $expr syntax, for most me it would be still impractical to use.

I.e. asserting a certain sequence of messages with a mocked context and assuring the messages have expected content (which by design may not impl {Partial,}Eq) I would usually use (super simplified):

type A = u8;
struct B(u8);
struct C(u8);
struct D(u8);

enum Msgs {
Specific { a: A, b: B, c: C, d: D},
// snip
}
assert_matches!(rx.next().unwrap(), Msgs::Specific { a, b: B(b), .. } => { assert_eq!(a, b); })
assert_matches!(rx.next().unwrap(), Msgs::Specific { b, c: C(c), .. } => { assert_eq!(b, c); })
..

so for this the extension with => is required to replace the crate assert_matches in practice from my experience.

[m-ou-se]

In cases like that, the if guard on the pattern also works, right?

assert_matches!(rx.next(), Some(Msgs::Specific { a, b: B(b), .. }) if a == b);

[drahnr]

While your approach is valid for simple cases, I disagree for the general use case. This is not very ergonomic as soon as there are various sub patterns, that have to be asserted against transformed struct member values or larger enums with multiple elements common for messaging enums.

assert_matches!(rx.next(), Some(Msgs::Specific { a, b: B(b), .. }) => {
  assert_eq!(a, b);
  let container = transform($container);
  if container.is_empty() {
       assert!(..);
  } else {
      assert!(..);
  }
});
..

It could still be expressed with boolean expression combinators using the current impl, yet that adds a lot visual complexity that imho should not exist in a test case.

Note that I am not saying the impl is bad or anything :) - I would very much like to see assert_matches! in std! This effort is much appreciated!

[m-ou-se]

@rust-lang/libs Ping for opinions.

Should assert_matches accept => { .. } blocks to allow for nested asserts (and other code)? The existing assert_matches macros in the ecosystem accept that. But I personally feel it makes things a bit too complicated and unclear.

[BurntSushi]

It's an interesting extension. At first it looked a bit odd to me, but after ruminating on it a bit, it does seem fairly natural to me.

In terms of moving forward:

1. Would adding this later be a backwards compatible addition?
2. How often is this extension used in practice? In theory, we could search the code on crates.io and count the ratio between uses of assert_matches! with and without this extension. If the ratio is very small, then maybe we land it without the extension if the answer to (1) is "yes."

[joshtriplett]

This makes me think of the proposal to add => syntax to matches!, which the libs team declined.

This seems very similar, and I feel like the arguments are the same. If we have assert_matches!(x, Some(Variant(a, b)) => a == b), similar use cases would motivate if matches!(x, Some(Variant(a, b)) => a == b) { ... }.

I personally feel that in both cases the => syntax adds enough complexity to make it worth spelling out a match or if let, instead. I don't really want to see an entire block of arbitrary code embedded inside an assert_matches!. I also think this will become easier to write when we finish if let chaining.

[m-ou-se]

assert_matches!(x, Some((a, b)) if a == b) would assert that x is a Some and a and b are equal, but assert_matches!(x, Some((a, b)) => a == b) would only assert that x is a Some, and return the value of a == b without asserting anything about it. The difference there is far too subtle for me.

We could require the => expr to be (), but I don't think that solves the problem of unclear semantics entirely.

[joshtriplett]

assert_matches!(x, Some((a, b)) if a == b) would assert that x is a Some and a and b are equal, but assert_matches!(x, Some((a, b)) => a == b) would only assert that x is a Some, and return the value of a == b without asserting anything about it. The difference there is far too subtle for me.

For me as well; that was not clear to me at all.

[BoxyUwU]

A separate macro that always requires a => expr arm i.e. unwrap_matches extract_matches might make it less ambiguous/more clear

[gilescope]

Well if we're agreed on leaving it as is then we can head towards stabilising it...

[gThorondorsen]

@drahnr in #82775 (comment)

While your approach is valid for simple cases, I disagree for the general use case. This is not very ergonomic as soon as there are various sub patterns, that have to be asserted against transformed struct member values or larger enums with multiple elements common for messaging enums.

assert_matches!(rx.next(), Some(Msgs::Specific { a, b: B(b), .. }) => {
  assert_eq!(a, b);
  let container = transform($container);
  if container.is_empty() {
       assert!(..);
  } else {
      assert!(..);
  }
});
..

It could still be expressed with boolean expression combinators using the current impl, yet that adds a lot visual complexity that imho should not exist in a test case.

Note that I am not saying the impl is bad or anything :) - I would very much like to see assert_matches! in std! This effort is much appreciated!

Your example can be rewritten to a match guard that always returns true except when it panics, like this.

-assert_matches!(rx.next(), Some(Msgs::Specific { a, b: B(b), .. }) => {
+assert_matches!(rx.next(), Some(Msgs::Specific { a, b: B(b), .. }) if ({
     assert_eq!(a, b);
     let container = transform($container);
     if container.is_empty() {
         assert!(/* … */);
     } else {
         assert!(/* … */);
     }
-});
+    true
+}));

Definitely non-obvious when you don't know about it, but hardly worse to write if you are adding to an existing test suite with the pattern everywhere.

[drahnr]

Re-reading the comments, I fully support the current impl and withdraw my previous concerns.

[fee1-dead]

I would prefer the macro to expand as let .. else { panic!() } and users would be able to use the bindings after the assertion.

Does this sound reasonable? Would feature(let_else) block the stabilization of this macro, or can we use allow_internal_unstable?

[jhpratt]

@fee1-dead that would be very confusing to me. An assertion should assert and nothing more imo

[steffahn]

I think there's a lot of value in having assert_matches!(...) be basically just an assert!(matches!(...)) with better error messages (and hence with an additional Debug bound). It makes the macro much easier to understand if those two expressions are essentially equivalent. Even if assert_matches and matches would both support the => expr syntax, this rule would be broken: At least following the existing (rejected) proposal for matches' support of => expr, the "equivalent" to assert_matches!(... => ...) would suddenly be matches!(... => ...).unwrap().

[m-ou-se]

Looks like we reached consensus on not having the => syntax.

@rfcbot merge

[rfcbot]

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

• [x] @Amanieu
• [x] @BurntSushi
• [x] @dtolnay
• [x] @joshtriplett
• [x] @m-ou-se
• [ ] @yaahc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[m-ou-se]

If we stabilize this as std::assert_matches!(), then we'll break a few crates. See https://github.com/rust-lang/rust/issues/82913.

chrysanthemum-0.0.1/src/verify.rs:                             use assert_matches::*;
error-context-0.1.2/src/lib.rs:                                use assert_matches::*;
port-finance-lending-0.1.0/tests/helpers/mod.rs:               use assert_matches::*;
rfc6381-codec-0.1.0/src/lib.rs:                                use assert_matches::*;
lethe-0.6.0/src/ui/args.rs:                                    use assert_matches::*;
lethe-0.6.0/src/actions/wipe.rs:                               use assert_matches::*;
rant-4.0.0-alpha.9/tests/runtime_tests.rs:                     use assert_matches::*;
kekbit-core-0.2.3/src/shm.rs:                                  use assert_matches::*;
port-finance-variable-rate-lending-0.1.2/tests/helpers/mod.rs: use assert_matches::*;

@rust-lang/libs-api How should we continue?

[m-ou-se]

cc'ing the macro resolution expert: @petrochenkov. maybe you have any ideas?

[yaahc]

If we stabilize this as std::assert_matches!(), then we'll break a few crates. See #82913.

I'm confused, how are these still being broken by assert_matches after https://github.com/rust-lang/rust/pull/87195?

[jo-so]

Would it be possible to allow => $x:ident to make it possible to unwrap with assert_matches!? I often have tests like:

let x = assert_matches!(fun(), Ok(x) => x);
assert_eq!(x.name, "Test");
assert_eq!(x.fld, 12);

Now I have to write this what's a little longish.

let x = fun();
assert_matches!(x, Ok(_));
let x = x.unwrap();
assert_eq!(x.name, "Test");
assert_eq!(x.fld, 12);

[sfackler]

What's the purpose of using assert_matches at all there?

[jhpratt]

That has been previously discussed and decided against.

[m-ou-se]

I'm confused, how are these still being broken by assert_matches after #87195?

I thought moving the macro into that module without re-exporting it in the prelude or crate root was only a temporary fix while this feature is unstable. I think once this is stable, it shouldn't require an extra import, to make sure it's available in the same way as assert_eq!() etc.

[jo-so]

Steven Fackler schrieb am Wed 16. Mar, 05:02 (-0700):

What's the purpose of using assert_matches at all there?

I want to assert that the return type matches Ok() and want to fail otherwise. If it's Ok(), I want to go on and run other checks.

[sfackler]

How would the behavior of your test change if you removed that line?

[jo-so]

How would the behavior of your test change if you removed that line?

It wouldn't compile, because the variable is not defined.