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

Tracking issue for `:vis` macro matcher #41022

Closed
durka opened this Issue Apr 2, 2017 · 28 comments

Comments

Projects
None yet
@durka
Contributor

durka commented Apr 2, 2017

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

Introduced in #41012.

@est31

This comment has been minimized.

Contributor

est31 commented May 12, 2017

cc #41949

@durka

This comment has been minimized.

Contributor

durka commented May 17, 2017

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

This comment has been minimized.

Contributor

bluss commented Oct 19, 2017

Is this mature enough to be stabilized?

@durka

This comment has been minimized.

Contributor

durka commented Oct 19, 2017

#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

This comment has been minimized.

Contributor

durka commented Dec 13, 2017

Shall we stabilize this?

@cramertj

This comment has been minimized.

Member

cramertj commented Jan 17, 2018

This feature was implemented and tested in #41012. AFAICT there have been no issues or problems since then.

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

rfcbot commented Jan 17, 2018

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

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 #41022 (comment)

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

This comment has been minimized.

Contributor

nikomatsakis commented Jan 17, 2018

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

@nrc

This comment has been minimized.

Member

nrc commented Jan 18, 2018

@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

This comment has been minimized.

Contributor

durka commented Jan 18, 2018

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Jan 18, 2018

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

This comment has been minimized.

Contributor

nikomatsakis commented Jan 18, 2018

@durka thanks for the examples.

@cramertj

This comment has been minimized.

Member

cramertj commented Mar 6, 2018

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

@nrc

This comment has been minimized.

Member

nrc commented Mar 7, 2018

@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

This comment has been minimized.

Contributor

nikomatsakis commented Mar 7, 2018

@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

This comment has been minimized.

Member

kennytm commented Mar 7, 2018

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

This comment has been minimized.

Contributor

durka commented Mar 7, 2018

@kennytm

This comment has been minimized.

Member

kennytm commented Mar 7, 2018

@durka crate is already a keyword...

@durka

This comment has been minimized.

Contributor

durka commented Mar 7, 2018

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

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Mar 8, 2018

@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

This comment has been minimized.

Member

cramertj commented Jul 25, 2018

@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

This comment has been minimized.

Member

nrc commented Jul 27, 2018

@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

This comment has been minimized.

rfcbot commented Jul 27, 2018

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

@rfcbot

This comment has been minimized.

rfcbot commented Aug 6, 2018

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

@cramertj

This comment has been minimized.

Member

cramertj commented Aug 13, 2018

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

This comment has been minimized.

Contributor

jkozlowski commented Aug 13, 2018

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

@jkozlowski jkozlowski referenced this issue Aug 15, 2018

Merged

Stabilize macro_vis_matcher #53370

3 of 3 tasks complete

kennytm added a commit to kennytm/rust that referenced this issue Aug 20, 2018

Rollup merge of rust-lang#53370 - jkozlowski:stabilize-macro_vis_matc…
…her, r=cramertj

Stabilize macro_vis_matcher

This PR should stabilize [macro_vis_matcher](rust-lang#41022) feature.

- [ ] "reference" book changes: rust-lang-nursery/reference#400
- [ ] "Rust by example" book changes: rust-lang/rust-by-example#1096
- [ ] "clippy" changes: rust-lang-nursery/rust-clippy#3055

r? @cramertj

@ehuss ehuss referenced this issue Aug 21, 2018

Open

Syntax Highlighting: Unstable Tracking Issue #333

2 of 7 tasks complete

kennytm added a commit to kennytm/rust that referenced this issue Aug 21, 2018

Rollup merge of rust-lang#53370 - jkozlowski:stabilize-macro_vis_matc…
…her, r=cramertj

Stabilize macro_vis_matcher

This PR should stabilize [macro_vis_matcher](rust-lang#41022) feature.

- [ ] "reference" book changes: rust-lang-nursery/reference#400
- [ ] "Rust by example" book changes: rust-lang/rust-by-example#1096
- [ ] "clippy" changes: rust-lang-nursery/rust-clippy#3055

r? @cramertj
@Centril

This comment has been minimized.

Contributor

Centril commented Sep 15, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment