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

Made matches! more useful by adding mapping support #79188

Closed
wants to merge 1 commit into from

Conversation

zesterer
Copy link
Contributor

@zesterer zesterer commented Nov 19, 2020

This PR adds support for a trailing expression after the pattern in the matches! macro.

While matches! is extremely useful by itself, it does not have the ability to do anything other than perform a binary match.

A common pattern in Rust code looks something like the following:

enum Foo {
    A(i32),
    B(bool),
    C,
}

let maybe_a = if let Foo::A(n) = my_foo {
    Some(n)
} else {
    None
};

This is rather unwieldy, particularly when used inline. This PR instead allows the following:

let maybe_a = matches!(my_foo, Foo::A(n) => n);

Points in favour:

  • matches! becomes significantly more useful with this change
  • It is not a breaking change
  • The behaviour is a natural extension of the match syntax that this macro emulates

Points not in favour:

  • The name of the macro implies a binary operation. While Option is still binary in nature, it isn't quite so clear that a non-bool value may be produced

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 19, 2020
@Mark-Simulacrum
Copy link
Member

I think this is too far on the "concise code" path -- I agree that the match to pull out of the option or other enum can be a bit annoying, but I would rather see alternatives (e.g., let else statements) explored in the language itself rather than extensions to matches! -- I think this makes matches! too complex.

cc @withoutboats since I think you've had similar thoughts before, though I could be misremembering

@zesterer zesterer force-pushed the master branch 3 times, most recently from 02e4f82 to 6df807c Compare November 19, 2020 02:10
@est31
Copy link
Member

est31 commented Nov 19, 2020

An alternative that doesn't have the binary issue is the match_or macro from the postfix-macros crate. It's usable without postfix notation as well so can be added to the language today.

@joshtriplett
Copy link
Member

joshtriplett commented Nov 19, 2020

I think matches! was very much intended for use in boolean conditions: "does this expression match this pattern".

I also feel that the "implicit Option" approach here feels like an optimization for writing code at the expense of reading code.

Reading if matches!(expr, Pat | OtherPat), it's obvious at a glance what that does, even if you've never seen matches! before, as long as you've seen match before.

I don't think we should do this. A third-party crate could provide such a macro just as easily.

I also agree that let ... else would provide a better solution in many cases.

@zesterer
Copy link
Contributor Author

I see there is resistance to this change, so before closing I'll address the arguments made so far and put forward my best arguments.

I think matches! was very much intended for use in boolean conditions

It seems clear that we already train users to think of Option as being fundamentally binary in nature. Option::and and Option::or clearly point towards this and I frequently see Option used in the wild as a boolean with some extra ancillary data in the true case as opposed to simply being used as 'nullable' type.

feels like an optimization for writing code at the expense of reading code

I can understand that perspective, but I do not agree. This additional syntax is consistent with the syntax of match and its behaviour is self-evident for those that have used match (as with the if guard). Long, multi-line if lets have a habit of obscuring underlying logic (i.e: to extract data from an enum variant). Personally, I've encountered this problem a lot when reading and writing logic-heavy procedural generation code for Veloren. The otherwise concise codebase is littered with if lets that break up method chains and create garden-path patterns of flow control that mislead the reader about intent.

A third-party crate could provide such a macro just as easily

I feel like this option would be an unfortunate and unintuitive downgrade compared to support in std. matches already mirrors the existing syntax of match. I personally expected to be able to add some arm expression only to be surprised to find that this was not the case.

As an additional point: Rust has long had a bit of problem when it comes to the usability of enums. There exist additional types like std::mem::Discriminant that go a short way toward resolving those issues, but ultimately aren't as useful as they first appear (one cannot create a discriminant without first having an instance of that variant to create it from).

Third-party crates such as enum-iterator and strum exist to plug some of these holes but they often feel like ad-hoc fixes for gaps in the language and std.

While this PR does not specifically address enums, I hope it might act as a precursor to making working with unique enums (rather than nesting Result and Option as I have done frequently for the convenience of their inline methods rather than any real technical requirement) more ergonomic.

@pickfire
Copy link
Contributor

Shouldn't it be

let let maybe_a = my_foo;

What if matches!() did not match? Making it an Option seemed kinda magical, what if users can confirm an exact match, do they need to unwrap?

@zesterer
Copy link
Contributor Author

@pickfire Would a separate macro named maybe_match solve this problem in your mind?

@pickfire
Copy link
Contributor

@pickfire Would a separate macro named maybe_match solve this problem in your mind?

Not sure. Since both breaks the control flow such it behaves like unwrap and I still feel like something is off. I would rather use matches! even though it is more line as it is more explicit, or maybe match with a quick return.

@zesterer
Copy link
Contributor Author

@pickfire I'm not sure what you mean by 'breaks the flow control'. Conditionally executing inline code is a precedent already set by many features of Rust.

@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 7, 2020
@JohnCSimon
Copy link
Member

Ping from triage:
@zesterer can you please address the check failures?

@pickfire
Copy link
Contributor

pickfire commented Dec 7, 2020

Do we need a FCP for something like this?

@withoutboats
Copy link
Contributor

I'm inclined to agree with @Mark-Simulacrum and @joshtriplett: I think this is too "gadgety" for a std macro. We've tended to have a philosophy of giving the std macros as few "bells and whistles" as possible, and making them do one thing as straightforwardly as possible. matches! takes an expression and a pattern and evaluates to a bool; I prefer that to also giving it a variant with different syntax that evaluates to an Option. I think its fine to require a match or an if let for this case.

A similar case was the dbg! macro; in the original RFC, it had a much more complex API with many variations and options. The version that we have merged on the other hand is just about as simple as possible.

(I'll note that if you don't need to bind anything inside the pattern, very soon you will be able to do matches!(foo, Bar | Baz).then(|| quux) as a single-liner, though it doesn't support the example in the top post.)

Thanks for the PR @zesterer; if you want to this could be a great little package on crates.io (and if it sees widespread use, maybe that would shift the balance on including it in std). But I'm currently inclined against adding this API to std.

@rfcbot fcp close

@withoutboats withoutboats added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 7, 2020
@withoutboats
Copy link
Contributor

@rfcbot fcp close (no label)

@rfcbot
Copy link

rfcbot commented Dec 7, 2020

Team member @withoutboats has proposed to close 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-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Dec 7, 2020
@zesterer
Copy link
Contributor Author

zesterer commented Dec 7, 2020

@withoutboats Thanks for the response. Out of interest, would you be inclined to change your position on this if it were a separate macro named something like maybe_match?

@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 Dec 8, 2020
@rfcbot
Copy link

rfcbot commented Dec 8, 2020

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

@withoutboats
Copy link
Contributor

@withoutboats Thanks for the response. Out of interest, would you be inclined to change your position on this if it were a separate macro named something like maybe_match?

On the one hand, I would be more favorable to that, because it would be consistent with my understanding of std's style guidelines for macros. In other words, if we wanted to add this API, that's how I would expect us to add it. But I still wouldn't be inclined to add this functionality to std immediately. matches was added to std after spending years in a separate crate in crates.io, and was identified for inclusion because it was a dependency of a large number of other crates, relative to its very small size. If a library providing a macro like maybe_matches became similarly popular, that could make a good case for adding another macro to std.

@joshtriplett
Copy link
Member

@withoutboats Thank you.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 18, 2020
@rfcbot
Copy link

rfcbot commented Dec 18, 2020

The final comment period, with a disposition to close, 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.

@m-ou-se m-ou-se closed this Dec 18, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.