Rust: Tracking issue for `:vis` macro matcher

[durka]

Tracks stabilization for accepting :vis as a fragment specifier in macros, gated by #![feature(macro_vis_matcher)].

Introduced in #41012.

[est31]

cc #41949

[durka]

I tried in #40984 to parse a pattern like $(#[$attr:meta])* $v:vis but the visibility isn't enough to terminate the repetition. It's unclear if this is a systemic issue with macro_rules! or if it can be worked around.

Edit: fixed by #42913.

[bluss]

Is this mature enough to be stabilized?

[durka]

45388 ~will be~ was a breaking change to :vis: consider macro_rules! foo { (crate) => { true }; ($v:vis) => { false } } and macro_rules! foo { ($v:vis crate) => { } }.

[durka]

Shall we stabilize this?

[cramertj]

This feature was implemented and tested in https://github.com/rust-lang/rust/pull/41012. AFAICT there have been no issues or problems since then.

@rfcbot fcp merge

[rfcbot]

Team member @cramertj has proposed to merge this. The next step is review by the rest of the tagged teams:

• [x] @aturon
• [x] @cramertj
• [x] @eddyb
• [x] @nikomatsakis
• [x] @nrc
• [x] @pnkfelix
• [x] @withoutboats

Concerns:

• if we make further changes to the visibility modifiers as we continue to evolve the module system, will this cause back compat issues here? resolved by https://github.com/rust-lang/rust/issues/41022#issuecomment-408330588

Once a majority of reviewers approve (and none object), 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.

[nikomatsakis]

I would like to see a comment pointing out the relevant tests so I can review the behavior. =)

[kennytm]

@nikomatsakis

• https://github.com/rust-lang/rust/blob/master/src/test/compile-fail/issue-42755.rs
• https://github.com/rust-lang/rust/blob/master/src/test/run-pass/macro-first-set.rs#L53
• https://github.com/rust-lang/rust/blob/master/src/test/run-pass/macro-pub-matcher.rs

[nrc]

@rfcbot concern if we make further changes to the visibility modifiers as we continue to evolve the module system, will this cause back compat issues here?

[durka]

I would assume that :vis will continue to match whatever visibility
specifiers are added. Such further changes should be carefully designed to
avoid introducing ambiguities.

On Wed, Jan 17, 2018 at 9:39 PM, Nick Cameron [e-mail]
wrote:

@rfcbot https://github.com/rfcbot concern if we make further changes to
the visibility modifiers as we continue to evolve the module system, will
this cause back compat issues here?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/41022#issuecomment-358519542,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC3n525WWXDYbK-7vTJc8J9D2cVRXflks5tLq7tgaJpZM4Mw6bP
.

[nikomatsakis]

I agree with @durka. This is definitely "a thing", but I think that to a large extend these ambiguities exist already. For example, introducing the crate modifier and the possibility of paths like crate::X introduced an ambiguity that affects the :vis visibility specifier -- but also tuple structs (same with pub(in foo) style visibility modifiers).

[nikomatsakis]

@durka thanks for the examples.

[cramertj]

@nrc Was your concern adequately addressed, or do you have more questions?

[nrc]

@cramertj it kind of does. However, we seem to be very close to settling on a final design for modules/visibility reform and it seems to me that waiting another month or so for that before stabilising would be beneficial. Consider the concern resolved if others don't think that meaningfully reduces risk.

[nikomatsakis]

@nrc hmm -- actually -- now that you mention it =)

So the main change there (which is not afaik controversial) is that we are going to add crate as a keyword. One would expect the behavior of $vis to change when crate is added, which may of course break macros. This is of course nothing new. (crate also adds a measure of ambiguity -- at least in some proposals -- in that paths could plausibly begin with crate.) I continue to regret that we have format specifiers at all.

All that said, I'm still sort of inclined to go forward with vis. We routinely break macros by extending Rust's grammars, sadly enough. But it'd be nice to know what the approved follow-set of vis is, I forget. As long as it's suitably narrow, this shouldn't be a big problem.

[kennytm]

The follow set is currently:

1. ,
2. all identifiers and keywords except priv
3. tokens that can_begin_type()
4. meta-var which is :ident, :ty or :path

We may need to change rule 2 to exclude crate. But why priv is outside of the follow-set while pub is in it? Or is it just an oversight? 🤔

[durka]

I can't say I appreciate the fatalistic attitude of "well, we're going to
break macros and assume there's no opposition". But in that case, perhaps
we should hold off stabilizing :vis until crate is a keyword?

On Wed, Mar 7, 2018 at 2:03 PM, kennytm [e-mail] wrote:

The follow set
https://github.com/rust-lang/rust/blob/4cdbac639af2074b8a07b4391b4e3e28b4118487/src/libsyntax/ext/tt/macro_rules.rs#L847-L860
is currently:

1. ,
2. all identifiers and keywords except priv
3. tokens that can_begin_type()
4. meta-var which is :ident, :ty or :path

We may need to change rule 2 to exclude crate. But why priv is outside of
the follow-set while pub is in it? Or is it just an oversight? 🤔

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/issues/41022#issuecomment-371247292,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC3n0P0McLIjEfhJJF5OJq2aDPyhnZQks5tcC7ygaJpZM4Mw6bP
.

[kennytm]

@durka crate is already a keyword...

[durka]

Yes, I (and @nikomatsakis I guess) meant it is going to be accepted as a keyword in a new context.

[nikomatsakis]

@durka

I can't say I appreciate the fatalistic attitude of "well, we're going to break macros and assume there's no opposition".

Sorry if I came off too blasé. I'm mostly just embittered with the situation, which I find quite frustrating -- we foresaw the problem, but our solution just didn't work. It annoys me, in part because the simpler solution that we originally had in mind would have worked fine. =) But clearly we are not going to allow this to stop us from modifying our grammar.

I hope we can address in Macros 2.0 by changing how fragments work. As far as I know roughly the only scheme that works is to have $x:foo consume all token trees until something from the designated "follow set" of foo is found, and then try to parse those tokens as a foo, and error if extra tokens are left over. But that's a topic for another thread I suppose.

But in that case, perhaps we should hold off stabilizing :vis until crate is a keyword?

I think it's reasonable.

[cramertj]

@nrc

However, we seem to be very close to settling on a final design for modules/visibility reform and it seems to me that waiting another month or so for that before stabilising would be beneficial. Consider the concern resolved if others don't think that meaningfully reduces risk.

Now that we've come closer to finalizing the modules system under the 2018 edition, do you feel more comfortable resolving your concern here?

[nrc]

@rfcbot resolved if we make further changes to the visibility modifiers as we continue to evolve the module system, will this cause back compat issues here?

(Assuming that :vis will match crate like pub(crate))

[rfcbot]

:bell: This is now entering its final comment period, as per the review above. :bell:

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[cramertj]

This is in need of a stabilization PR! There's a stabilization guide on the forge. Please post here if you plan to take this issue! (I'll circulate it around and see if anyone wants to take it as a good first or third PR)

[jkozlowski]

@cramertj as discussed, I would like to try my hand at doing the stabilisation PR. Will ping if I get stuck

[Centril]

This has been stabilized and the reference has been amended, so this seems done.

[alercah]

I'm popping in here while trying to more fully document the follow-set rules in the reference, and the crate discussion above doesn't quite make sense to me. Was the intent to exclude crate from the follow-set of vis? Because it is currently in it---both because crate has can_begin_type() and because crate is an identifier other than non-raw priv.