Simplify visibility grammar

[IsaacWoods]

Rendered

[Centril]

I'm in theory in favor of this change as it lets users naturally move from pub(super) to pub(super::super) and such when needed without having to change their mental model re. paths. However, I also think that this paper-cut isn't that common so if we change nothing, that's not so bad either.

However, there's an edge-case to consider, namely:

struct Tuple( pub (crate::Foo) );

to handle that (and avoid the breakage you'd otherwise probably cause), we can retain ~LL(k) with the following strategy:

// NOTE: This is pseudocode that doesn't care about diagnostics and such things...

let (vis, typ) = if eat_next_token("pub") {
    if eat_next_token("(") {
        if is_next_token("in") {
            path = parse_simple_path()?;
            expect_token(")")?;
            (Some(PubRestricted(path)), parse_type()?)
        } else {
            // This is OK because afaik, SimplePath should be a subset of Type:
            let typ = parse_type()?;

            if let Some(path) = as_simple_path(typ) {
                expect_token(")")?;
                (Some(PubRestricted(path)), parse_type()?)
            } else {
                expect_token(")")?;
                (Some(Pub), typ)
            }
        }
    } else {
        (Some(Pub), parse_type()?)
    }
} else {
    (None, parse_type()?)
};

I think that complicates the parser, so I'm not sure it's worthwhile... If we used a parser strategy such as GLL it would get less complicated and we could use your definition almost verbatim (I think).

Also, there's already a pretty good diagnostic:

error[E0704]: incorrect visibility restriction
 --> src/lib.rs:1:12
  |
1 | pub(super::super) fn foo() {}
  |            ^^^^^ help: make this visible only to module `super::super` with `in`: `in super::super`
  |
  = help: some possible visibility restrictions are:
          `pub(crate)`: visible only on the current crate
          `pub(super)`: visible only in the current module's parent
          `pub(in path::to::module)`: visible only on the specified path

[IsaacWoods]

Ah yes, I hadn't considered that ambiguity. Is it possible to disambiguate between paths to modules and paths to types at that stage of parsing (as in, do we know whether crate::Foo is a (badly named) module or a type until name resolution)? If not, I don't think this approach is possible.

[petrochenkov]

That ambiguity is the single reason why in exists (see the long bikeshed in rust-lang/rust#32409).

However, we did a crater run in rust-lang/rust#33100 and it found zero regressions from treating any pub ( as a start of visibility.

However, pub(thing) where thing is not crate or super is pretty much unused, so it may make sense to leave it alone even if it's not perfect.
Moreover, on 2018 edition pub(in thing) needs to be written as pub(in crate::thing) putting the last nail in the coffin of that feature.

(pub(in module) is still very much important for understanding our visibility model though, even if it's not useful in practice.)

[scottmcm]

However, pub(thing) where thing is not crate or super is pretty much unused, so it may make sense to leave it alone even if it's not perfect.

That's my biggest thought here. I'm just not worried about the in.

[scottmcm]

@rfcbot fcp postpone

I don't think this is urgent, and there doesn't seem to be any new information here that would overcome the reasons that in was added originally (#2640 (comment)).

[rfcbot]

Team member @scottmcm has proposed to postpone this. The next step is review by the rest of the tagged team members:

• @Centril
• @aturon
• @cramertj
• @eddyb
• @joshtriplett
• @nikomatsakis
• @pnkfelix
• @scottmcm
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

[IsaacWoods]

Yep, I mistook the reason for in being used, and hadn't seen the original discussion. I agree with the consensus that this isn't a big deal for the work to handle it (if it's possible at all). Apologies for the noise.

[rfcbot]

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

[rfcbot]

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

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC is now postponed.