Skip to content
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

Simplify visibility grammar #2640

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@IsaacWoods
Copy link

IsaacWoods commented Feb 20, 2019

Rendered

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Feb 20, 2019

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

This comment has been minimized.

Copy link
Author

IsaacWoods commented Feb 20, 2019

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

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 20, 2019

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

This comment has been minimized.

Copy link
Member

scottmcm commented Feb 21, 2019

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

This comment has been minimized.

Copy link
Member

scottmcm commented Feb 21, 2019

@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

This comment has been minimized.

Copy link

rfcbot commented Feb 21, 2019

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

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

This comment has been minimized.

Copy link
Author

IsaacWoods commented Feb 21, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.