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

syntax: Always parse pub ( path_start in tuple structs as visibility #33100

Closed
wants to merge 1 commit into from

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Apr 19, 2016

Addresses #32409 (comment)

Pros: Visibilities can be applied to tuple struct fields, only two symbols of look-ahead are required.

struct S(pub(foo::bar) u8);

parses as struct with a pub(restricted) field of type u8.

Cons: Some tuple structs stop parsing

struct S(pub (foo::bar::<u8>, foo::baz));

We start parsing pub (foo::bar as a visibility and fail somewhere in the middle.

Why it is not so bad:

  • The breakage seems to be rare, at least rustc itself doesn't contain any affected code, even in tests. Crater run is needed.
  • The workaround is simple - surround the type with parentheses - ((foo, bar)). ( isn't a path start token, so ((foo, bar)) parses as a type. Alternatively, a type alias can be used for (foo, bar).
  • It doesn't affect macros. If a macro accept types we can pass a tuple (foo::bar::<u8>, foo::baz) to it without fear of syntactic conflicts even if it's preceeded by pub in the macro.
macro_rules! m {
    ($t: ty) => (struct S(pub $t);)
}

m! { (foo::bar::<u8>, foo::baz) } // OK

Alternatives:

  • Parse pub (path) TYPE with hacks. Always parse what goes after pub as a type (but not $ty nonterminal) (unless it's (crate) or $vis) and try to reinterpret it as a visibility if it's not followed by , or ).
    Reinterpretation fails if the parsed path is not TyKind::Path wrapped into TyKind::Paren or contains type parameters.
  • Use lookahead of arbitrary length, parse the whole path first and decide what to do after that. A path can be an actual visibility or a tuple's first element, or an operand of type sum, etc.
  • Support only unambiguous pub(crate) (and $vis if Add a vis matcher to macro_rules!. rfcs#1575 is accepted) in tuple structs. This is essentially this PR other way round - everything that starts like a type parses as a type. However workarounds are wordy if pub(restricted) is actually wanted.
  • Use some other syntax until the current one is de-facto stabilized (I, personally, like pub@path, but would prefer to keep the current syntax).
  • Do not support pub(restricted) in tuple structs. This just looks like a surrender.

[breaking-change], needs a crater run before proceeding.
r? @nikomatsakis

@nikomatsakis nikomatsakis added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed A-const-fn labels Apr 20, 2016
@nikomatsakis
Copy link
Contributor

Nominating for discussion at lang-team meeting. Just for add'l info, I'll kick off a crater run.

@nikomatsakis
Copy link
Contributor

Result of crater run:

  • 4653 crates tested: 3176 working / 1402 broken / 0 regressed / 0 fixed / 75 unknown.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 22, 2016

So we discussed this in the @rust-lang/lang meeting yesterday. The conclusion was that, even though there are no regressions observed on crates.io, we don't feel we can make this change. It is too much of a breaking change, in that it effectively breaks "newtype'd tuples". Moreover, it still introduces irregularities in the grammar where extra parens are required. It just seems to be preferring the newer syntax over the old, which is probably the opposite of how we ought to do things. We also discussed the problem more generally. The feeling was that just permitted pub in tuple structs is really quite unfortunate and not a satisfactory solution either. Our preferred path -- which I will note on the original bug as well -- was to explore the "cover grammar" alternative, where we basically use additional lookahead, or -- alternatively -- parse what follows the parens as a type, and then reinterpret as a path if necessary. Naturally I'd be interested to note if this is untenable (but comment on the original issue).

Therefore I'm going to close the PR. Thanks @petrochenkov in any case!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants