Regex::must(...) to replace Regex::new(...).unwrap()?

[BurntSushi]

This issue is intended to brainstorm some ideas on how to report regex parse errors to Rust programmers.

One of my long running projects has been a rewrite of the regex-syntax crate. There are several goals I'm achieving via this rewrite, and one of them in particular is better error messages for regex parse errors.

Here's an example:

Here's another example:

The above error messages correspond to the fmt::Display implementation on the error returned by the parser. What I'd like to have happen is for these error messages to appear to programmers when they create malformed regexes in their source code. Specifically, today, the code pattern for building a regex that is known at compile time is:

let re = Regex::new(...).unwrap();

The issue here is that if the regex contains a parse error, then the unwrap implementation will show the Debug message for the error instead of the Display implementation. That's definitely the right behavior in most cases, but in this case, I'd really like to show a nicer error message. My question is: how can I achieve that?

I can think of two ways:

1. Make the Debug implementation defer to the Display implementation. This causes unwrap to show the right error message, but now we don't have a "normal" Debug implementation, which feels kind of wrong to me.
2. Create a new constructor on regex called must (name subject to bikeshedding) that is basically the same as Regex::new(...).unwrap(), except it will emit the Display impl for the error instead of the Debug impl. The downside here is that users of the regex crate need to use must, and this particular style of handling panics isn't really idiomatic in the Rust ecosystem, where we instead prefer an explicit unwrap. The key difference here is that regexes in the source code with parse errors are inherently programmer errors, and programmers should get nice regex error messages, because the Debug impl isn't going to be particularly helpful.

Are there other ways of approaching this problem? What do other people think? My inclination right now is hovering around (2), particularly since Regex::new(...).unwrap() is not only extraordinarily common, but is also correct in the vast majority of cases (with it being incorrect only when the regex isn't known until runtime). That is, normalizing that code pattern with an explicit constructor feels like a good thing to me on its own, but it's not clear how the balances with established idioms.

cc @rust-lang/libs @ethanpailes @robinst @killercup @steveklabnik @bluss

[sfackler]

It seems like this may be a case where new panics and there's an alternative function (maybe just FromStr?) if you actually need to check for a parse error. I don't think I've ever written anything other than Regex::new().unwrap().

[BurntSushi]

@sfackler That's an interesting thought. I guess the problem there is that it's a pretty significant (albeit trivial to fix) breaking change. My plan is to introduce the regex-syntax rewrite without doing a semver bump on regex.

[sfackler]

Oh yeah, I didn't see that this was talking about regex-syntax breakage rather than regex.

[robinst]

Having a panicking constructor is a good idea I think. Some thoughts on that:

1. RegexBuilder would need to get the same method. The current method name there is build. So maybe it would be nice if the names for the panicking new and build shared a common prefix/suffix? Some ideas to show what I mean: new_ok, build_ok. new_expect, build_expect. new_or_fail, build_or_fail etc
2. I'm not a fan of the name must. Is there an existing naming convention in the standard library for something that can fail?
3. Would most cases where you want a panicking new use a pattern that is actually a &'static str (not just a &str). Would it be a good idea to require the panicking constructor to require the argument to be a &'static str? And in case the pattern is dynamically created, you'd probably want to handle the error in a special way and not panic. I'm not sure how this would be applicable to RegexBuilder though.

[clarfonthey]

Longer-term I think the best solution is having some form of compile-time checking as the default and making a runtime-specific version more obvious. I wouldn't mind if initially, the compile-time version just checked the syntax and did a new().unwrap().

[clarfonthey]

To clarify: I think that having a regex! macro which guaranteed that the regex would be valid at compile time, where panicking ever is a bug, should be the primary method of making a regex. It doesn't matter how this is formed (and it should be similar to the current method, not the former macro), but shifting everyone to this method is ideal, as it can display the error in a human-friendly way. The runtime-level version would probably be better named parse to make it clear that it's dynamically generating the regex. Usually, you won't unwrap that, but if you do, it means your code is wrong and you'd be debugging via the usual means.

[BurntSushi]

Compile time checking is definitively out of scope for this issue.

The &'static str idea is nice. It would be backwards compatible if we wanted to loosen that too.

I like must because it is short. We could add it as a method to RegexBuilder too. I would be sad if we ended up with long unwieldy names like "something_or_fail`, especially for such a common operation.

[BurntSushi]

The standard library tends to go in the opposite direction I think, where fallible methods are prefixed with try_. But I don't think there are any instances of constructors.

[killercup]

Great ideas, I especially like the &'static str special :) I also wanted to point out that there is an invalid_regex lint in clippy.

A new thought: Show the fancy parse error when RUST_BACKTRACE=1 (or a special other env var or feature is set) instead or when a special method is used.

[ethanpailes]

I'll just add my voice to the chorus in favor of option number 2.

My bikeshedding 2 cents on the name is that length matters the most because I expect that people will end up writing this a lot, but must isn't quite as obvious as it could be. I kind of like the new_ok/build_ok option. I don't think it is much more obvious than must but @robinst is right that there should be an option for using this with RegexBuilder and this way the two approaches are related.

[BurntSushi]

OK, so it looks like I can actually sidestep this issue for the most part. One thing I forgot about was that the error type in the regex crate doesn't actually use the error type from regex-syntax to avoid creating a public dependency. Instead, it just converts the syntax error to a string. Since the debug representation at this point is just going to show that string anyway, we might as well format it a little more nicely. Example:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Syntax(
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
regex parse error:
    hello\p{Greeek}wat
         ^^^^^^^^^^
error: Unicode property not found
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
)', libcore/result.rs:945:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.

Apologies for the distraction folks!

(I still do kind of like the aesthetics of Regex::must, but I suppose we can stew on that.)