Resolve `$crate`s for pretty-printing at more appropriate time by petrochenkov · Pull Request #57155 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation11 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

Choose a reason for hiding this comment

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

Thanks!

📌 Commit e40d7d9 has been approved by dtolnay

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 added a commit that referenced this pull request

Dec 28, 2018

Resolve $crates 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 $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

ehuss mentioned this pull request

Jan 1, 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)

@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).

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.

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

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.

This was referenced

Jan 20, 2019

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 $crates 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 $crates 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

Jul 10, 2019

…ulacrum

Fix pretty-printing of $crate (take 4)

Pretty-print $crate as crate or crate_name in unstructured tokens like a <math xmlns="http://www.w3.org/1998/Math/MathML"><semantics><mrow><mi>c</mi><mi>r</mi><mi>a</mi><mi>t</mi><mi>e</mi><mi>c</mi><mi mathvariant="normal">‘</mi><mi>i</mi><mi>n</mi><mi mathvariant="normal">‘</mi><mi>f</mi><mi>o</mi><mi>o</mi><mo stretchy="false">!</mo><mo stretchy="false">(</mo><mi>a</mi></mrow><annotation encoding="application/x-tex">crate c in foo!(a </annotation></semantics></math>cratec‘in‘foo!(acrate 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.

Labels

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Resolve $crates for pretty-printing at more appropriate time by petrochenkov · Pull Request #57155 · rust-lang/rust (original) (raw)

Conversation

Choose a reason for hiding this comment

Resolve `$crate`s for pretty-printing at more appropriate time by petrochenkov · Pull Request #57155 · rust-lang/rust (original) (raw)