-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Fix, document, and test parser and pretty-printer edge cases related to braced macro calls #119427
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
584de16
to
48ac127
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I agree with the idea, but I'm not sure about the implementation. Is there a way to make this nicer?
| TryBlock(..) | ||
| ConstBlock(..) => false, | ||
|
||
MacCall(mac_call) => mac_call.args.delim != Delimiter::Brace, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed, if all the call sites are not calling this if e ≈ MacCall
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the commit that adds this MacCall
case inside expr_requires_semi_to_be_stmt
, I change all the call sites to preserve their pre-existing behavior. So this case starts out as not reachable. But after that, one at a time for each call site, I update each call site that should be reaching this, together with adding documentation and tests for that call site.
☔ The latest upstream changes (presumably #120121) made this pull request unmergeable. Please resolve the merge conflicts. |
r? compiler |
#120690 |
This is waiting on me to incorporate Waffle's insightful feedback, and to rebase. |
Give a name to each distinct manipulation of pretty-printer FixupContext There are only 7 distinct ways that the AST pretty-printer interacts with FixupContext: 3 constructors (including Default), 2 transformations, and 2 queries. This PR turns these into associated functions which can be documented with examples. This PR unblocks rust-lang#119427 (comment). In order to improve the pretty-printer's behavior regarding parenthesization of braced macro calls in match arms, which have different grammar than macro calls in statements, FixupContext needs to be extended with 2 new fields. In the previous approach, that would be onerous. In the new approach, all it entails is 1 new constructor (`FixupContext::new_match_arm()`).
Give a name to each distinct manipulation of pretty-printer FixupContext There are only 7 distinct ways that the AST pretty-printer interacts with FixupContext: 3 constructors (including Default), 2 transformations, and 2 queries. This PR turns these into associated functions which can be documented with examples. This PR unblocks rust-lang#119427 (comment). In order to improve the pretty-printer's behavior regarding parenthesization of braced macro calls in match arms, which have different grammar than macro calls in statements, FixupContext needs to be extended with 2 new fields. In the previous approach, that would be onerous. In the new approach, all it entails is 1 new constructor (`FixupContext::new_match_arm()`).
Rollup merge of rust-lang#124191 - dtolnay:fixup, r=compiler-errors Give a name to each distinct manipulation of pretty-printer FixupContext There are only 7 distinct ways that the AST pretty-printer interacts with FixupContext: 3 constructors (including Default), 2 transformations, and 2 queries. This PR turns these into associated functions which can be documented with examples. This PR unblocks rust-lang#119427 (comment). In order to improve the pretty-printer's behavior regarding parenthesization of braced macro calls in match arms, which have different grammar than macro calls in statements, FixupContext needs to be extended with 2 new fields. In the previous approach, that would be onerous. In the new approach, all it entails is 1 new constructor (`FixupContext::new_match_arm()`).
@rustbot ready |
Thanks for the PR, @dtolnay, looks great! However, after looking through all the commits, I think I don't understand enough about the parsing subtleties involved here to review this. r? parser |
…lse' The change to the test is a little goofy because the compiler was guessing "correctly" before that `falsy! {}` is the condition as opposed to the else body. But I believe this change is fundamentally correct. Braced macro invocations in statement position are most often item-like (`thread_local! {...}`) as opposed to parenthesized macro invocations which are condition-like (`cfg!(...)`).
It is impossible for expr here to be a braced macro call. Expr comes from `parse_stmt_without_recovery`, in which macro calls are parsed by `parse_stmt_mac`. See this part: let kind = if (style == MacStmtStyle::Braces && self.token != token::Dot && self.token != token::Question) || self.token == token::Semi || self.token == token::Eof { StmtKind::MacCall(P(MacCallStmt { mac, style, attrs, tokens: None })) } else { // Since none of the above applied, this is an expression statement macro. let e = self.mk_expr(lo.to(hi), ExprKind::MacCall(mac)); let e = self.maybe_recover_from_bad_qpath(e)?; let e = self.parse_expr_dot_or_call_with(e, lo, attrs)?; let e = self.parse_expr_assoc_with( 0, LhsExpr::AlreadyParsed { expr: e, starts_statement: false }, )?; StmtKind::Expr(e) }; A braced macro call at the head of a statement is always either extended into ExprKind::Field / MethodCall / Await / Try / Binary, or else returned as StmtKind::MacCall. We can never get a StmtKind::Expr containing ExprKind::MacCall containing brace delimiter.
|
self.restrictions.contains(Restrictions::STMT_EXPR) | ||
&& !classify::expr_requires_semi_to_be_stmt(e) | ||
&& !classify::expr_requires_comma_to_be_match_arm(e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is maybe my only nit in the PR: that expr_is_complete
uses expr_requires_comma_to_be_match_arm
"feels" a bit less self-descriptive than it could be, since we use expr_is_complete
for things like parse_expr_dot_or_call_with_
which doesn't really have to do with match arms.
Not really sure how to make this actionable though, since I'm not sure if a rename seems tough. AFAICT this just preserves the old behavior, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is preserving old behavior. The exact same behavior that used to be incorrectly called expr_requires_semi_to_be_stmt
is now called expr_requires_comma_to_be_match_arm
.
Regarding naming, one possibility is that instead of distinguishing a "commas in match arms" and "semicolons in statements" case (and the implicit 3rd case, expressions that are neither statement nor match arm, such as a function arg), we could rename things along the following lines:
- "parse the longest possible expression"
- "parse an expression using earlier boundary rule" (for a match arm, range, or whatever else looks at expr_is_complete)
- "parse an expression until the earliest possible boundary" (for a statement)
Another possibility is to omit this specific commit ("Add classify::expr_requires_comma_to_be_match_arm") and keep using match e { MacCall(_) => …, _ => expr_requires_semi_to_be_stmt(e) }
in the cases where expr_requires_semi_to_be_stmt
is not the desired behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more option — instead of having what's currently in the PR:
// compiler/rustc_ast/src/util/classify.rs
pub fn expr_requires_semi_to_be_stmt(e: &ast::Expr) -> bool {
match &e.kind {
If(..)
| Match(..)
| Block(..)
| While(..)
| Loop(..)
| ForLoop { .. }
| TryBlock(..)
| ConstBlock(..) => false,
MacCall(mac_call) => mac_call.args.delim != Delimiter::Brace,
_ => true,
}
}
pub fn expr_requires_comma_to_be_match_arm(e: &ast::Expr) -> bool {
match &e.kind {
MacCall(_) => true,
_ => expr_requires_semi_to_be_stmt(e),
}
}
we could have:
pub fn expr_is_complete(e: &ast::Expr) -> bool {
// this is negative of the old "expr_requires_semi_to_be_stmt"
match &e.kind {
If(..)
| Match(..)
| Block(..)
| While(..)
| Loop(..)
| ForLoop { .. }
| TryBlock(..)
| ConstBlock(..) => true,
MacCall(_) | _ => false,
}
}
pub fn expr_requires_semi_to_be_stmt(e: &ast::Expr) -> bool {
match &e.kind {
MacCall(mac_call) => mac_call.args.delim != Delimiter::Brace,
_ => !expr_is_complete(e),
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer either the final option or the renaming you suggested first above. I'll leave it up to you. Otherwise this PR looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🆒
@bors r+ |
This comment has been minimized.
This comment has been minimized.
@bors r=compiler-errors |
☀️ Test successful - checks-actions |
Finished benchmarking commit (8cc6f34): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 675.636s -> 678.17s (0.38%) |
Review note: this is a deceptively small PR because it comes with 145 lines of docs and 196 lines of tests, and only 25 lines of compiler code changed. However, I recommend reviewing it 1 commit at a time because much of the effect of the code changes is non-local i.e. affecting code that is not visible in the final state of the PR. I have paid attention that reviewing the PR one commit at a time is as easy as I can make it. All of the code you need to know about is touched in those commits, even if some of those changes disappear by the end of the stack.
This is a follow-up to #119105. One case that is not relevant to
-Zunpretty=expanded
, but which came up as I'm porting #119105 and #118726 intosyn
's printer andprettyplease
's printer where it is relevant, and is also relevant to rustc'sstringify!
, is statement boundaries in the vicinity of braced macro calls.Rustc's AST pretty-printer produces invalid syntax for statements that begin with a braced macro call:
Before this PR: output is not valid Rust syntax.
fn main() { m! {} + 1; }
After this PR: valid syntax.
fn main() { (m! {}) + 1; }