Introduce the regex! macro for easier reusable regex#1371
Conversation
BurntSushi
left a comment
There was a problem hiding this comment.
Thank you!
I think we also want a copy of this macro in the bytes sub-module.
I continue to overall be in favor of this, but the naming here is brutal. I really really continue to like the name regex! for this. The repetition is unfortunate, but even with the terser name of re here, you chose to use its unqualified form in examples. Which to me is an argument in favor of using regex as the name for the macro. IMO, the benefit of a terse name is being able to use its qualified form, i.e., regex::re!(...).
I think re is not a horrible name though. I think it's either that or regex personally.
|
|
||
| use crate::{error::Error, RegexBuilder}; | ||
|
|
||
| /// A convenient way to construct regex patterns. |
There was a problem hiding this comment.
| /// A convenient way to construct regex patterns. | |
| /// A convenient way to construct regex patterns from string literals. |
| /// The static can also be named, allowing for reuse across functions: | ||
| /// | ||
| /// ``` | ||
| /// # use regex::re; |
There was a problem hiding this comment.
| /// # use regex::re; | |
| /// use regex::re; |
I don't like it when examples leave out stuff like this. I know std does it and... I'm not a huge fan. I would rather show users how the thing is being imported. I also find that the examples more closely model reality. :-)
There was a problem hiding this comment.
I tend not to like it in most cases, but feel it helps the flow when multiple examples can be read together as a block with interspersed docs. That said, this example is no longer relevant and removed :)
| /// An invalid pattern will panic when it is first used: | ||
| /// | ||
| /// ```should_panic | ||
| /// # use regex::re; |
There was a problem hiding this comment.
| /// # use regex::re; | |
| /// use regex::re; |
| /// re.is_match("invalid -> ("); // panic! | ||
| /// ``` | ||
| /// | ||
| /// [`LazyLock`]: https://doc.rust-lang.org/std/sync/struct.LazyLock.html |
There was a problem hiding this comment.
I think just [LazyLock] will link to the std docs automatically.
There was a problem hiding this comment.
Huh, that's good to know. Link no longer needed, in any case
| let re: &$crate::Regex = ®EX; | ||
| re | ||
| }}; | ||
| ($vis:vis $name:ident, $re:literal) => { |
There was a problem hiding this comment.
Is there precedent for these sorts of macros that introduce a name in the ecosystem? Is this the right syntax? What about re!(SOME_REGEX = "blah blah")?
There was a problem hiding this comment.
I'm not familiar with any, and with an extremely quick skim of the top ~100 crates on crates.io I didn't see anything. I have a slight preference for the comma version because it reads a bit more like what you might find in (...) function call syntax elsewhere
It's also possible to drop this named version if you prefer. I do think it's useful though, I have a macro like this in a few crates and use it nearly as much as the anonymous version (e.g. grouping related regex patterns at the top of a file when they are used in different functions).
There was a problem hiding this comment.
I do kind of like it. But yeah, I'd say drop it for now. I'm a little hesitant to be blazing a trail here. Also, this can be added back later without breaking changes.
| /// A convenient way to construct regex patterns. | ||
| /// | ||
| /// This macro can be used to construct reusable instances of [`Regex`] with | ||
| /// reduced boilerplate. The constructed `Regex` is wrapped in a [`LazyLock`] so |
There was a problem hiding this comment.
I don't like that we discuss the implementation here. I think we should talk about semantics and not how it's achieved. (Unless you feel like it's really important, in which case, it should go in a separate "Implementation" block that makes it clear this is just an FYI and could change.)
For example, I kind of wonder whether it would be better to use regex-automata::util::lazy::Lazy. It gives up the "at most once" guarantee, but works in core-only environments.
It might be nice to keep the #[cfg(feature = "std")] gate here for now, but switch to regex-automata's Lazy. If no issues appear after a while, then we could remove the feature gate. But that still gives us the flexibility to switch to LazyLock if there are problems.
There was a problem hiding this comment.
Note that in order to use Lazy from regex-automata, we'd need to define a wrapper type to avoid making regex-automata a public dependency. Then that needs to be exported in the crate root. That's kind of a bummer... And if we ever switched back to LazyLock, that type would need to stay and it would be vestigial. Blah...
You might also wonder why targeting core-or-alloc-only use cases even matters... I'm thinking about libraries that depend on regex without any features enabled. They can't use this macro unless they setup their own std feature. Basically, any time you have a #[cfg(feature = "...")] around a public API, you're passing the buck of dealing with that to other downstream libraries. So I try hard to avoid putting such things in public APIs for exactly that reason.
Thoughts?
There was a problem hiding this comment.
I had gone back and forth on this a bit; I didn't know there was a reasonably easy solution in regex_automata, that sounds well worth using to me.
Any thoughts about using #[doc(hidden)] pub mod __private { pub use regex_automata::util::lazy::Lazy; }? That pattern always feels a bit hacky but it does avoid the need for a public wrapper, and expresses some intent to keep the inner workings opaque. Technically somebody with a regex_automata` dependency could still do:
re!(FOO, "abc");
let _: ®ex_automata::util::lazy::Lazy<Regex> = &FOO;But that doesn't seem worth worrying about.
A wrapper could also live in a private module like that, and/or make the needed API match LazyLock so it could become a type alias one day if that is desired.
There was a problem hiding this comment.
The problem with that approach is that it becomes difficult to name the type returned by the macro. I hate it when libraries do this, because there are a variety of reasons why naming a type is useful. I would much rather just add a wrapper type to the crate root than be the source of such annoyances. :-) It's a bit odd and out of place, but I think I can stomach it.
Having the wrapper type's API match a subset of LazyLock seems very wise. Good idea.
There was a problem hiding this comment.
The returned type of the anonymous syntax should just be a &'static Regex, the bit at https://github.com/rust-lang/regex/pull/1371/changes#diff-3be4b210bb3c85bbca46bfb05dfed4dc66c9564a5761c1a9b7bd1e9524c57b24R69-R70 is to keep the LazyLock workings opaque. I think the only way the LazyLock/Lazy could leak is via the named versions. As-written, users should do:
re!(FOO, "abc");
let _: &Regex = FOO;but can do:
let _: &LazyLock<Regex> = FOO;If we're dropping the named version then I think this concern goes away?
(With rust-lang/rust#63065 we could hide this more by expanding to static FOO: impl Deref<Target = Regex>, but that's not happening anytime soon. Maybe there are some better tricks.)
There was a problem hiding this comment.
I was just coming back to write a comment correcting what I wrote, hah. OK, in this case, I'm okay with using a doc(hidden) hack on a re-export of regex-automata's type directly. And if we ever add the named version back, then we can figure out what to do then.
There was a problem hiding this comment.
Switched to the regex-automata version in the latest update and updated the docs to just say it's a static without giving details.
I made a mention of this in the top post but is there a way to do this?
Generally I think that with any name, this is something much more likely to be imported than qualified - I only mentioned that because it was brought up in the issue. I just prefer But that's subjective and three extra characters won't make or break anything, so I'm happy to defer to your preference :) |
Ugh, really!?!? I'm surprised I didn't know this. Probably because I almost never publish macros. Yeah unfortunately just leave it out. Using a different name would suck. And byte regexes are likely used way less frequently.
I'm going to link this PR on some socials and see what we get as a vibe check. |
e432399 to
393b232
Compare
|
Arriving here from socials to provide a vibe check! I dig this a lot and look forward to the convenience. Seems quite nice to me, I'd immediately use it. 👍 I like and prefer |
| /// Because `LazyLock` is used internally, `re!` can only be used with the | ||
| /// `std` feature and with Rust 1.80 or greater. |
There was a problem hiding this comment.
The PR description says:
Using regex_automata's Lazy means there are no MSRV concerns, and this works without std.
So I guess this is no longer a prerequisite?
There are three common abbreviations for regular expressions:
This crate uses |
|
I also favor With that said, |
|
MSRV is a non-issue here in any direction. I should bump to Rust 2024 anyway. |
|
What if you tack on a |
|
I'm pretty meh on |
|
You can have a macro being used as pub mod bytes {
#[macro_export]
macro_rules! __regex {
($re:literal) => {{ .. }};
}
pub use __regex as regex;
}IIRC this is a historical issue that should be resolved with rust-lang/rust#39412 but 39412 is also a long-standing issue :P |
|
I'm okay with a hidden top-level macro, as long as we can put a |
|
Came here from the bluesky post :) You can add You can see the resulting docs here. |
re! macro for easier reusable regexregex! macro for easier reusable regex
| #[doc(hidden)] | ||
| #[macro_export] | ||
| macro_rules! __bytes_regex { | ||
| // ($vis:vis) => {}; |
There was a problem hiding this comment.
| // ($vis:vis) => {}; |
Seems like a redundant comment-out line?
| /// [`clippy::invalid_regex`]: https://rust-lang.github.io/rust-clippy/master/#invalid_regex | ||
| #[macro_export] | ||
| macro_rules! regex { | ||
| // ($vis:vis) => {}; |
There was a problem hiding this comment.
| // ($vis:vis) => {}; |
ditto
|
Yeah, |
|
Oh, sorry for not clarifying; the screenshot above is in the bytes module. Neither That said, this looks identical to the |
|
It seems the issue is that the macro is in Luckily it seems that moving just the re-export to // src/bytes.rs:
/*! ... 90 line doc comment ... */
pub use crate::{builders::bytes::*, regex::bytes::*, regexset::bytes::*};
#[doc(inline)]
pub use crate::__bytes_regex as regex;
// Note that `crate::` is now required |
Add a wrapper around `regex_automata`'s `Lazy` as a simple way to
construct a `Regex` that is compiled once but used multiple times.
Sample usage:
if regex!(r"\d+").is_match("123") { /* ... */ }
let re: &Regex = regex!(r"\d+");
This idea has been discussed in [rust-lang#709]. To address a few of the concerns
from that issue:
1. More than 1 `regex!` can be used in a block (for the anonymous
version, the name is in a scope).
2. Using `regex_automata`'s `Lazy` means there are no MSRV concerns, and
this works without std.
3. There is no compile-time checking. The docs make it clear that
`regex!` should not be used with invalid regex and suggest enabling
the clippy lint, leaving the door somewhat open for e.g. a `const fn`
syntax validator in the future (though this seems unlikely).
A `regex::bytes::regex!` version is also included.
Closes: rust-lang#709
[rust-lang#709]: rust-lang#709
|
That works 🎉! I think everything else should be addressed |

Add a wrapper around
regex_automata'sLazyas a simple way toconstruct a
Regexthat is compiled once but used multiple times.Sample usage:
This idea has been discussed in #709. To address a few of the concerns
from that issue:
regex!can be used in a block (for the anonymousversion, the name is in a scope).
regex_automata'sLazymeans there are no MSRV concerns, andthis works without std.
regex!should not be used with invalid regex and suggest enablingthe clippy lint, leaving the door somewhat open for e.g. a
const fnsyntax validator in the future (though this seems unlikely).
A
regex::bytes::regex!version is also included.Closes: #709