Add a vis matcher to macro_rules!.

[DanielKeep]

Add a vis matcher to macro_rules! that matches valid visibility annotations.

(Rendered).

(Implementation).

[strega-nil]

Please. Please please please please please please please please please. Please.

That is all.

[durka]

This is sorely needed and the design presented here looks simple yet comprehensive.

It would be great if we could time this with the implementation of RFC 1422 so that macros can be updated.

Another possible name for the matcher is :pub.

[petrochenkov]

Another alternative is to make visibility always representable as pub(path).

With RFC 1422 "non-pub" can already be represented as pub(self).
For pub(crate) we need to make the path to a crate root module expressible in code, it would be useful irrespectively to this issue. :: looks like a path to a crate root, however it doesn't even parse currently, but it can be fixed.
For pub something that is a path syntactically, but is not semantically valid, can be used. For example pub(::super) looks pretty intuitive - a parent (super) of the crate root (::) is the whole universe of crates.

Also, this parsing problem may be relevant to this RFC.

[durka]

Another alternative is to make visibility always representable as pub(path).

I don't quite understand what you mean by this. Do you mean to transform something like m!(struct S { x: i32 }) to m!(pub(self) struct S { pub(self) x: i32 }) before expanding the macro? That doesn't really seem workable as the macro might not even be parsing it as a regular struct.

Also, this parsing problem may be relevant to this RFC.

Well, building in a matcher to the macro system will make macros more resilient to whatever solution is devised to fix that problem!

[petrochenkov]

cc @pnkfelix
RFC 1422 contains uniform syntactic forms for visibility as a future direction, but it turns out that they are needed now.

[DanielKeep]

Another alternative is to make visibility always representable as pub(path).

I'm not sure that this is entirely relevant. This would imply either that we require users invoking macros to always write visibility annotations in some "maximally specific" form, or that macro_rules! somehow rewrite invocations that contain what might be visibility annotations to instead include the maximally specific form... which is likely impossible.

I agree that having a uniform syntax for visibility annotations would be useful for expansions and other forms of code generation, but it doesn't help us with parsing what's already valid in the language. At least, I don't think so, based on what you've said. I could be misunderstanding things.

[petrochenkov]

@DanielKeep

either that we require users invoking macros to always write visibility annotations in some "maximally specific" form, or that macro_rules! somehow rewrite invocations that contain what might be visibility annotations to instead include the maximally specific form

Non-user facing macros can be invoked with "maximally specific" visibilities, user-facing macros can use a thin shim to transform pub, pub(path) or nothing into its "canonical"/"maximally specific" form and then work with "canonical" forms only.
(I have no idea how convenient it will be in practice, I never wrote user-facing macros.)

[DanielKeep]

@petrochenkov That's not really all that different from the current situation. Currently, if you need to use the visibility, you have to parse it, then pack it inside a tt, then unpack at the other end. If you're going for a uniform, restricted form, you'd probably still want to pass it around as a tt... depending on how the parser is implemented, you may or may not be able to get away without the reparse trick.

Plus, you'd still require a new, third rule for restricted visibility, and it doesn't improve the situation with struct fields (though I admit that one's still half-broken even with this proposal).

[durka]

Yeah, what @petrochenkov is proposing is the "Do nothing" alternative to this RFC.

[DanielKeep]

I threw together an implementation, linked in the top comment. As a result of this, the follow set was modified to include comma and exclude priv.

[DanielKeep]

I added NtTy and :: to the follow list on the basis that you should be able to match against struct A(pub i32, ::Thing); etc. The implementation and test was also updated.

[nikomatsakis]

I'm in favor of this RFC, but there is one complexity that I don't see discussed in the RFC itself -- visibilities now include pub(path), not just pub. Moreover, the interpretation of something as a visibility -- at least in tuple structs -- requires lookahead to resolve (due to stuff like struct Foo(pub(path) (ty))). I imagine that this would mean that one cannot be 100% compatible with a tuple struct, or at least not without a more complex workaround, since something like:

macro_rules! foo {
    (struct $name:id ( $v:vis $t:ty )) => { ... }
}

when applied to struct Foo(pub (i32, u32)) would fail to parse. That is, I would expect the parsing of the visibility to greedily gobble the (i32 but choke on the ,. That would then reject this arm and fallback to nothing.

[durka]

The addition of RFC 1422 is one reason that this matcher would be so useful, and so difficult for macros to work around today. The problem with parsing tuple structs was already discovered and fixed in the normal parser, I thought.

[nikomatsakis]

@durka I agree this RFC would be useful for dealing with visibilities. Moreover, rust-lang/rust#33161 (not yet landed) does address the issue of tuple struct parsing. But that fix will not help macros, since it relies on (effectively) backtracking in the parser based on what follows the visibility (though it's implemented as reinterpreting a type as a path, the principle is the same). My understanding of how macros work is that backtracking is limiting to backtracking between arms today, so you'd have to have something like this to model tuple structs (which I guess is tolerable, actually):

macro_rules! foo {
    (struct $name:id ( pub $t:ty )) => { ... }
    (struct $name:id ( $v:vis $t:ty )) => { ... }
}

Regardless, I'd like to see some discussion of this general topic in the RFC.

[DanielKeep]

@nikomatsakis Urgh, yuck.

My initial thought was: "Oh, easy; you can't have restricted paths on tuple struct fields, so we'll just add a nrvis (non-restricted visibility) matcher as well and problem solved!" Then I saw 33161.

If we could destructure tokens, the easy fix would be to add a vis_n_ty matcher. But we can't.

I wondered if maybe we could just grab the following tts, construct a sub-parser and use that to find out whether pub is followed by a delimited path. No, extracting tts is destructive.

At present, I don't have any idea what to do about this. I'll mull on it.

[durka]

I see now that the fix for rust-lang/rust#33161 was applied outside the parse_visibility function itself. I was hoping that given struct Foo(pub (i32, u32)), the :pub matcher would eat pub, speculatively eat (i32, see the , and go back to just pub. This is backtracking, but not in macros per se -- it's in the matcher itself.

[nrc]

I'm in favour of the idea, but I worry about the fact that the semantic meaning of the visibility modifier does not correspond exactly with the syntax. For example, matching the visibility of a private field in a struct would give an empty string, but applying that to a method in a trait means 'public' (obvs, this is more of a problem in reverse).

I'd also like to see a solution to the pub(restricted) issue before accepting this RFC. And though I'm in favour of adding a visibility matcher, I'm not in favour of adding two of them since that seems like too much complexity.

I wonder if this ends up being something that can only be handled by procedural macros - the complexity here suggests to me that it perhaps can't be handled by syntactic macros.

[durka]

Well it can be handled by syntactic macros, it's just a huge pain: https://is.gd/F4WRHr

[nikomatsakis]

I am definitely sympathetic to the aims, but I think the pub(restricted) issue is not yet resolved to my satisfaction. To handle pub(restrictied), we've been planning to rely on a cover grammar, but that only works if you can decide (based on the tokens that follow) how to interpret pub(XXX) (is that XXX a type or a path). This works ok in the Rust grammar, which we control, but it's not obvious to me how it should work within the matcher --

@durka points out that we could use some kind of speculative forward parsing. I think the idea would be that we push a "checkpoint" and try to interpret pub(XXX) as a visibility and then continue with the macro. If everything works out ok, that's great, but otherwise we reset and try to consider just pub as the visibility. This would be a very different sort of parser (much more like a PEG or parser combinator than what we have now), but let's leave that aside for a second (though I think implementation concerns are valid here). It also seems to introduce some room for other weird ambiguities, particularly if we ever fix the issues around fallback between arms. Consider a macro like this one:

macro_rules! foo {
    ($v:vis) => { ... }
    ($v:vis $t:ty) => { ... }
}

Should pub (u32) match the first or second arm?

(I am vaguely concerned that this will have new forwards compatibility concerns as well, seems like everything does, though that's just FUD on my part until I come up with sometime more concrete.)

[durka]

I just wish we had considered this while deciding on the syntax for
restricted visibility instead of shunting macros to the wayside as usual.
In theory we could re-open that while it's still unstable and see if there
is a more parseable syntax (one that doesn't have the tuple issue).

On Mon, Aug 8, 2016 at 2:14 PM, Niko Matsakis notifications@github.com
wrote:

I am definitely sympathetic to the aims, but I think the pub(restricted)
issue is not yet resolved to my satisfaction. To handle pub(restrictied),
we've been planning to rely on a cover grammar, but that only works if you
can decide (based on the tokens that follow) how to interpret pub(XXX)
(is that XXX a type or a path). This works ok in the Rust grammar, which
we control, but it's not obvious to me how it should work within the
matcher --

@durka https://github.com/durka points out that we could use some kind
of speculative forward parsing. I think the idea would be that we push a
"checkpoint" and try to interpret pub(XXX) as a visibility and then
continue with the macro. If everything works out ok, that's great, but
otherwise we reset and try to consider just pub as the visibility. This
would be a very different sort of parser (much more like a PEG or parser
combinator than what we have now), but let's leave that aside for a second
(though I think implementation concerns are valid here). It also seems to
introduce some room for other weird ambiguities, particularly if we ever
fix the issues around fallback between arms. Consider a macro like this one:

macro_rules! foo {
($v:vis) => { ... }
($v:vis $t:ty) => { ... }
}

Should pub (u32) match the first or second arm?

(I am vaguely concerned that this will have new forwards compatibility
concerns as well, seems like everything does, though that's just FUD on my
part until I come up with sometime more concrete.)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1575 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAC3n2b85Yb58-8_13aZKOLiv48HckdFks5qd3HugaJpZM4IBxG5
.

[DanielKeep]

I hadn't considered revising pub's syntax... given some of the parsing weirdness already present, maybe that would be a good idea even without this RFC? I can't think of any good alternatives, myself... pub @ path? pub "path"? pub{path}?

As an aside, I'd intended to see if I could find a (more drastic) implementation solution for the POC, but I just cannot get rustc to build any more. For the moment at least, work on the POC is stalled. Without that, I'm not enthusiastic about putting forward any specific proposal to solve the restricted pub problem, since I wouldn't be able to verify it being tractible.

[petrochenkov]

@DanielKeep
pub@path looks pretty nice for my taste. I suggested it too as an alternative in rust-lang/rust#33100.
If @ becomes a starting token for some type in the future then we could just continue always parsing pub@ as visibility and require parens if that is not the case (pub (@TYPE)).
Changing the syntax for pub(restricted) would be unfortunate, but still doable, I suppose.

[nikomatsakis]

pub@path is ambiguous. How do you parse pub@foo :: bar? is that pub(foo) ::bar? or pub(foo::bar)?

[petrochenkov]

How do you parse pub@foo :: bar?

Greedily? pub(foo::bar)
Not sure what problems such an ambiguity can cause (at least it looks more benign than the existing one).

[durka]

I guess pub{restricted} has no ambiguities (currently).

On Wed, Aug 10, 2016 at 5:30 PM, Vadim Petrochenkov <
notifications@github.com> wrote:

How do you parse pub@foo :: bar?

Greedily? pub(foo::bar)
Not sure what problems such an ambiguity can cause (at least it looks more
benign than the existing one).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1575 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAC3n27VGUVysdAWYfKaeI2OYMdD-VZ7ks5qekL2gaJpZM4IBxG5
.

[durka]

or pub(in foo::bar).

[petrochenkov]

pub(in foo::bar)

pub(super) and pub(crate) are going to be used very often in crates with >1 levels of modules, I wouldn't want to make them any longer/noisier than they are now. Preferably they should be shorter (ideally simple pub should desugar into one of those, but it's too late to change this now).
(Curly braces in pub{super} look... inappropriate, but that's subjective, otherwise it's good.)

[DanielKeep]

@petrochenkov Well, pub(super) and pub(crate) could be exceptions; they're fixed length and cannot possibly be types, so they hopefully wouldn't be an issue. Maybe have them as aliases for pub(in super) and pub(in crate) just for consistency.

[nikomatsakis]

Let's take the discussion about alternate syntaxes to the tracking issue for pub(restricted), rust-lang/rust#32409. I left a comment there summarizing the dilemna: rust-lang/rust#32409 (comment)

[nikomatsakis]

Hear ye, hear ye! This RFC is now entering final comment period with an inclination to close. The general feeling is that the we should revisit after we decide whether or not to adopt a distinct syntax for pub(restricted).

Summary of the conversation:

• one can make all visibilities represented as pub(path)
• the meaning of a visibility declaration -- or lack thereof -- is dependent on context (e.g., in a pub enum, no modifier means public), which could be a concern
• the pub(restricted) syntax has real parsing problems)

@rust-lang/lang members, please check off your name to signal agreement. Leave a comment with concerns or objections. Others, please leave comments. Thanks!

• @nikomatsakis
• @nrc
• @aturon
• @eddyb
• @pnkfelix (on vacation)

[nikomatsakis]

Based on the reasoning in the previous comment, we've decided to close this RFC. As stated, if we adopted a distinct syntax for pub(restricted), it would make sense to revisit. Thanks @DanielKeep for the RFC!

[durka]

pub(restricted) is now stabilized with an unambiguous syntax. Can we revive this?