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

crate-ify and delete unused code from syntax::parse #51265

Merged
merged 1 commit into from
Jun 10, 2018

Conversation

Mark-Simulacrum
Copy link
Member

This is intended primarily to ensure the compiler catches dead code for us in
more cases.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 1, 2018
@rust-highfive

This comment has been minimized.

@Mark-Simulacrum Mark-Simulacrum force-pushed the cleanup-syntax-parse branch 2 times, most recently from 9ade7c8 to 7ff11e3 Compare June 1, 2018 02:50
@rust-highfive

This comment has been minimized.

@eddyb
Copy link
Member

eddyb commented Jun 4, 2018

r? @jseyfried @petrochenkov or @pnkfelix

@rust-highfive rust-highfive assigned jseyfried and unassigned eddyb Jun 4, 2018
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jun 9, 2018

📌 Commit 0b4f8e8 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 9, 2018
@bors
Copy link
Contributor

bors commented Jun 9, 2018

🔒 Merge conflict

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 9, 2018
@bors
Copy link
Contributor

bors commented Jun 9, 2018

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

@Mark-Simulacrum
Copy link
Member Author

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jun 9, 2018

📌 Commit 60058e5 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 9, 2018
@bors
Copy link
Contributor

bors commented Jun 9, 2018

⌛ Testing commit 60058e5 with merge ae0659c...

bors added a commit that referenced this pull request Jun 9, 2018
…henkov

crate-ify and delete unused code from syntax::parse

This is intended primarily to ensure the compiler catches dead code for us in
more cases.
@bors
Copy link
Contributor

bors commented Jun 10, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing ae0659c to master...

@topecongiro
Copy link
Contributor

Is it possible to make some metods public again? I find that the changes introduced in this PR break a few number of tools and libraries that depend on libsyntax.

@Mark-Simulacrum
Copy link
Member Author

Hm, I somewhat expected that tools/libraries like Rocket should mostly be using proc macro's these days, not libsyntax directly. If there's a list of methods/types that need to be made public, I'd be happy to review a PR which makes them pub again.

@Mark-Simulacrum Mark-Simulacrum deleted the cleanup-syntax-parse branch June 11, 2018 13:59
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 11, 2018
…ochenkov

Make parse_ident public

`parse_ident` was made private in rust-lang#51265. In rustfmt the method is used to create a custom parser for macro call.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 11, 2018
…Simulacrum

Make parse_seq_to_end and parse_path public

(see rwf2/Rocket#660, rust-lang#51265)

Rocket currently uses `parse_seq_to_end` and `parse_path` in its codegen macros. Assuming I tested correctly, this is the minimal set of methods that are currently necessary to build Rocket again. I would be happy to add documentation of this and Rocket's other usages, if desired.
PSeitz added a commit to PSeitz/rust that referenced this pull request Jun 11, 2018
span_fatal and parse_block  were made private in rust-lang#51265. These methods are used in stainless.

Related rust-lang#51498 rust-lang#51504
bors added a commit that referenced this pull request Jun 12, 2018
Make parse_ident public

`parse_ident` was made private in #51265. In rustfmt the method is used to create a custom parser for macro call.
bors added a commit that referenced this pull request Jun 12, 2018
Make span_fatal and parse_block public

span_fatal and parse_block  were made private in #51265. These methods are used in stainless.

Related #51498 #51504
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 12, 2018
…Simulacrum

Make parse_seq_to_end and parse_path public

(see rwf2/Rocket#660, rust-lang#51265)

Rocket currently uses `parse_seq_to_end` and `parse_path` in its codegen macros. Assuming I tested correctly, this is the minimal set of methods that are currently necessary to build Rocket again. I would be happy to add documentation of this and Rocket's other usages, if desired.
@tikue
Copy link
Contributor

tikue commented Jun 22, 2018

What about unstable proc macros that relied on these fns?

@Mark-Simulacrum
Copy link
Member Author

I believe the expectation is that even unstable proc macros should be using syn/quote or other libraries for the purpose of parsing Rust code and generating Rust code; they should not be using libsyntax. That is, proc macros shouldn't use #![feature(rustc_private)]. In the specific case of tarpc, it looks like it's using the older plugin registrar functionality, not proc macros -- it should be migrated to proc macros, since those will stabilize far earlier I imagine.

@tikue
Copy link
Contributor

tikue commented Jun 22, 2018

@Mark-Simulacrum Thanks for the quick response. It's been tough to follow the state of proc macros, so I haven't updated things in a while. Last time I tried I ran into cross-crate problems, and the bug I filed, #44817, hasn't been fixed yet. Do you mind giving me a quick pointer to what I should be doing right now?

@Mark-Simulacrum
Copy link
Member Author

I don't believe that there's a way to fix #44817 with proc macros quite yet, and I believe we're unprepared to stabilize anything "hygienic" at this point. For the time being, if you want to fix the issue opened on tarpc I'll approve PRs that re-pub the relevant APIs; long term I imagine there will be a better solution for you. I also haven't looked at how the syntax extensions in tarpc are implemented, it's possible that we already have sufficient functionality for them. If you want, you might be able to get people more familiar with that feature to help on internals or IRC.

@tikue
Copy link
Contributor

tikue commented Jun 22, 2018

@Mark-Simulacrum I tried to get it working with syn but am running into issues. I think the main problem is actually that things like $crate::futures::InfoFuture aren't expanded before being passed to the proc macro, and syn can't parse things like that.

@Mark-Simulacrum
Copy link
Member Author

It's probably best to move this to internals.rust-lang.org, but that feels odd to me. I'd expect syn to be able to parse proc macro input -- including $crate, if that's possible in proc macros.

@tikue
Copy link
Contributor

tikue commented Jun 22, 2018

Ok, so the real problem -- which doesn't seem to have changed since I last tried to move off of plugins -- is that I can't call a proc_macro from inside a macro_rules macro.

error[E0433]: failed to resolve. Did you mean `tarpc::util`?
   --> <_snake_to_camel macros>:2:1
    |
1   |     / ( type $ i : ident = $ t : ty ; ) => {
2   |     | $ crate :: impl_snake_to_camel ! ( type $ i = $ t ; ) ; } ; (
    |     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |     | |
    |     | Did you mean `tarpc::util`?

I tried to reexport that proc macro from the tarpc crate root, but doing so doesn't make it visible to the macros declared in tarpc.

If there's no way to do this, then I can't migrate to proc_macro at this time, and would prefer to roll back this PR.

bors added a commit that referenced this pull request Jun 23, 2018
Re-pub some items whose visibilities were recently reduced.

Reasons described in the most recent comments of #51265.

tarpc can't move off of plugins until proc macros can be reexported from other crates.

Fixes google/tarpc#191
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants