Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracking Issue for assert_matches #82775

Open
4 of 7 tasks
Tracked by #16
m-ou-se opened this issue Mar 4, 2021 · 84 comments
Open
4 of 7 tasks
Tracked by #16

Tracking Issue for assert_matches #82775

m-ou-se opened this issue Mar 4, 2021 · 84 comments
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@m-ou-se
Copy link
Member

m-ou-se commented Mar 4, 2021

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

Unresolved Questions

  • Add => expr syntax?
    • Nope, to confusing. (See discussion below.)
@m-ou-se m-ou-se added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Mar 4, 2021
@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 9, 2021

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.

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
Copy link
Contributor

drahnr commented Mar 11, 2021

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
Copy link
Member Author

m-ou-se commented Mar 11, 2021

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
Copy link
Contributor

drahnr commented Mar 23, 2021

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
Copy link
Member Author

m-ou-se commented Mar 23, 2021

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

m-ou-se commented Mar 24, 2021

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
Copy link
Member

joshtriplett commented Mar 24, 2021

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
Copy link
Member

BoxyUwU commented Jun 4, 2021

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

@gilescope
Copy link
Contributor

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

@gThorondorsen
Copy link

@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
Copy link
Contributor

drahnr commented Dec 1, 2021

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

@fee1-dead
Copy link
Member

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
Copy link
Member

jhpratt commented Dec 14, 2021

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

@steffahn
Copy link
Member

steffahn commented Jan 2, 2022

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
Copy link
Member Author

m-ou-se commented Jan 3, 2022

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

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jan 3, 2022

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

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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 3, 2022
@rfcbot
Copy link

rfcbot commented Jan 5, 2022

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

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 5, 2022
@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Jan 15, 2022
@ssokolow
Copy link

And to those expressing their disagreement with my viewpoints as dislikes, I kindly ask you to formulate your viewpoints and explain why you disagree with mine.

For me, my biggest concern is ensuring that, once the nature of pattern syntax has been learned, the number of "dialects" of it that must be memorized doesn't grow. (i.e. "refutable" vs. "irrefutable" is already enough in a language with so much other complexity that also needs to be learned.)

@Voultapher
Copy link
Contributor

I agree that having multiple pattern syntaxes would be a bad thing. I believe that, in this situation the choice of the word assert_matches in combination with the if allowed in the match arm syntax is a serious footgun, and having multiple syntaxes, can both be true at the same time. I suspect some of the issues can be alleviated by better documentation, something I'm open to contribute to. At the same time, it's not clear to me that reacting to "we have a problem" with refusing the problems's existence by pointing to problematic alternatives is a good course to follow.

@Allen-Webb
Copy link

refusing the problems's existence by pointing to problematic alternatives is a good course to follow.

I think you are misunderstanding what i am trying to say. I am saying if you think the if syntax is confusing, we should fix it in the general sense, not for assert_matches. I don't see this as the right place to try to address changes to the pattern syntax.

Maybe I am misunderstanding you if you are talking about different comments.

@Voultapher
Copy link
Contributor

Voultapher commented Jan 31, 2024

To me I'm mostly fine with pattern syntax in existing places, though it's difficult to asses ones own biases and knowledge curse. For me assert_matches presents a unique form of confusion risk. That might sound surprising, given in the other places like match I'm happy with the syntax. The same rules can lead to desirable and undesirable outcomes in different places. I'm not sure how to fix this, the design space is large. For me the first step would be acknowledging the potential problem and doing some experiments that check my assumption of how confusing this really is. Earlier I've asked a college and they were confused by the syntax, as was I. That's sample size 2, we can do better.

Also there is precedence for not allowing the full pattern syntax everywhere, for example if let Ok(x) if x.len() < 10 = x { ... } is not valid.

@tmccombs
Copy link
Contributor

Ok, so they do different things, so it's not a clear comparison.

assert_matches!(c, Ok(x) | Err(x) if x.len() < 100);

is basically equivalent to :

match {
  Ok(x) | Err(x) if x.len() < 100 => {}
  _ => panic!("a meaningful error message");
}

If the if guard isn't allowed in assert_matches or uses a different syntax it begs the question of why it isn't consistent with the equivalent match expression.

It's much more clear what the if is binding and applying to.

I don't think this is any less of a problem in the match example above. Although, in my opinion is is pretty clear that | binds more tightly than if.

And to those expressing their disagreement with my viewpoints as dislikes, I kindly ask you to formulate your viewpoints and explain why you disagree with mine.

Well, I've mostly just agreed with others have said and haven't had a lot else to add to it. Or time to formulate a more thorough response.

I do have a couple of ideas that might help with this though:

  1. Have a lint that suggests adding parenthesis around multiple patterns separated by | if there is an if guard (I'm actually not entirely sure if that is valid syntax today, so maybe allow that if it isn't currently allowed). So for example suggest that you use (Ok(x) | Err(x)) if x.len() < 100 instead of OK(x) | Err(x) if x.len().
  2. If we had postfix macros, this might be a good candidate: IMO c.assert_matches!(Ok(x) | Err(x) if x.len() < 100) is a little more clear that the if is part of the match pattern instead of saying only match if a condition is true.
  3. Allow && instead of if. If we were to stay consistent this would need to be allowed in match expressions as well, which would require a separate RFC. But such a change could be added later.

@Voultapher
Copy link
Contributor

I don't think this is any less of a problem in the match example above. Although, in my opinion is is pretty clear that | binds more tightly than if.

I should clarify, I meant it's more clear it applies to the branch, as highlighted by the separate line and something following it, the branch body. In assert_matches it is the end of the expression, making that IMO less clear.

@Allen-Webb
Copy link

Allen-Webb commented Jan 31, 2024

Have a lint that suggests adding parenthesis around multiple patterns separated by | if there is an if guard (I'm actually not entirely sure if that is valid syntax today, so maybe allow that if it isn't currently allowed). So for example suggest that you use (Ok(x) | Err(x)) if x.len() < 100 instead of OK(x) | Err(x) if x.len().

Something like this could apply in all cases patterns are used in macro_rules!, but it isn't valid syntax today. It would also be a bit weird to allow it in that case and not allow cases like Ok(x) | (Err(x) if x.len() < 100)

@nnethercote
Copy link
Contributor

FWIW, assert!(matches!(c, Ok(x) | Err(x) if x.len() < 100)); is already valid.

Also, about this running example: binding a variable in multiple arms and then using it in a condition is pretty unusual.

If you don't like the fact that Ok(x) | Err(x) if x.len() < 100 is a valid pattern, I can understand that. It's arguably surprising. But I don't buy the argument that assert_matches! is particularly different to match or anywhere else a pattern can appear.

@nsunderland1
Copy link
Contributor

I think it's also worth thinking about what the worst case is. If I write this, expecting the if to only apply to the Err case:

assert_matches!(c, Ok(x) | Err(x) if x.len() < 100);

The result is a strictly stronger assertion than I intended to write. There are cases where that might be undesirable, but it's nowhere near as bad as an unintentionally weak assertion. It will never let invariant violations slip through, the worst case is an unneeded panic.

@Voultapher
Copy link
Contributor

Voultapher commented Feb 1, 2024

But I don't buy the argument that assert_matches! is particularly different to match or anywhere else a pattern can appear.

I think the difference comes down to english grammar. "match if" makes sense, "assert that pattern if condition" also makes sense, however that's not the semantics. It's either "panic if not pattern and or not condition" or "assert that pattern and condition". Am I really the only one here that finds that problematic?

Might be a language thing, my native language is German, so maybe this aspects affects me more?

@fee1-dead
Copy link
Member

I think it's also worth thinking about what the worst case is. If I write this, expecting the if to only apply to the Err case:

assert_matches!(c, Ok(x) | Err(x) if x.len() < 100);

The result is a strictly stronger assertion than I intended to write.

Perhaps I can assume that if you only wanted the pattern to apply to the Err case, you would probably write:

assert_matches!(c, Ok(_) | Err(x) if x.len() < 100);

and get an error:

error[E0408]: variable `x` is not bound in all patterns
 --> src/main.rs:4:24
  |
4 |     assert_matches!(c, Ok(_) | Err(x) if x.len() < 100);
  |                        ^^^^^       - variable not in all patterns
  |                        |
  |                        pattern doesn't bind `x`

which could give you some clue that you can't really do this. (if this confusion is common, then it would be a diagnostics issue, and we can suggest removing the Ok(_) | altogether)

For people who aren't familiar with matches!, though, the following statement would probably be equally confusing:

if matches!(c, Ok(x) | Err(x) if x.len() < 100) {
        
}

and if you tried to read it out loud token by token it would sound like "if matches c Ok(x) or Err(x) if the length of x is smaller than 100".

I think if we were to stabilize assert_matches! the syntax should be equivalent to assert!(matches!. An alternative syntax like matches!(Ok(x) | Err(x) && x.len() < 100) could be equally (or actually more, IMO) confusing as to why it doesn't follow the same if guard syntax on match (it's called matches!) (but follows if let chain syntax but without the = c after the pattern)

@Voultapher
Copy link
Contributor

Voultapher commented Feb 1, 2024

I agree that using something like matches!(Ok(x) | Err(x) && x.len() < 100) causes more issues than it helps. We should definitely not invent a new pattern syntax. Maybe there are options that are less confusing than the existing proposal, but don't require a new pattern syntax?

  1. Swap the parameter order around. assert_matches!(Ok(x) | Err(x) if x.len() < 100, c); that might help make it more clear that the if is part of the pattern and not some assert_matches specific thing.
  2. Expand the existing assert! macro, maybe something like assert!(match c => Ok(x) | Err(x) if x.len() < 100); or assert!(match c { Ok(x) | Err(x) if x.len() < 100 });. That would use practically no new concepts. With the exception of the omitted curly braces, this would be even closer to the existing match syntax. No idea if that is technically possible, and I suspect it would be bound to a new Rust edition. grep.app suggests that ~0.1% of assert usages would collide with this.
  3. Disallow if guards in assert_matches mimicking constructs like if let, where if guards are also not allowed. This would limit confusion risk at the cost of consistency and applicability. grep.app suggests this would affect ~5.7% of the use cases.
  4. Similar to 3. disallow the if guard, but allow a branch body e.g. assert_matches!(c, Ok(x) | Err(x) => assert!(x.len() < 100));, this would diverge from matches! which is probably a bad idea.
  5. Similar to 2 but use a new assert name assert_pattern!(match x { Ok(x) | Err(x) if x.len() < 100 });.
  6. Have an abstraction that allows to capture the semantics of a single pattern as an object, that can be used and passed around. E.g. matcher!(Ok(x) | Err(x) if x.len() < 100).assert(c); or pattern!(Ok(x) | Err(x) if x.len() < 100).assert_matches(c);. This would probably require a significant amount of changes. That said I've come across a couple of places in Rust where abstracting over the specific match logic would have been really nice.
  7. We use a time machine to replace if with where in the pattern syntax. And also make binding more clear somehow. assert_matches!(c, (Ok(x) | Err(x)) where x.len() < 100);

List of all suggestions:

_. assert_matches!(c, Ok(x) | Err(x) if x.len() < 100);
1. assert_matches!(Ok(x) | Err(x) if x.len() < 100, c);
2. assert!(match c => Ok(x) | Err(x) if x.len() < 100);
2. assert!(match c { Ok(x) | Err(x) if x.len() < 100 });
3. Not express-able
4. assert_matches!(c, Ok(x) | Err(x) => assert!(x.len() < 100));
5. assert_pattern!(match x { Ok(x) | Err(x) if x.len() < 100 });
6. matcher!(Ok(x) | Err(x) if x.len() < 100).assert(c);
6. pattern!(Ok(x) | Err(x) if x.len() < 100).assert_matches(c);
7. assert_matches!(c, (Ok(x) | Err(x)) where x.len() < 100);

That's after roughly an hour of exploring the design space. I believe there are more options.

Of course there is the elephant in the room that any deviation from the assert_matches crate will make adoption more work. However if the criteria is purely based on that, what point is there in discussing design?

@tmccombs
Copy link
Contributor

tmccombs commented Feb 1, 2024

Another option:

assert_matches!( c { Ok(x) | Err(x) if x.len() < 100 })

And for consistentency also allow the following syntax with the matches! macro:

matches!( c { Ok(x) | Err(x) if len(x) < 100 })

I don't like 1 because then the order is the opposite of the existing matches! macro. Similarly I'm unhappy with 3 or 4, because it is substantially different from how matches! works.

@nnethercote nnethercote added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Feb 1, 2024
@nnethercote
Copy link
Contributor

I'm going to add some meta-commentary because I think this conversation has gotten to a point that's not productive.

  • There have been 20 comments in the past couple of days.
  • Nine of them by @Voultapher, who strongly opposes the syntax.
  • Eleven of them by seven other people, none of whom appear to oppose the syntax.

Ultimately it's up to the libs-api team to decide this. I'm sure they will consider the newest arguments put forth.

@nnethercote nnethercote removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Feb 1, 2024
@Firstyear
Copy link
Contributor

I think more misunderstandings leading to incorrect usage would occur if the syntax deviated from match

This comment pretty much nails it. assert_matches!() should be equivalent to "assert!(matches!( .. ))" else people will be surprised if it acts differently.

@8573
Copy link

8573 commented Feb 2, 2024

My two cents are that the precedence of | vs if in patterns is confusing (whether in assert_matches, matches, or match) and that the solution would be not to disallow if in one use of patterns but to use parentheses to clarify the precedence, with an optional Clippy lint to require the parentheses.

@pravic
Copy link
Contributor

pravic commented Feb 2, 2024

Why are you, guys, arguing in the assert_matches topic if the syntax is exactly the same as in matches! macro? You should have had this discussion during matches! stabilization - here it's too late because assert_matches is simply assert!(matches!(expr), msg).

P.S. And if matches! has different rules, assert_matches! should be exactly the same - otherwise, it's a bad architectural choice.

@Voultapher
Copy link
Contributor

Voultapher commented Feb 2, 2024

Why are you, guys, arguing in the assert_matches topic if the syntax is exactly the same as in matches! macro?

Please carefully read what I've previously written. I think there is a meaningful difference. Here are the two core sources of confusion:

  1. What part of the pattern does the if bin to? Ok(x) | Err(x) if x.len(), the Err or both? That ship as you said has sailed a long time ago, even long before the matches! macro.
  2. I think the intuitive reading of assert_matches!(c, Ok(x) if x.len() < 100) is: "assert that it matches the pattern if the condition is met". However that's not the semantics. It's actually "assert that it matches the pattern and the condition is met". I argue that while matches! is affected by this to some degree, it's significantly worse for assert_matches! because of the logic inversion. I've made that point a couple times now, for example here.

@nnethercote
Copy link
Contributor

Everyone, please stop, the same points are just being rehashed.

@Rigidity
Copy link

Rigidity commented Mar 5, 2024

3 years and no stabilization for something as simple as this? 😦

@rust-lang rust-lang locked and limited conversation to collaborators Mar 5, 2024
@rust-lang rust-lang unlocked this conversation Apr 15, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Apr 15, 2024

Tracking issues have a tendency to become unmanageable. Please open a dedicated new issue and link to this issue for absolutely any topics you want to discuss or have questions about. See rust-lang/compiler-team#739 for details and discussions on this prospective policy.

@YarekTyshchenko
Copy link

YarekTyshchenko commented Jul 8, 2024

FWIW, assert!(matches!(c, Ok(x) | Err(x) if x.len() < 100)); is already valid.

I came looking for assert_eq! equivalent but with matches! funcionality: Assert eq prints left and right side when there's an inequality, and is very useful is tests. the functionaly equivalent assert!(matches!(...)); does not print a nice error message :(

Come to think of it, maybe what I want would be better named as assert_eq_matches!(...) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.