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

Resolve $crates for pretty-printing at more appropriate time #57155

Merged
merged 2 commits into from
Dec 28, 2018

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Dec 27, 2018

Doing it in BuildReducedGraphVisitor wasn't a good idea, identifiers wasn't actually visited half of the time.
As a result some $crates weren't resolved and were therefore pretty-printed as $crate literally, which turns into two tokens during re-parsing of the pretty-printed text.

Now we are visiting and resolving $crate identifiers in an item right before sending that item to a proc macro attribute or derive.

Fixes #57089

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 27, 2018
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Dec 27, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Dec 27, 2018

📌 Commit e40d7d9 has been approved by dtolnay

@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 Dec 27, 2018
@bors
Copy link
Contributor

bors commented Dec 28, 2018

⌛ Testing commit e40d7d9 with merge e8ca35e...

bors added a commit that referenced this pull request Dec 28, 2018
Resolve `$crate`s for pretty-printing at more appropriate time

Doing it in `BuildReducedGraphVisitor` wasn't a good idea, identifiers wasn't actually visited half of the time.
As a result some `$crate`s weren't resolved and were therefore pretty-printed as `$crate` literally, which turns into two tokens during re-parsing of the pretty-printed text.

Now we are visiting and resolving `$crate` identifiers in an item right before sending that item to a proc macro attribute or derive.

Fixes #57089
@bors
Copy link
Contributor

bors commented Dec 28, 2018

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

@bors bors merged commit e40d7d9 into rust-lang:master Dec 28, 2018
@grovesNL
Copy link
Contributor

grovesNL commented Jan 19, 2019

@petrochenkov @dtolnay I noticed an issue with macro expansion with -Z unstable-options --pretty=expanded including $crate in the output for the bitflags crate/macro, which is unexpected.

The issue started somewhere between nightlies 2018-12-27-fb86d604b and 2018-12-28-60e825389 which seems to be when this landed, though I haven't bisected further yet. It is possible this change is related?

I have an example here: https://gist.github.com/grovesNL/5d3ddc7b95c1939be26c64ba09f1ac2f

(Original issue was reported in mozilla/cbindgen#272)

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Jan 19, 2019

@grovesNL
Yes, this PR or #56647 is the most likely reason.

Does cbindgen really need to parse output of --pretty=expanded as Rust code?
In general it's not guaranteed to be well-formed, and certainly not guaranteed to be correct (due to macro hygiene).

@grovesNL
Copy link
Contributor

It's not clear if there is a better alternative at the moment – it's definitely very useful to have the expanded macro source for use cases like cbindgen.

I guess cbindgen could special case certain "known" macros in cbindgen (i.e. bitflags and others) or the downstream crates using cbindgen could manually expand macros. Both of these options are a bit problematic, because it would mean cbindgen is unable to work with most macros in general.

@petrochenkov
Copy link
Contributor Author

Ok, I'll try to fix this specific case since it worked before.

@grovesNL
Copy link
Contributor

Thanks! Please note I also don’t mean to request a fix here if this use case isn’t supported, because it would inevitably break again at some point. I’m just trying to understand how we should go about making this work, even if it means reworking cbindgen heavily.

#43364 discusses a long term fix a bit, but there doesn’t seem to be clear consensus yet.

@petrochenkov
Copy link
Contributor Author

Fixed in #57915

Centril added a commit to Centril/rust that referenced this pull request Jan 28, 2019
Pretty print `$crate` as `crate` or `crate_name` in more cases

So, people do parse output of `--pretty=expanded` (sigh), so covering only the legacy proc-macro case (like it was done in rust-lang#57155) is not enough.

This PRs resolves all `$crate`s produced by macros, so they are all printed in the parseable form `$crate::foo` -> `crate::foo` or `crate_name::foo`.

Fixes rust-lang#38016 (comment)
Fixes rust-lang#57155 (comment)
Centril added a commit to Centril/rust that referenced this pull request Jan 28, 2019
Pretty print `$crate` as `crate` or `crate_name` in more cases

So, people do parse output of `--pretty=expanded` (sigh), so covering only the legacy proc-macro case (like it was done in rust-lang#57155) is not enough.

This PRs resolves all `$crate`s produced by macros, so they are all printed in the parseable form `$crate::foo` -> `crate::foo` or `crate_name::foo`.

Fixes rust-lang#38016 (comment)
Fixes rust-lang#57155 (comment)
@petrochenkov petrochenkov deleted the dcrate3 branch June 5, 2019 16:24
Centril added a commit to Centril/rust that referenced this pull request Jul 10, 2019
…ulacrum

Fix pretty-printing of `$crate` (take 4)

Pretty-print `$crate` as `crate` or `crate_name` in unstructured tokens like `a $crate c` in `foo!(a $crate c)`, but only if those tokens are printed as a part of AST pretty-printing, rather than as a standalone token stream.

Fixes rust-lang#62325
Previous iterations - rust-lang#56647, rust-lang#57155, rust-lang#57915.
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.

$crate unexpectedly disintegrates depending on unrelated macro invocations
5 participants