Tracking Issue for assert_matches

[m-ou-se]

View all comments

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

• Implementation: Add assert_matches macro. #82770
• Temporarily move the macro into std::assert_matches::assert_matches to avoid breakage: Is there a gentler way to land the assert_matches macro? #82913
• Final commenting period (FCP)
• Stabilization PR
  • Update the must_use message on is_none (per Add messages to Option's and Result's must_use annotation for is_* #62431 (review))
  • Mention this macro in the documentation of matches!()

Unresolved Questions

• Add => expr syntax?
  • Nope, too 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.