Skip to content

Parse pub(restricted) visibilities on tuple struct fields #33161

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 2 commits into from
Apr 28, 2016
Merged

Parse pub(restricted) visibilities on tuple struct fields #33161

merged 2 commits into from
Apr 28, 2016

Conversation

jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Apr 23, 2016 •
edited
Loading

Parse pub(restricted) on tuple struct fields (cc #32409).

r? @nikomatsakis

@jseyfried jseyfried changed the title Parse tuple struct field vis Parse pub(restricted) visibilities on tuple struct fields Apr 23, 2016
@jseyfried
Copy link
Contributor Author

cc @petrochenkov

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 23, 2016 •
edited
Loading

Ha, I expected to implement this myself today.

I'd like to see more tests for error messages for failed reinterpretation.
Also, ty nonterminals shouldn't be interpreted as visibilities even if they expand into (path), i.e. pub $t u8 or pub ($t) u8 where $t: ty shouldn't parse (this needs tests too).

@jseyfried
Copy link
Contributor Author

jseyfried commented Apr 23, 2016 •
edited
Loading

@petrochenkov

I'd like to see more tests for error messages for failed reinterpretation.

Will do. Do you think there should be a different error message for failed reinterpretation?
Now, pub(non-path type) ... behaves exactly as it did before this PR.

Also, ty nonterminals shouldn't be interpreted as visibilities even if they expand into (path)

Good point, I hadn't considered that. Will do.

@petrochenkov
Copy link
Contributor

Do you think there should be a different error message for failed reinterpretation?

It just needs to be reasonable, I think the current one is ok.

@bors
Copy link
Collaborator

bors commented Apr 24, 2016

☔ The latest upstream changes (presumably #33179) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried jseyfried force-pushed the parse_tuple_struct_field_vis branch from 0deeebf to 1bfc50b Compare April 25, 2016 02:56
jseyfried
jseyfried reviewed Apr 25, 2016
View reviewed changes
src/test/compile-fail/privacy/restricted/tuple-struct-fields/test.rs
type T = ();
struct S1(pub(foo) (), pub(T), pub(crate) (), pub(((), T)));
struct S2(pub(id!(foo)) ()); //~ ERROR expected one of `+` or `,`, found `(`
//~| ERROR expected one of `+`, `;`, or `where`, found `(`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cf #33188

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikomatsakis we have this test case, but the macro id should be

#![feature(type_macros)]
macro_rules! id {
    ($t:ty) => { $t }
}

instead of how it is defined now.

Is this the type of test you had in mind?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add more tests, but due to poor error recovery we need one test per parse error.

@jseyfried
Copy link
Contributor Author

I rebased and addressed @petrochenkov's comments.

nikomatsakis
nikomatsakis reviewed Apr 27, 2016
View reviewed changes
src/libsyntax/parse/parser.rs
let ty = p.parse_ty_sum()?;
let mut vis = p.parse_visibility(false)?;
let ty_is_interpolated =
p.token.is_interpolated() || p.look_ahead(1, |t| t.is_interpolated());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have any tests covering this interpolated case? I expected to see one where we substitute in a $t:ty.

It is a bit of a shame that we have no $p:path fragment specificier, though I guess rust-lang/rfcs#1575 would address the use case.

@nikomatsakis nikomatsakis mentioned this pull request Apr 27, 2016
Closed
@nikomatsakis
Copy link
Contributor

@jseyfried

Is this the type of test you had in mind?

Yes, and maybe also having pub($t) in the expansion?

@jseyfried
Copy link
Contributor Author

also having pub($t) in the expansion?

Good point, will do.

@nikomatsakis
Copy link
Contributor

r=me w/ a few more tests

also, @jseyfried, you can I think use the revisions mechanism if you'd prefer to have multiple errors per file. You have to add

// revisions: a b c

then write code with #[cfg(a)] etc. You can then specify the expected error as

//[a] error-pattern:foo

or //[a]~ ERROR foo.

@nikomatsakis
Copy link
Contributor

(Though maybe it doesn't work with that class of test yet)

@jseyfried
Copy link
Contributor Author

use the revisions mechanism if you'd prefer to have multiple errors per file

Interesting, I'll give it a try.

@jseyfried
Copy link
Contributor Author

The revisions mechanism won't work for parse errors since we always parse and expand unconfigured items (cf my comment in #25544).

@jseyfried jseyfried force-pushed the parse_tuple_struct_field_vis branch from 1bfc50b to d2ca7d0 Compare April 27, 2016 22:03
@jseyfried jseyfried force-pushed the parse_tuple_struct_field_vis branch from d2ca7d0 to 78a8127 Compare April 27, 2016 23:42
@jseyfried
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Collaborator

bors commented Apr 27, 2016

📌 Commit 78a8127 has been approved by nikomatsakis

@bors
Copy link
Collaborator

bors commented Apr 28, 2016

⌛ Testing commit 78a8127 with merge 0f9ba99...

bors added a commit that referenced this pull request Apr 28, 2016
@bors
Auto merge of #33161 - jseyfried:parse_tuple_struct_field_vis, r=niko…
0f9ba99 
…matsakis

Parse `pub(restricted)` visibilities on tuple struct fields

Parse `pub(restricted)` on tuple struct fields (cc #32409).

r? @nikomatsakis
@bors
Copy link
Collaborator

bors commented Apr 28, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@bors bors merged commit 78a8127 into rust-lang:master Apr 28, 2016
@jseyfried jseyfried deleted the parse_tuple_struct_field_vis branch May 31, 2016 23:13
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jseyfried @petrochenkov @bors @nikomatsakis