Skip to content

Made matches! more useful by adding mapping support - #79188

Closed
zesterer wants to merge 1 commit into
rust-lang:masterfrom
zesterer:master
Closed

zesterer wants to merge 1 commit into
rust-lang:masterfrom
zesterer:master

Conversation

@zesterer

@zesterer zesterer commented Nov 19, 2020 •

Copy link
Copy Markdown
Contributor

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

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
Copy Markdown
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

est31 commented Nov 19, 2020

Copy link
Copy Markdown
Member

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

joshtriplett commented Nov 19, 2020 •

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

@pickfire

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

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

@pickfire

pickfire commented Dec 7, 2020

Copy link
Copy Markdown
Contributor

Do we need a FCP for something like this?

@withoutboats

Copy link
Copy Markdown
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 [DEPRECATED; DO NOT USE] label Dec 7, 2020
@withoutboats

Copy link
Copy Markdown
Contributor

@rfcbot fcp close (no label)

@rfcbot

rfcbot commented Dec 7, 2020 •

Copy link
Copy Markdown

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

zesterer commented Dec 7, 2020

Copy link
Copy Markdown
Contributor Author

@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

rfcbot commented Dec 8, 2020

Copy link
Copy Markdown

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

@withoutboats

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

rfcbot commented Dec 18, 2020

Copy link
Copy Markdown

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
@joshtriplett joshtriplett mentioned this pull request Mar 23, 2021
6 of 7 tasks
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 [DEPRECATED; DO NOT USE]

Projects

None yet

Development

Successfully merging this pull request may close these issues.