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

Pretty-printing adds additional parentheses, breaking the proc-macro reparse check #75734

Closed
Aaron1011 opened this issue Aug 20, 2020 · 3 comments · Fixed by #77135
Closed

Pretty-printing adds additional parentheses, breaking the proc-macro reparse check #75734

Aaron1011 opened this issue Aug 20, 2020 · 3 comments · Fixed by #77135
Labels
A-pretty Area: Pretty printing (including `-Z unpretty`) A-proc-macros Area: Procedural macros

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Aug 20, 2020

The following code:

fn main() {
    &|_: u8| {};
}

is pretty-printed as:

fn main() { &(|_: u8| { }); }

The extra parenthesis completely change the structure of the re-parsed TokenStream (we now have tokens wrapped in a TokenTree::Delimited), breaking the pretty-print and reparse check done during proc-macro expansion. This results in an instance of #43081.

This behavior was introduced by #43742. The PR description states that is intended to "make pprust easier to use with programmatically constructed ASTs." - however, I can't find a concrete example of code that is affected by this.

I see a few possible ways of fixing this:

  1. Try to ignore this type of 'unnecessary' parentheses when we compare the original and re-parsed tokens. This seems extremely difficult, since we can't know which parenthesis are necessary without actually parsing the TokenStream.
  2. Skip inserting these extra parentheses during pretty-printing for proc-macro expansion, but insert them during all other pretty-printing. This should work correctly, but represents yet another hack needed by the pretty-print/reparsing code.
  3. Partially revert pprust: fix parenthesization of exprs #43742 - we would still insert parenthesis when pretty-printing the HIR, but we would rely solely on ExprKind::Paren when pretty-printing the AST. I'm not exactly certain what the consequences of doing this are. Do we ever end up pretty-printing code-generated AST nodes (e.g. from builtin macros), other than when explicitly requested via -Z unpretty=expanded?

If we could get it to work without breaking anything, solution 3 seems like the cleanest way of solving this.

cc @petrochenkov

@Aaron1011 Aaron1011 added A-proc-macros Area: Procedural macros A-pretty Area: Pretty printing (including `-Z unpretty`) labels Aug 20, 2020
@dtolnay
Copy link
Member

dtolnay commented Aug 20, 2020

Also causes dtolnay/async-trait#118. Rustc inserted unnecessary parentheses around the closure before calling async_trait, then lost all the location information on the source tokens and blamed the user for handwriting those unnecessary parentheses.

@petrochenkov
Copy link
Contributor

If we could get it to work without breaking anything

It would break expressions with "virtual parentheses", like $expr in macro_rules.
$expr * 2 where $expr is 2 + 2 is not the same thing as 2 + 2 * 2, which would be the pretty-printing result if it weren't adding parentheses.

@petrochenkov
Copy link
Contributor

Cases like this could be caught when comparing the original and re-parsed token streams though, because the "virtual parentheses" (Delimiter::None) could still be in place in the original token stream.

So we can print without parentheses -> reparse -> compare, and then re-print with parentheses if the comparison fails.
(This may make sense if failing comparisons are rare and don't affect performance.)

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 11, 2020
…etrochenkov

Attach tokens to all AST types used in `Nonterminal`

We perform token capturing when we have outer attributes (for nonterminals that support attributes - e.g. `Stmt`), or when we parse a `Nonterminal` for a `macro_rules!` argument. The full list of `Nonterminals` affected by this PR is:

* `NtBlock`
* `NtStmt`
* `NtTy`
* `NtMeta`
* `NtPath`
* `NtVis`
* `NtLiteral`

Of these nonterminals, only `NtStmt` and `NtLiteral` (which is actually just an `Expr`), support outer attributes - the rest only ever have token capturing perform when they match a `macro_rules!` argument.

This makes progress towards solving rust-lang#43081 - we now collect tokens for everything that might need them. However, we still need to handle `#[cfg]`, inner attributes, and misc pretty-printing issues (e.g. rust-lang#75734)

I've separated the changes into (mostly) independent commits, which could be split into individual PRs for each `Nonterminal` variant. The purpose of having them all in one PR is to do a single Crater run for all of them.

Most of the changes in this PR are trivial (adding `tokens: None` everywhere we construct the various AST structs). The significant changes are:

* `ast::Visibility` is changed from `type Visibility = Spanned<VisibilityKind>` to a `struct Visibility { kind, span, tokens }`.
* `maybe_collect_tokens` is made generic, and used for both `ast::Expr` and `ast::Stmt`.
* Some of the statement-parsing functions are refactored so that we can capture the trailing semicolon.
* `Nonterminal` and `Expr` both grew by 8 bytes, as some of the structs which are stored inline (rather than behind a `P`) now have an `Option<TokenStream>` field. Hopefully the performance impact of doing this is negligible.
flip1995 pushed a commit to flip1995/rust that referenced this issue Sep 24, 2020
…etrochenkov

Attach tokens to all AST types used in `Nonterminal`

We perform token capturing when we have outer attributes (for nonterminals that support attributes - e.g. `Stmt`), or when we parse a `Nonterminal` for a `macro_rules!` argument. The full list of `Nonterminals` affected by this PR is:

* `NtBlock`
* `NtStmt`
* `NtTy`
* `NtMeta`
* `NtPath`
* `NtVis`
* `NtLiteral`

Of these nonterminals, only `NtStmt` and `NtLiteral` (which is actually just an `Expr`), support outer attributes - the rest only ever have token capturing perform when they match a `macro_rules!` argument.

This makes progress towards solving rust-lang#43081 - we now collect tokens for everything that might need them. However, we still need to handle `#[cfg]`, inner attributes, and misc pretty-printing issues (e.g. rust-lang#75734)

I've separated the changes into (mostly) independent commits, which could be split into individual PRs for each `Nonterminal` variant. The purpose of having them all in one PR is to do a single Crater run for all of them.

Most of the changes in this PR are trivial (adding `tokens: None` everywhere we construct the various AST structs). The significant changes are:

* `ast::Visibility` is changed from `type Visibility = Spanned<VisibilityKind>` to a `struct Visibility { kind, span, tokens }`.
* `maybe_collect_tokens` is made generic, and used for both `ast::Expr` and `ast::Stmt`.
* Some of the statement-parsing functions are refactored so that we can capture the trailing semicolon.
* `Nonterminal` and `Expr` both grew by 8 bytes, as some of the structs which are stored inline (rather than behind a `P`) now have an `Option<TokenStream>` field. Hopefully the performance impact of doing this is negligible.
@bors bors closed this as completed in 4ba5068 Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pretty Area: Pretty printing (including `-Z unpretty`) A-proc-macros Area: Procedural macros
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants