-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
[RFC-2011] Expand matches!
#110382
[RFC-2011] Expand matches!
#110382
Conversation
r? @wesleywiser (rustbot has picked a reviewer for you, use r? to override) |
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
This comment has been minimized.
This comment has been minimized.
It looks like there is an issue with bindings defined in the pattern and then being used in the guard. The following code is broken: fn main() {
matches!(Some(1), Some(x) if x == 2);
} EDIT: not sure I have time to help but since you can't build #![feature(no_core, staged_api, rustc_attrs, lang_items, decl_macro)]
#![no_core]
#![stable(feature = "rust", since ="1.0")]
#[rustc_builtin_macro]
#[stable(feature = "matches_macro", since = "1.42.0")]
pub macro matches($expression:expr, $pattern:pat $(if $guard:expr)? $(,)?) {{ /* compiler built-in */ }}
#[lang = "sized"]
trait Sized {}
enum Option<T> {
Some(T),
None
}
use Option::Some;
fn main() {
matches!(Some(1), Some(x) if x == 2);
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! I think this is a good example use of the builtin#
syntax. I have a draft branch for it, and matches
is a really good macro to start with because it is comparatively easy to parse.
Edit: branch lives here. I'm not suggesting that matches
should use builtin#
right in this PR, I would port matches in a future builtin#
PR.
This comment has been minimized.
This comment has been minimized.
Well, that is unfortunate. Taking a different approach, This new approach doesn't look semantically appropriated and might incur a negative performance impact but as commented before, I am open to new suggestions. There is also a plan to include a mechanism that will allow developers to tweak how much information |
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
@rustbot labels -S-waiting-on-author +S-waiting-on-review @petrochenkov Macros are now being expanded to their Thanks @est31 for the |
@@ -86,6 +86,9 @@ fn main() { | |||
// index | |||
[ [1i32, 1][elem as usize] == 3 ] => "Assertion failed: [1i32, 1][elem as usize] == 3\nWith captures:\n elem = 1\n" | |||
|
|||
// matches | |||
[ matches!(Some(elem == 1i32), None) ] => "Assertion failed: builtin # matches(Some(elem == 1i32), None)\nWith captures:\n elem = 1\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assertion failed: builtin # matches ...
🤷
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The job Click to see the possible cause of the failure (guessed by this bot)
|
I don't think this is needed at all. It is better to have the macro be a normal macro_rules or 2.0 macro instead. |
ExprKind::Matches(expr, _, _) => { | ||
self.manage_cond_expr(expr); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@est31 Someway, somehow it is necessary to have an indicator pointing to the scrutinee expression of matches!
in order to capture its content and the use of regular declarative macros makes matches!
in this context a opaque ExprKind::MacCall
which can't be captured without further processing. In regards to macro 2.0, I personally don't know how it can help here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah then I have misunderstood and this PR is not an use case for builtin #
really. I'm sorry for having suggested it earlier.
Your PR currently hardcodes the matches!
macro, meaning that it can't be shadowed or used via a different name. The same is true for offset_of!
which currently isn't part of the prelude, and which is also unstable, but this PR exposes it without feature gating. Also the builtin equivalent of offset_of is not meant to be used directly as it doesnt have as good error reporting or recovery (and it tolerates some things that are caught one layer higher by the macro_rules macro).
So this is not an approach that reviewers will merge I think.
If you want to do hardcoding, you could confine it at least to what happens inside the assert!()
call and just check the path of the MacCall
and if it only has one item and that one is matches
, then processing the content. This would also not work via shadowing and use with a different name, but would be only confined to uses inside assert!()
, not everywhere, and would not touch offset_of
. It would also not need a new AST item which should still be added sparingly.
That would not be perfect but many times better than what there is now in this PR.
If you want to be a bit more advanced you could maybe try to resolve the macro first, then check if it is the same SyntaxExtension
as the one of a pre-stored matches
syntax extension.
The correct way of using builtin #
here is to both turn matches
and assert
into builtin #
constructs, but personally I'm more of a fan of re-parsing the same info multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @est31
Hum... Looks like a way forward is unclear, three different approaches were already presented (including adhoc processing inside assert
as you commented) without much apparent success.
To avoid further back-and-forth, I think it is better to wait for @petrochenkov. Personally, I am happy with any strategy as long as matches!
are expanded :)
If you want to be a bit more advanced you could maybe try to resolve the macro first, then check if it is the same SyntaxExtension as the one of a pre-stored matches syntax extension.
I hadn't thought that, looks promising!
I'm still skeptical about this whole idea. |
Maybe we shouldn't even pretend resolving Is that the same thing as @est31 suggests in #110382 (comment)? |
Another alternative is to "just use The only problem is backward incompatibility of adding |
Yes, and indeed without the "bit more advanced" part. No change of the AST is needed for that, nor is it using special abilities of builtin macros that aren't available to ordinary proc-macros. It will weird out people a little bit I guess when they shadow the |
For the record, the expansion of pub enum MyError {
ExternalBarError(BarErrorStructure),
ExternalFooError(FooErrorStructure),
MyCustomErrorVariant,
...
}
fn do_some_testing(error: &MyError) {
// It is not possible to use `assert_eq!` because third-parties don't implement `PartialEq`
assert!(matches!(error, MyError::MyCustomErrorVariant));
} It is possible to restrain the parsing of With all discussion so far, I think the situation needs more investigation in order to create an acceptable and reasonable solution. Right? Should we then postpone or go ahead with a special |
Which macros would those be? I cannot think of many macros in the standard library that have a bool output. I could think that one wants |
Ooopss, sorry. I was expanding the term to anything that accepts let elem = 1;
// assert!(vec![elem, 2] == vec![]);
let v = vec![elem, 2];
asset!(v == vec![]);
// Other potential examples are `env!`, `format!` and `offset_of!`. |
☔ The latest upstream changes (presumably #111928) made this pull request unmergeable. Please resolve the merge conflicts. |
Well, I guess further progress will require more discussion or even a RFC. Hope the future holds better opportunities for this feature. |
cc #44838
I really thought for months how to expand
matches
and the best approach I could find was the introduction of a newExprKind
variant as well as turningmatches!
into a built-in macro. Please, let me known if there is a better method.cc @oli-obk Reviewed the last PRs
cc @petrochenkov We talked a bit about this subject