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

Add the `matches!( $expr, $pat ) -> bool` macro #65479

Merged
merged 4 commits into from Oct 24, 2019

Conversation

@SimonSapin
Copy link
Contributor

SimonSapin commented Oct 16, 2019

Motivation

This macro is:

  • General-purpose (not domain-specific)
  • Simple (the implementation is short)
  • Very popular on crates.io (currently 37th in all-time downloads)
  • The two previous points combined make it number one in left-pad index score

As such, I feel it is a good candidate for inclusion in the standard library.

In fact I already felt that way five years ago: #14685 (Although the proof of popularity was not as strong at the time.)

API

Back then, the main concern was that this macro may not be quite universally-enough useful to belong in the prelude.

Therefore, this PR adds the macro such that using it requires one of:

use core::macros::matches;
use std::macros::matches;

Like arms of a match expression, the macro supports multiple patterns separated by | and optionally followed by if and a guard expression:

let foo = 'f';
assert!(matches!(foo, 'A'..='Z' | 'a'..='z'));

let bar = Some(4);
assert!(matches!(bar, Some(x) if x > 2));

Implementation constraints

A combination of reasons make it tricky for a standard library macro not to be in the prelude.

Currently, all public macro_rules macros in the standard library macros end up “in the prelude” of every crate not through use std::prelude::v1::*; like for other kinds of items, but through #[macro_use] on extern crate std;. (Both are injected by src/libsyntax_ext/standard_library_imports.rs.)

#[macro_use] seems to import every macro that is available at the top-level of a crate, even if through a pub use re-export.

Therefore, for matches! not to be in the prelude, we need it to be inside of a module rather than at the root of core or std.

However, the only way to make a macro_rules macro public outside of the crate where it is defined appears to be #[macro_export]. This exports the macro at the root of the crate regardless of which module defines it. See macro scoping in the reference.

Therefore, the macro needs to be defined in a crate that is not core or std.

Implementation

This PR adds a new matches_macro crate as a private implementation detail of the standard library. This crate is #![no_core] so that libcore can depend on it. It contains a macro_rules definition with #[macro_export].

libcore and libstd each have a new public macros module that contains a pub use re-export of the macro. Both the module and the macro are unstable, for now.

The existing private macros modules are renamed prelude_macros, though their respective source remains in macros.rs files.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Oct 16, 2019

r? @alexcrichton

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

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Oct 16, 2019

@rust-lang/libs, what do you think?

If having this macro in the prelude sounds acceptable after all, this PR can be much simpler.

src/libmatches_macro/lib.rs Outdated Show resolved Hide resolved
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Oct 16, 2019

Back then, the main concern was that this macro may not be quite universally-enough useful to belong in the prelude.

I think this concern holds true even more so today. In particular, #53667 (which is being worked on) would allow us to write e.g. is!(let $pat = $expr && ...) and there is desire to add either let $pat = $expr as a bool-typed expression or $expr is $pat. Therefore, I think this macro should continue to live outside the standard library because it replicates language features (and so we'll have churn wrt. the teaching material wrt. the macro and the eventual replacements).

@@ -0,0 +1,8 @@
error: cannot find macro `matches` in this scope

This comment has been minimized.

Copy link
@Centril

Centril Oct 16, 2019

Member

Please move all the related tests into their own directory.

This comment has been minimized.

Copy link
@SimonSapin

SimonSapin Oct 17, 2019

Author Contributor

What directory? I looked for a prelude directory and didn’t find one. Or do you mean one just for this macro?

This comment has been minimized.

Copy link
@Centril

Centril Oct 17, 2019

Member

Yeah just one for the macro to keep things organised.

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Oct 17, 2019

Is there an easy way to get data on matches! vs assert_matches!? The latter feels like it doesn't have the problems that centril mentions, as we have assert_eq!(x, y) despite assert!(x == y) existing.

(And, anecdotally, the times I see it mentioned in chat are most often for "I'm trying to test something, but I can't use == Ok(3) because the error type isn't PartialEq" or similar.)

@petrochenkov petrochenkov self-assigned this Oct 17, 2019
@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Oct 17, 2019

Is there an easy way to get data on matches! vs assert_matches!?

I can’t think of one. A not-so-easy way might be to grep the source code of every crate in https://crates.io/crates/matches/reverse_dependencies

My anecdotal evidence is that I’ve used matches! in a number of crates (including several in the first page of the list above) but I haven’t used assert_matches! at all.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Oct 17, 2019

I think this concern holds true even more so today.

I feel this isn’t quite the same concern (that was about the bar being very high for adding anything to the prelude), but the rest of your comment makes a fair point.

A similar point was made in #14685 with RFC 160’s if let. While if let does cover some use cases that would otherwise be a good fit for matches!, there are plenty of other cases where matches! is still useful.

I feel the same about RFC 2497’s if let chains. It does significantly extend the set of cases where matches! becomes unnecessary, but does not replace it completely.

would allow us to write e.g. is!(let $pat = $expr && ...)

Can you say more about what this is! macro would do? In particular, what makes it different from matches!($expr, $pat if ...) other than syntax (which could be changed in this PR)?

there is desire to add either let $pat = $expr as a bool-typed expression or $expr is $pat

Is there any concrete proposal for those yet?

this macro […] replicates language features (and so we'll have churn wrt. the teaching material wrt. the macro and the eventual replacements).

While this churn is a valid concern, I think “replicates” is not accurate. This macro supports many cases that if let doesn’t, even with RFC 2497. As to hypothetical future language proposals I’d prefer to limit the importance we give them when making decisions:

  • We also make Rust for people who use it today (and in 12 weeks), not just for people who will use it in 4 years
  • A proposal might not make it because of serious issues/limitations. (For example: What does it mean for the scope of bindings for let statements to start allowing refutable patterns? Can we make is a keyword?)
  • Even if a proposal is good in isolation, infinite growth of the language (and of syntax in particular) may not be desirable. (See “complexity budget”)
@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Oct 17, 2019

Even though I've personally never used matches in my life, I think we should add this to std:

  1. It is a very small API addition (one macro, without any fancy options, only a few lines of code).
  2. It is a very popular API (37th most popular crate despite being so easy to write yourself).
  3. It is a very stable API (the matches crate has not had a breaking change in the 5 years it has existed; technically this is more stable than std :p).

This sounds like an ideal candidate for std to me.

However, I would go further and say that we should revisit the decision made about the adding it to the prelude in 2014. nrc's comment at the time doesn't say anything about this macro being particularly unworthy of being in the prelude, just about the fact that adding macros to std means adding them to the prelude. But we've added macros to std and the prelude since then, and this would be the first macro we add that doesn't get added to the prelude.

This would be the first macro we've added to std that isn't in the prelude. I think we should just add this like every other macro and accept that being in the prelude is how macros work, rather than creating this extra crate hack to get around that. Alternatively:

  1. Can we find a reason the matches macro in particular should not go in the prelude?
  2. Does anyone on libs think we should stop adding macros to the prelude as a matter of policy, and start adding them to std from now on using this separate crate hack Simon has shown?
@Centril

This comment has been minimized.

Copy link
Member

Centril commented Oct 17, 2019

I feel the same about RFC 2497’s if let chains. It does significantly extend the set of cases where matches! becomes unnecessary, but does not replace it completely.

Yes, but if let $pat = $expr is an expression typed at bool then matches! is entirely unnecessary and then some (e.g. let Some(x) = foo() && let Ok(y) = bar(x) && ...).

Can you say more about what this is! macro would do? In particular, what makes it different from matches!($expr, $pat if ...) other than syntax (which could be changed in this PR)?

It would be defined as macro_rules! is { ($e:expr) => { if $e { true } else { false } } } and would, combined with let_chains-chains also subsume matches!.

Is there any concrete proposal for those yet?

Not yet; we're taking an incremental approach with let_chains.

We also make Rust for people who use it today (and in 12 weeks), not just for people who will use it in 4 years

The question here is not whether matches! should be implementable or not. It already exists. Rather, the question is whether we should elevate it to the standard library. What would be the purpose of doing so? Well I can think of:

  • It's there out of the box / more conveniently there (same argument as for dbg!'s inclusion).
  • Some people do not want to take on mini-crate dependencies for e.g. trust issues.
  • It would become the idiomatic way to do things. This one in particular is in my view a problem (especially if it is in the prelude, which answers @withoutboats's question) as the language improves and matches! is subsumed completely by it.

(For example: What does it mean for the scope of bindings for let statements to start allowing refutable patterns?

Well there are options to chose from here which include e.g. making the scope short or long (and the latter is forward compatible with the latter).

Can we make is a keyword?)

The syntax has previously been implemented experimentally in rust-lang/rfcs#2260 (comment) and is does not need to be a keyword because it is a binary operator.

Even if a proposal is good in isolation, infinite growth of the language (and of syntax in particular) may not be desirable. (See “complexity budget”)

Implementing the syntax of #53667 in a sane way already does so for let-as-expression as well (in fact it's already implemented on nightly). Instead, semantic restrictions (at the level of HIR lowering) are currently imposed on where let may occur. As for the complexity of lifting those semantic restrictions, one may argue that a) lifting them reduces overall user-facing complexity and b) matches! introduces a similar amount of complexity as far as the user is concerned (in fact possibly more complexity because it is a macro).

It is a very popular API (37th most popular crate despite being so easy to write yourself).

In this case, I think this exposes the fact that there's a missing language feature.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Oct 17, 2019

Can you say more about what this is! macro would do? In particular, what makes it different from matches!($expr, $pat if ...) other than syntax (which could be changed in this PR)?

It would be defined as macro_rules! is { ($e:expr) => { if $e { true } else { false } } } and would, combined with let_chains-chains also subsume matches!.

My understanding of RFC 2497 is that if let chains are a special case in the grammar of if, not expressions, and so would not be matched by $e: expr. So the definition would have to be something like:

macro_rules! is {
    (let $p: pat = $e:expr $( && $guard: expr  )*) => {…} 
}

Or something recursive, to support additional let patterns instead of just boolean guards. Modulo limitations imposed by macro future-proofing. At that point, the same macro can be implemented today without RFC 2497 (with a more verbose expansion) and would be pretty much equivalent to this PR.

@Centril

This comment has been minimized.

Copy link
Member

Centril commented Oct 17, 2019

My understanding of RFC 2497 is that if let chains are a special case in the grammar of if, not expressions, and so would not be matched by $e: expr. So the definition would have to be something like:

That's what the RFC specifies but it isn't really implementable in a good way so the actual change on nightly was instead to just add let $pat = $expr to the expression grammar (tho not immediately in the expr macro fragment -- right now you need parens around it) since that is much cleaner to specify and implement.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 17, 2019

It's possible to achieve the same observable behavior (public, but not in prelude) without introducing new crates and modules, by using a macro item. (libcore declarations for some built-in macros already do that.)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Oct 17, 2019

I don't know whether it's reasonable to add a library solution for this for the time until a language solution is ready, but I find the if guard part especially questionable because it's kind of a hack existing only because chaining with && is not supported.

@petrochenkov petrochenkov removed their assignment Oct 17, 2019
@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Oct 17, 2019

Like the macro’s name, the $( if $guard: expr )? part of the macro mirrors the syntax of match expressions.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 18, 2019

I don't really have much of an opinion on whether or not to add this macro, I'm fine either way. On the topic of organization, though, and responding to @withoutboats's comment above I agree that adding this shouldn't require build system hacks and weird crates. I would personally say that if we're going to add this we should do one of two things:

  • Either add it to the prelude
  • Or don't add it to the prelude but require usage to be std::matches!(...)

For consistency with all other exported macros I would probably go with adding it to the prelude. If we're adding a macro to libcore/libstd and we don't think we should add it to the prelude, then we probably shouldn't be adding the macro to libcore/libstd.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Oct 18, 2019

Adding to the prelude would be my preference, and indeed would make this PR much simpler.

I submitted it this way based on earlier conversations with libs team members saying that a non-prelude macro would be more likely to be accepted.

Or don't add it to the prelude but require usage to be std::matches!(...)

This is what I tried to do, but see PR description. Not having the std::macros module would require more invasive changes to the language (in semantics of #[macro_use]) or the compiler (in how prelude macros are injected).

At the moment, being importable from the root of std is what makes a macro be in the prelude.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 22, 2019

☔️ The latest upstream changes (presumably #65671) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin

This comment has been minimized.

Copy link
Contributor Author

SimonSapin commented Oct 22, 2019

I’ll rebase after we decide on whether we want this in the prelude.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Oct 22, 2019

@rfcbot ask libs "Are we alright adding matches to the prelude?"

To make progress, could people check the box if they don't object to adding a matches macro to the prelude, and leave a comment if they do object?

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Oct 22, 2019

Team member @withoutboats has asked teams: T-libs, for consensus on:

"Are we alright adding matches to the prelude?"

SimonSapin added 2 commits Oct 16, 2019
# Motivation

This macro is:

* General-purpose (not domain-specific)
* Simple (the implementation is short)
* Very popular [on crates.io](https://crates.io/crates/matches)
  (currently 37th in all-time downloads)
* The two previous points combined make it number one in
  [left-pad index](https://twitter.com/bascule/status/1184523027888988160)
  score

As such, I feel it is a good candidate for inclusion in the standard library.

In fact I already felt that way five years ago:
#14685
(Although the proof of popularity was not as strong at the time.)

Back then, the main concern was that this macro may not be quite
universally-enough useful to belong in the prelude.

# API

Therefore, this PR adds the macro such that using it requires one of:

```
use core::macros::matches;
use std::macros::matches;
```

Like arms of a `match` expression,
the macro supports multiple patterns separated by `|`
and optionally followed by `if` and a guard expression:

```
let foo = 'f';
assert!(matches!(foo, 'A'..='Z' | 'a'..='z'));

let bar = Some(4);
assert!(matches!(bar, Some(x) if x > 2));
```

# Implementation constraints

A combination of reasons make it tricky
for a standard library macro not to be in the prelude.

Currently, all public `macro_rules` macros in the standard library macros
end up “in the prelude” of every crate not through `use std::prelude::v1::*;`
like for other kinds of items,
but through `#[macro_use]` on `extern crate std;`.
(Both are injected by `src/libsyntax_ext/standard_library_imports.rs`.)

`#[macro_use]` seems to import every macro that is available
at the top-level of a crate, even if through a `pub use` re-export.

Therefore, for `matches!` not to be in the prelude, we need it to be
inside of a module rather than at the root of `core` or `std`.

However, the only way to make a `macro_rules` macro public
outside of the crate where it is defined
appears to be `#[macro_export]`.
This exports the macro at the root of the crate
regardless of which module defines it.
See [macro scoping](
https://doc.rust-lang.org/reference/macros-by-example.html#scoping-exporting-and-importing)
in the reference.

Therefore, the macro needs to be defined in a crate
that is not `core` or `std`.

# Implementation

This PR adds a new `matches_macro` crate as a private implementation detail
of the standard library.
This crate is `#![no_core]` so that libcore can depend on it.
It contains a `macro_rules` definition with `#[macro_export]`.

libcore and libstd each have a new public `macros` module
that contains a `pub use` re-export of the macro.
Both the module and the macro are unstable, for now.

The existing private `macros` modules are renamed `prelude_macros`,
though their respective source remains in `macros.rs` files.
@SimonSapin SimonSapin force-pushed the SimonSapin:matches branch from dfcb9a8 to f7ebe19 Oct 23, 2019
@SimonSapin SimonSapin changed the title Add `core::macros::matches!( $expr, $pat ) -> bool` Add the `matches!( $expr, $pat ) -> bool` macro Oct 23, 2019
@SimonSapin SimonSapin force-pushed the SimonSapin:matches branch from 4ed9e5c to e76a184 Oct 23, 2019
@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Oct 23, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 23, 2019

📌 Commit e76a184 has been approved by alexcrichton

Centril added a commit to Centril/rust that referenced this pull request Oct 23, 2019
Add the `matches!( $expr, $pat ) -> bool` macro

# Motivation

This macro is:

* General-purpose (not domain-specific)
* Simple (the implementation is short)
* Very popular [on crates.io](https://crates.io/crates/matches) (currently 37th in all-time downloads)
* The two previous points combined make it number one in [left-pad index](https://twitter.com/bascule/status/1184523027888988160) score

As such, I feel it is a good candidate for inclusion in the standard library.

In fact I already felt that way five years ago: rust-lang#14685 (Although the proof of popularity was not as strong at the time.)

# API

<details>
<del>

Back then, the main concern was that this macro may not be quite universally-enough useful to belong in the prelude.

Therefore, this PR adds the macro such that using it requires one of:

```rust
use core::macros::matches;
use std::macros::matches;
```

</del>
</details>

Like arms of a `match` expression, the macro supports multiple patterns separated by `|` and optionally followed by `if` and a guard expression:

```rust
let foo = 'f';
assert!(matches!(foo, 'A'..='Z' | 'a'..='z'));

let bar = Some(4);
assert!(matches!(bar, Some(x) if x > 2));
```

<details>
<del>

# Implementation constraints

A combination of reasons make it tricky for a standard library macro not to be in the prelude.

Currently, all public `macro_rules` macros in the standard library macros end up “in the prelude” of every crate not through `use std::prelude::v1::*;` like for other kinds of items, but through `#[macro_use]` on `extern crate std;`. (Both are injected by `src/libsyntax_ext/standard_library_imports.rs`.)

`#[macro_use]` seems to import every macro that is available at the top-level of a crate, even if through a `pub use` re-export.

Therefore, for `matches!` not to be in the prelude, we need it to be inside of a module rather than at the root of `core` or `std`.

However, the only way to make a `macro_rules` macro public outside of the crate where it is defined appears to be `#[macro_export]`. This exports the macro at the root of the crate regardless of which module defines it. See [macro scoping](https://doc.rust-lang.org/reference/macros-by-example.html#scoping-exporting-and-importing) in the reference.

Therefore, the macro needs to be defined in a crate that is not `core` or `std`.

# Implementation

This PR adds a new `matches_macro` crate as a private implementation detail of the standard library. This crate is `#![no_core]` so that libcore can depend on it. It contains a `macro_rules` definition with `#[macro_export]`.

libcore and libstd each have a new public `macros` module that contains a `pub use` re-export of the macro. Both the module and the macro are unstable, for now.

The existing private `macros` modules are renamed `prelude_macros`, though their respective source remains in `macros.rs` files.

</del>
</details>
bors added a commit that referenced this pull request Oct 23, 2019
Rollup of 12 pull requests

Successful merges:

 - #64178 (More Clippy fixes for alloc, core and std)
 - #65144 (Add Cow::is_borrowed and Cow::is_owned)
 - #65193 (Lockless LintStore)
 - #65479 (Add the `matches!( $expr, $pat ) -> bool` macro)
 - #65518 (Avoid ICE when checking `Destination` of `break` inside a closure)
 - #65583 (rustc_metadata: use a table for super_predicates, fn_sig, impl_trait_ref.)
 - #65641 (Derive `Rustc{En,De}codable` for `TokenStream`.)
 - #65648 (Eliminate `intersect_opt`.)
 - #65657 (Remove `InternedString`)
 - #65691 (Update E0659 error code long explanation to 2018 edition)
 - #65696 (Fix an issue with const inference variables sticking around under Chalk + NLL)
 - #65704 (relax ExactSizeIterator bound on write_bytes)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Oct 24, 2019
Rollup of 12 pull requests

Successful merges:

 - #64178 (More Clippy fixes for alloc, core and std)
 - #65144 (Add Cow::is_borrowed and Cow::is_owned)
 - #65193 (Lockless LintStore)
 - #65479 (Add the `matches!( $expr, $pat ) -> bool` macro)
 - #65518 (Avoid ICE when checking `Destination` of `break` inside a closure)
 - #65583 (rustc_metadata: use a table for super_predicates, fn_sig, impl_trait_ref.)
 - #65641 (Derive `Rustc{En,De}codable` for `TokenStream`.)
 - #65648 (Eliminate `intersect_opt`.)
 - #65657 (Remove `InternedString`)
 - #65691 (Update E0659 error code long explanation to 2018 edition)
 - #65696 (Fix an issue with const inference variables sticking around under Chalk + NLL)
 - #65704 (relax ExactSizeIterator bound on write_bytes)

Failed merges:

r? @ghost
@bors bors merged commit e76a184 into rust-lang:master Oct 24, 2019
4 checks passed
4 checks passed
pr Build #20191023.22 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
@SimonSapin SimonSapin deleted the SimonSapin:matches branch Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.