-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
:vis matcher for macro_rules #41012
:vis matcher for macro_rules #41012
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
|
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.
Code LGTM
src/libsyntax/ext/tt/macro_rules.rs
Outdated
match *tok { | ||
TokenTree::Token(_, ref tok) => match *tok { | ||
Comma => Ok(true), | ||
ModSep => Ok(true), |
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.
Why do we just allow Comma
and ModSep
? Seems arbitrary to me.
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.
If I remember correctly, I allowed ModSep
because you can already have pub ::path::to::a_type
that you might want to parse and special-case. I allowed comma because having some kind of sequence-delimiting token is useful. If you're passing bits of parsed input around, you might have inner!($thing, $a_vis, $more_tts)
. Everything else was excluded based on wanting to err on the side of being conservative.
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.
I allowed ModSep because you can already have
pub ::path::to::a_type
This seems quite inconsistent, you can already have pub ARBITRARY_TYPE
, but only a subset of paths is allowed.
This should probably use all can_begin_type
tokens (in addition to comma) + ty/path/ident
for MetaVarDecl
s.
Or all is_path_start
tokens (in addition to comma) + ty/path/ident
for MetaVarDecl
s if we want to be conservative.
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.
can_begin_type
seems good, tuple declarations really restrict us from changing visibility syntax too much in the future (as we saw during the pub(restricted)
debate).
src/libsyntax/parse/token.rs
Outdated
@@ -371,6 +371,7 @@ pub enum Nonterminal { | |||
NtGenerics(ast::Generics), | |||
NtWhereClause(ast::WhereClause), | |||
NtArg(ast::Arg), | |||
NtVis(ast::Visibility), |
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.
Now the comment // These are not exposed to macros ....
applies to NtVis
, it's better moved higher in the list.
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.
Fixed.
/* | ||
The rule duplication is currently unavoidable due to the leading attribute | ||
matching. | ||
*/ |
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.
@DanielKeep why did you not use :vis
at the top level of this macro??
Comments addressed. |
6f03265
to
41dcfec
Compare
LGTM, but needs @rust-lang/lang decision |
As a note: I observed someone needing this on the #rust IRC channel a few days ago, to avoid hard-coding "pub". |
@rfcbot fcp merge |
Team member @nrc has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, 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 reviewed |
|
||
* `vis`: a visibility qualifier. Examples: nothing (default visibility); `pub`; `pub(crate)`. | ||
|
||
A `vis` variable may be followed by a comma, ident, type, or path. |
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, I'm feeling dense: Why can a vis
variable be followed by a comma?
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.
To quote @DanielKeep's rationale from above (hidden comment):
I allowed comma because having some kind of sequence-delimiting token is useful. If you're passing bits of parsed input around, you might have
inner!($thing, $a_vis, $more_tts)
.
This isn't required, we can exclude it and you'd have to use ($a_vis)
to pass around parsed visibility fragments within macros.
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.
It seems harmless to me. I can't imagine using ,
as an "operator" in a visibility specifier (at last outside of parens or something).
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.
@pnkfelix are you still worried about this? Should we exclude it in order to get this PR into FCP? I just don't want this to continue languishing for too long...
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.
no its okay; thanks for your patience.
☔ The latest upstream changes (presumably #39987) made this pull request unmergeable. Please resolve the merge conflicts. |
@rfcbot reviewed |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
Rebased. |
☔ The latest upstream changes (presumably #41312) made this pull request unmergeable. Please resolve the merge conflicts. |
Rebased. |
@bors r=petrochenkov |
📌 Commit cfa51f2 has been approved by |
:vis matcher for macro_rules Resurrection of @DanielKeep's implementation posted with [RFC 1575](rust-lang/rfcs#1575). @jseyfried was of the opinion that this doesn't need an RFC. Needed before merge: - [x] sign-off from @DanielKeep since I stole his code - [x] feature gate - [x] docs
Resurrection of @DanielKeep's implementation posted with RFC 1575.
@jseyfried was of the opinion that this doesn't need an RFC.
Needed before merge: