Skip to content

Reduce precedence of expressions that have an outer attr #134661

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+164 −49
Open

Reduce precedence of expressions that have an outer attr #134661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d75b0ae
Add AST pretty-printer tests involving attr precedence
dtolnay Jun 13, 2025
2171f89
Reduce precedence of expressions that have an outer attr
dtolnay Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions compiler/rustc_ast/src/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1450,11 +1450,20 @@ impl Expr {
}

pub fn precedence(&self) -> ExprPrecedence {
fn prefix_attrs_precedence(attrs: &AttrVec) -> ExprPrecedence {
for attr in attrs {
if let AttrStyle::Outer = attr.style {
return ExprPrecedence::Prefix;
Copy link
Member

@fmease fmease Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think ExprPrecedence::Prefix isn't low enough. Consider the following cases involving binops:

#![feature(stmt_expr_attributes)]

macro_rules! group { ($e:expr) => { $e } }
macro_rules! extra { ($e:expr) => { #[allow()] $e } }

fn main() {                             // `-Zunpretty=expanded` on master & prefixattr:

    let _ = #[allow()] 1 + 1;           // let _ = #[allow()] 1 + 1;
    let _ = group!(#[allow()] 1) + 1;   // let _ = #[allow()] 1 + 1;
    let _ = 1 + group!(#[allow()] 1);   // let _ = 1 + #[allow()] 1;

    let _ = extra!({ 0 }) + 1;          // let _ = #[allow()] { 0 } + 1;
    let _ = extra!({ 0 } + 1);          // let _ = #[allow()] { 0 } + 1;
}

I haven't given it that much thought yet, so I don't know which specific precedence level(s) would make sense instead.

And indeed, this example is heavily inspired by the ones found in #15701 / #127436. So maybe answering this pretty-printing question is partially blocked by the open design question (meaning we can ignore it for now).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that bug is orthogonal to this one.

The bug fixed by this PR is about when an outer expression A contains a subexpression B where B contains an attribute, and in the prettyprinter-produced code the attribute ended up applying to parts of A instead of only to B. This is fixed by having A observe a different effective precedence for its subexpression B, making A generate parentheses around the subexpression containing the attribute, i.e. the attribute will end up inside the parentheses.

In contrast, the bug you have identified is about an outer expression A that has an attribute, and a subexpression B that has no attribute. In this case when parentheses are inserted, the attribute will end up outside the parentheses.

I will fix that as well but I expect it will involve unrelated code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix: #142476

}
}
ExprPrecedence::Unambiguous
}

match &self.kind {
ExprKind::Closure(closure) => {
match closure.fn_decl.output {
FnRetTy::Default(_) => ExprPrecedence::Jump,
FnRetTy::Ty(_) => ExprPrecedence::Unambiguous,
FnRetTy::Ty(_) => prefix_attrs_precedence(&self.attrs),
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Now with #134847, you also need to use prefix_attrs_precedence() in the "Break | Ret | Yield | Yeet" branch where value is None. For cases like group!(#[allow()] return).await. (GH doesn't let me comment there)

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. Great catch.

Expand All @@ -1463,7 +1472,7 @@ impl Expr {
| ExprKind::Yield(YieldKind::Prefix(value))
| ExprKind::Yeet(value) => match value {
Some(_) => ExprPrecedence::Jump,
None => ExprPrecedence::Unambiguous,
None => prefix_attrs_precedence(&self.attrs),
},

ExprKind::Become(_) => ExprPrecedence::Jump,
Expand All @@ -1490,7 +1499,7 @@ impl Expr {
| ExprKind::Let(..)
| ExprKind::Unary(..) => ExprPrecedence::Prefix,

// Never need parens
// Need parens if and only if there are prefix attributes.
ExprKind::Array(_)
| ExprKind::Await(..)
| ExprKind::Use(..)
Expand Down Expand Up @@ -1525,7 +1534,7 @@ impl Expr {
| ExprKind::While(..)
| ExprKind::Yield(YieldKind::Postfix(..))
| ExprKind::Err(_)
| ExprKind::Dummy => ExprPrecedence::Unambiguous,
| ExprKind::Dummy => prefix_attrs_precedence(&self.attrs),
}
}

Expand Down
21 changes: 16 additions & 5 deletions compiler/rustc_hir/src/hir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2285,12 +2285,23 @@ pub struct Expr<'hir> {
}

impl Expr<'_> {
pub fn precedence(&self) -> ExprPrecedence {
pub fn precedence(
&self,
for_each_attr: &dyn Fn(HirId, &mut dyn FnMut(&Attribute)),
Copy link
Member

Choose a reason for hiding this comment

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

This could instead be a &dyn Fn(&mut dyn FnMut(&Attribute)) (dropping the HirId) since any of the precedence wrappers have naturally access to expr and thus expr.hir_id. Not sure if cleaner 🤷

Copy link
Member

Choose a reason for hiding this comment

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

The double indirection is quite something but I guess it's no worse than <'a> &dyn Fn() -> Box<dyn Iterator<Item = &'a Attribute> + 'a>

Copy link
Member Author

Choose a reason for hiding this comment

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

The HirId argument is here because for_each_attr needs to be able to iterate not only self's attributes, but potentially some other Expr's attributes in the ExprKind::DropTemps case.

If the HirId were dropped from for_each_attr, the callers would need to change like this:

-    fn precedence(&self, expr: &hir::Expr<'_>) -> ExprPrecedence {
-        let for_each_attr = |id: HirId, callback: &mut dyn FnMut(&hir::Attribute)| {
-            self.attrs(id).iter().for_each(callback);
+    fn precedence(&self, mut expr: &hir::Expr<'_>) -> ExprPrecedence {
+        while let hir::ExprKind::DropTemps(inner, ..) = expr.kind {
+            expr = inner;
+        }
+        let for_each_attr = |callback: &mut dyn FnMut(&hir::Attribute)| {
+            self.attrs(expr.hir_id).iter().for_each(callback);
         };
         expr.precedence(&for_each_attr)
     }

which is fine but I think less straightforward than the current approach.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I missed that! Thanks for clearing it up, I agree the current approach is fine then.

) -> ExprPrecedence {
let prefix_attrs_precedence = || -> ExprPrecedence {
let mut has_outer_attr = false;
for_each_attr(self.hir_id, &mut |attr: &Attribute| {
has_outer_attr |= matches!(attr.style(), AttrStyle::Outer)
});
if has_outer_attr { ExprPrecedence::Prefix } else { ExprPrecedence::Unambiguous }
};

match &self.kind {
ExprKind::Closure(closure) => {
match closure.fn_decl.output {
FnRetTy::DefaultReturn(_) => ExprPrecedence::Jump,
FnRetTy::Return(_) => ExprPrecedence::Unambiguous,
FnRetTy::Return(_) => prefix_attrs_precedence(),
}
}

Expand All @@ -2315,7 +2326,7 @@ impl Expr<'_> {
| ExprKind::Let(..)
| ExprKind::Unary(..) => ExprPrecedence::Prefix,

// Never need parens
// Need parens if and only if there are prefix attributes.
ExprKind::Array(_)
| ExprKind::Block(..)
| ExprKind::Call(..)
Expand All @@ -2337,9 +2348,9 @@ impl Expr<'_> {
| ExprKind::Type(..)
| ExprKind::UnsafeBinderCast(..)
| ExprKind::Use(..)
| ExprKind::Err(_) => ExprPrecedence::Unambiguous,
| ExprKind::Err(_) => prefix_attrs_precedence(),

ExprKind::DropTemps(expr, ..) => expr.precedence(),
ExprKind::DropTemps(expr, ..) => expr.precedence(for_each_attr),
}
}

Expand Down
52 changes: 34 additions & 18 deletions compiler/rustc_hir_pretty/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@ impl<'a> State<'a> {
(self.attrs)(id)
}

fn precedence(&self, expr: &hir::Expr<'_>) -> ExprPrecedence {
let for_each_attr = |id: HirId, callback: &mut dyn FnMut(&hir::Attribute)| {
self.attrs(id).iter().for_each(callback);
};
expr.precedence(&for_each_attr)
}

fn print_attrs_as_inner(&mut self, attrs: &[hir::Attribute]) {
self.print_either_attributes(attrs, ast::AttrStyle::Inner)
}
Expand Down Expand Up @@ -1164,7 +1171,7 @@ impl<'a> State<'a> {
}
self.space();
self.word_space("=");
let npals = || parser::needs_par_as_let_scrutinee(init.precedence());
let npals = || parser::needs_par_as_let_scrutinee(self.precedence(init));
self.print_expr_cond_paren(init, Self::cond_needs_par(init) || npals())
}

Expand Down Expand Up @@ -1265,7 +1272,7 @@ impl<'a> State<'a> {
fn print_expr_call(&mut self, func: &hir::Expr<'_>, args: &[hir::Expr<'_>]) {
let needs_paren = match func.kind {
hir::ExprKind::Field(..) => true,
_ => func.precedence() < ExprPrecedence::Unambiguous,
_ => self.precedence(func) < ExprPrecedence::Unambiguous,
};

self.print_expr_cond_paren(func, needs_paren);
Expand All @@ -1279,7 +1286,10 @@ impl<'a> State<'a> {
args: &[hir::Expr<'_>],
) {
let base_args = args;
self.print_expr_cond_paren(receiver, receiver.precedence() < ExprPrecedence::Unambiguous);
self.print_expr_cond_paren(
receiver,
self.precedence(receiver) < ExprPrecedence::Unambiguous,
);
self.word(".");
self.print_ident(segment.ident);

Expand All @@ -1293,8 +1303,8 @@ impl<'a> State<'a> {

fn print_expr_binary(&mut self, op: hir::BinOpKind, lhs: &hir::Expr<'_>, rhs: &hir::Expr<'_>) {
let binop_prec = op.precedence();
let left_prec = lhs.precedence();
let right_prec = rhs.precedence();
let left_prec = self.precedence(lhs);
let right_prec = self.precedence(rhs);

let (mut left_needs_paren, right_needs_paren) = match op.fixity() {
Fixity::Left => (left_prec < binop_prec, right_prec <= binop_prec),
Expand Down Expand Up @@ -1323,7 +1333,7 @@ impl<'a> State<'a> {

fn print_expr_unary(&mut self, op: hir::UnOp, expr: &hir::Expr<'_>) {
self.word(op.as_str());
self.print_expr_cond_paren(expr, expr.precedence() < ExprPrecedence::Prefix);
self.print_expr_cond_paren(expr, self.precedence(expr) < ExprPrecedence::Prefix);
}

fn print_expr_addr_of(
Expand All @@ -1340,7 +1350,7 @@ impl<'a> State<'a> {
self.print_mutability(mutability, true);
}
}
self.print_expr_cond_paren(expr, expr.precedence() < ExprPrecedence::Prefix);
self.print_expr_cond_paren(expr, self.precedence(expr) < ExprPrecedence::Prefix);
}

fn print_literal(&mut self, lit: &hir::Lit) {
Expand Down Expand Up @@ -1483,7 +1493,7 @@ impl<'a> State<'a> {
self.print_literal(lit);
}
hir::ExprKind::Cast(expr, ty) => {
self.print_expr_cond_paren(expr, expr.precedence() < ExprPrecedence::Cast);
self.print_expr_cond_paren(expr, self.precedence(expr) < ExprPrecedence::Cast);
self.space();
self.word_space("as");
self.print_type(ty);
Expand Down Expand Up @@ -1580,24 +1590,30 @@ impl<'a> State<'a> {
self.print_block(blk, cb, ib);
}
hir::ExprKind::Assign(lhs, rhs, _) => {
self.print_expr_cond_paren(lhs, lhs.precedence() <= ExprPrecedence::Assign);
self.print_expr_cond_paren(lhs, self.precedence(lhs) <= ExprPrecedence::Assign);
self.space();
self.word_space("=");
self.print_expr_cond_paren(rhs, rhs.precedence() < ExprPrecedence::Assign);
self.print_expr_cond_paren(rhs, self.precedence(rhs) < ExprPrecedence::Assign);
}
hir::ExprKind::AssignOp(op, lhs, rhs) => {
self.print_expr_cond_paren(lhs, lhs.precedence() <= ExprPrecedence::Assign);
self.print_expr_cond_paren(lhs, self.precedence(lhs) <= ExprPrecedence::Assign);
self.space();
self.word_space(op.node.as_str());
self.print_expr_cond_paren(rhs, rhs.precedence() < ExprPrecedence::Assign);
self.print_expr_cond_paren(rhs, self.precedence(rhs) < ExprPrecedence::Assign);
}
hir::ExprKind::Field(expr, ident) => {
self.print_expr_cond_paren(expr, expr.precedence() < ExprPrecedence::Unambiguous);
self.print_expr_cond_paren(
expr,
self.precedence(expr) < ExprPrecedence::Unambiguous,
);
self.word(".");
self.print_ident(ident);
}
hir::ExprKind::Index(expr, index, _) => {
self.print_expr_cond_paren(expr, expr.precedence() < ExprPrecedence::Unambiguous);
self.print_expr_cond_paren(
expr,
self.precedence(expr) < ExprPrecedence::Unambiguous,
);
self.word("[");
self.print_expr(index);
self.word("]");
Expand All @@ -1611,7 +1627,7 @@ impl<'a> State<'a> {
}
if let Some(expr) = opt_expr {
self.space();
self.print_expr_cond_paren(expr, expr.precedence() < ExprPrecedence::Jump);
self.print_expr_cond_paren(expr, self.precedence(expr) < ExprPrecedence::Jump);
}
}
hir::ExprKind::Continue(destination) => {
Expand All @@ -1625,13 +1641,13 @@ impl<'a> State<'a> {
self.word("return");
if let Some(expr) = result {
self.word(" ");
self.print_expr_cond_paren(expr, expr.precedence() < ExprPrecedence::Jump);
self.print_expr_cond_paren(expr, self.precedence(expr) < ExprPrecedence::Jump);
}
}
hir::ExprKind::Become(result) => {
self.word("become");
self.word(" ");
self.print_expr_cond_paren(result, result.precedence() < ExprPrecedence::Jump);
self.print_expr_cond_paren(result, self.precedence(result) < ExprPrecedence::Jump);
}
hir::ExprKind::InlineAsm(asm) => {
self.word("asm!");
Expand Down Expand Up @@ -1669,7 +1685,7 @@ impl<'a> State<'a> {
}
hir::ExprKind::Yield(expr, _) => {
self.word_space("yield");
self.print_expr_cond_paren(expr, expr.precedence() < ExprPrecedence::Jump);
self.print_expr_cond_paren(expr, self.precedence(expr) < ExprPrecedence::Jump);
}
hir::ExprKind::Err(_) => {
self.popen();
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_typeck/src/callee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
};

if let Ok(rest_snippet) = rest_snippet {
let sugg = if callee_expr.precedence() >= ExprPrecedence::Unambiguous {
let sugg = if self.precedence(callee_expr) >= ExprPrecedence::Unambiguous {
vec![
(up_to_rcvr_span, "".to_string()),
(rest_span, format!(".{}({rest_snippet}", segment.ident)),
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_hir_typeck/src/cast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1100,7 +1100,7 @@ impl<'a, 'tcx> CastCheck<'tcx> {
}

fn lossy_provenance_ptr2int_lint(&self, fcx: &FnCtxt<'a, 'tcx>, t_c: ty::cast::IntTy) {
let expr_prec = self.expr.precedence();
let expr_prec = fcx.precedence(self.expr);
let needs_parens = expr_prec < ExprPrecedence::Unambiguous;

let needs_cast = !matches!(t_c, ty::cast::IntTy::U(ty::UintTy::Usize));
Expand Down
27 changes: 26 additions & 1 deletion compiler/rustc_hir_typeck/src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
//! See [`rustc_hir_analysis::check`] for more context on type checking in general.

use rustc_abi::{ExternAbi, FIRST_VARIANT, FieldIdx};
use rustc_ast::util::parser::ExprPrecedence;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::stack::ensure_sufficient_stack;
use rustc_data_structures::unord::UnordMap;
Expand All @@ -17,7 +18,7 @@ use rustc_errors::{
use rustc_hir::def::{CtorKind, DefKind, Res};
use rustc_hir::def_id::DefId;
use rustc_hir::lang_items::LangItem;
use rustc_hir::{ExprKind, HirId, QPath};
use rustc_hir::{Attribute, ExprKind, HirId, QPath};
use rustc_hir_analysis::NoVariantNamed;
use rustc_hir_analysis::hir_ty_lowering::{FeedConstTy, HirTyLowerer as _};
use rustc_infer::infer;
Expand Down Expand Up @@ -54,6 +55,30 @@ use crate::{
};

impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
pub(crate) fn precedence(&self, expr: &hir::Expr<'_>) -> ExprPrecedence {
let for_each_attr = |id: HirId, callback: &mut dyn FnMut(&Attribute)| {
for attr in self.tcx.hir_attrs(id) {
// For the purpose of rendering suggestions, disregard attributes
// that originate from desugaring of any kind. For example, `x?`
// desugars to `#[allow(unreachable_code)] match ...`. Failing to
// ignore the prefix attribute in the desugaring would cause this
// suggestion:
//
// let y: u32 = x?.try_into().unwrap();
// ++++++++++++++++++++
//
// to be rendered as:
//
// let y: u32 = (x?).try_into().unwrap();
// + +++++++++++++++++++++
if attr.span().desugaring_kind().is_none() {
callback(attr);
}
}
};
expr.precedence(&for_each_attr)
}

/// Check an expr with an expectation type, and also demand that the expr's
/// evaluated type is a subtype of the expectation at the end. This is a
/// *hard* requirement.
Expand Down
8 changes: 4 additions & 4 deletions compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// so we remove the user's `clone` call.
{
vec![(receiver_method.ident.span, conversion_method_name.to_string())]
} else if expr.precedence() < ExprPrecedence::Unambiguous {
} else if self.precedence(expr) < ExprPrecedence::Unambiguous {
vec![
(expr.span.shrink_to_lo(), "(".to_string()),
(expr.span.shrink_to_hi(), format!(").{}()", conversion_method_name)),
Expand Down Expand Up @@ -1395,7 +1395,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
{
let span = expr.span.find_oldest_ancestor_in_same_ctxt();

let mut sugg = if expr.precedence() >= ExprPrecedence::Unambiguous {
let mut sugg = if self.precedence(expr) >= ExprPrecedence::Unambiguous {
vec![(span.shrink_to_hi(), ".into()".to_owned())]
} else {
vec![
Expand Down Expand Up @@ -3106,7 +3106,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
"change the type of the numeric literal from `{checked_ty}` to `{expected_ty}`",
);

let close_paren = if expr.precedence() < ExprPrecedence::Unambiguous {
let close_paren = if self.precedence(expr) < ExprPrecedence::Unambiguous {
sugg.push((expr.span.shrink_to_lo(), "(".to_string()));
")"
} else {
Expand All @@ -3131,7 +3131,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let len = src.trim_end_matches(&checked_ty.to_string()).len();
span.with_lo(span.lo() + BytePos(len as u32))
},
if expr.precedence() < ExprPrecedence::Unambiguous {
if self.precedence(expr) < ExprPrecedence::Unambiguous {
// Readd `)`
format!("{expected_ty})")
} else {
Expand Down
15 changes: 15 additions & 0 deletions compiler/rustc_lint/src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use std::cell::Cell;
use std::slice;

use rustc_ast::BindingMode;
use rustc_ast::util::parser::ExprPrecedence;
use rustc_data_structures::fx::FxIndexMap;
use rustc_data_structures::sync;
use rustc_data_structures::unord::UnordMap;
Expand Down Expand Up @@ -850,6 +851,20 @@ impl<'tcx> LateContext<'tcx> {
})
}

/// Returns the effective precedence of an expression for the purpose of
/// rendering diagnostic. This is not the same as the precedence that would
/// be used for pretty-printing HIR by rustc_hir_pretty.
pub fn precedence(&self, expr: &hir::Expr<'_>) -> ExprPrecedence {
let for_each_attr = |id: hir::HirId, callback: &mut dyn FnMut(&hir::Attribute)| {
for attr in self.tcx.hir_attrs(id) {
if attr.span().desugaring_kind().is_none() {
callback(attr);
}
}
};
expr.precedence(&for_each_attr)
}

/// If the given expression is a local binding, find the initializer expression.
/// If that initializer expression is another local binding, find its initializer again.
///
Expand Down
2 changes: 1 addition & 1 deletion src/tools/clippy/clippy_lints/src/casts/unnecessary_cast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ pub(super) fn check<'tcx>(
Node::Expr(parent) if is_borrow_expr(cx, parent) && !is_in_allowed_macro(cx, parent) => {
MaybeParenOrBlock::Block
},
Node::Expr(parent) if cast_expr.precedence() < parent.precedence() => MaybeParenOrBlock::Paren,
Node::Expr(parent) if cx.precedence(cast_expr) < cx.precedence(parent) => MaybeParenOrBlock::Paren,
_ => MaybeParenOrBlock::Nothing,
};

Expand Down
Loading
Loading