-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Insert parentheses around binary operation with attribute #142476
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
base: master
Are you sure you want to change the base?
Changes from all commits
1ed0cbf
6008f48
29b5058
cf3981d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -386,18 +386,43 @@ impl<'a> State<'a> { | |
|
||
let ib = self.ibox(INDENT_UNIT); | ||
|
||
// The Match subexpression in `match x {} - 1` must be parenthesized if | ||
// it is the leftmost subexpression in a statement: | ||
// | ||
// (match x {}) - 1; | ||
// | ||
// But not otherwise: | ||
// | ||
// let _ = match x {} - 1; | ||
// | ||
// Same applies to a small set of other expression kinds which eagerly | ||
// terminate a statement which opens with them. | ||
let needs_par = fixup.would_cause_statement_boundary(expr); | ||
let needs_par = { | ||
// The Match subexpression in `match x {} - 1` must be parenthesized | ||
// if it is the leftmost subexpression in a statement: | ||
// | ||
// (match x {}) - 1; | ||
// | ||
// But not otherwise: | ||
// | ||
// let _ = match x {} - 1; | ||
// | ||
// Same applies to a small set of other expression kinds which | ||
// eagerly terminate a statement which opens with them. | ||
fixup.would_cause_statement_boundary(expr) | ||
} || { | ||
// If a binary operation ends up with an attribute, such as | ||
// resulting from the following macro expansion, then parentheses | ||
// are required so that the attribute encompasses the right | ||
// subexpression and not just the left one. | ||
// | ||
// #![feature(stmt_expr_attributes)] | ||
// | ||
// macro_rules! add_attr { | ||
// ($e:expr) => { #[attr] $e }; | ||
// } | ||
// | ||
// let _ = add_attr!(1 + 1); | ||
// | ||
// We must pretty-print `#[attr] (1 + 1)` not `#[attr] 1 + 1`. | ||
!attrs.is_empty() | ||
&& matches!( | ||
expr.kind, | ||
ast::ExprKind::Binary(..) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes me slightly uneasy that we have to "hard code" a collection of expr kinds separately here but rn I don't see a more robust alternative that's only based on (effective) precedence levels. |
||
| ast::ExprKind::Cast(..) | ||
| ast::ExprKind::Assign(..) | ||
| ast::ExprKind::AssignOp(..) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assortment of expr kinds is lacking macro_rules! attach { ($e:expr) => { #[allow()] $e }
fn main() { _ = attach!(0..1); } Current output is As for ranges with no lower bound |
||
) | ||
}; | ||
if needs_par { | ||
self.popen(); | ||
fixup = FixupContext::default(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,6 +92,18 @@ static EXPRS: &[&str] = &[ | |
"#[attr] loop {}.field", | ||
"(#[attr] loop {}).field", | ||
"loop { #![attr] }.field", | ||
// Attributes on a Binary, Cast, Assign, and AssignOp expression require | ||
// parentheses. | ||
"#[attr] (1 + 1)", | ||
"#[attr] (1 as T)", | ||
"#[attr] (x = 1)", | ||
"#[attr] (x += 1)", | ||
// If the attribute were not present on the binary operation, it would be | ||
// legal to render this without not just the inner parentheses, but also the | ||
// outer ones. `return x + .. .field` (Yes, really.) Currently the | ||
// pretty-printer does not take advantage of this edge case. | ||
Comment on lines
+101
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes total sense and it's still insane! |
||
"(return #[attr] (x + ..)).field", | ||
"(return x + ..).field", | ||
// Grammar restriction: break value starting with a labeled loop is not | ||
// allowed, except if the break is also labeled. | ||
"break 'outer 'inner: loop {} + 2", | ||
|
@@ -158,7 +170,12 @@ struct Unparenthesize; | |
impl MutVisitor for Unparenthesize { | ||
fn visit_expr(&mut self, e: &mut Expr) { | ||
while let ExprKind::Paren(paren) = &mut e.kind { | ||
let paren_attrs = mem::take(&mut e.attrs); | ||
*e = mem::replace(paren, Expr::dummy()); | ||
if !paren_attrs.is_empty() { | ||
assert!(e.attrs.is_empty()); | ||
e.attrs = paren_attrs; | ||
} | ||
} | ||
mut_visit::walk_expr(self, 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.
The explanation you've given in https://github.com/rust-lang/rust/pull/134661/files#r2143009318 makes perfect sense, thanks!