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.

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

Reduce precedence of expressions that have an outer attr #134661

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+133 −47
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
1 commit
Select commit
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
15 changes: 12 additions & 3 deletions compiler/rustc_ast/src/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1309,11 +1309,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).

}
}
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),
}
}

Expand Down Expand Up @@ -1345,7 +1354,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::Block(..)
Expand Down Expand Up @@ -1378,7 +1387,7 @@ impl Expr {
| ExprKind::UnsafeBinderCast(..)
| ExprKind::While(..)
| ExprKind::Err(_)
| ExprKind::Dummy => ExprPrecedence::Unambiguous,
| ExprKind::Dummy => prefix_attrs_precedence(&self.attrs),
}
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_ast/src/mut_visit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -696,7 +696,7 @@ fn walk_local<T: MutVisitor>(vis: &mut T, local: &mut P<Local>) {
vis.visit_span(span);
}

fn walk_attribute<T: MutVisitor>(vis: &mut T, attr: &mut Attribute) {
pub fn walk_attribute<T: MutVisitor>(vis: &mut T, attr: &mut Attribute) {
let Attribute { kind, id: _, style: _, span } = attr;
match kind {
AttrKind::Normal(normal) => {
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 @@ -1942,12 +1942,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)),
) -> 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 @@ -1972,7 +1983,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 @@ -1993,9 +2004,9 @@ impl Expr<'_> {
| ExprKind::Tup(_)
| ExprKind::Type(..)
| ExprKind::UnsafeBinderCast(..)
| 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 @@ -78,6 +78,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_inner_attributes(&mut self, attrs: &[hir::Attribute]) -> bool {
self.print_either_attributes(attrs, ast::AttrStyle::Inner, false, true)
}
Expand Down Expand Up @@ -1155,7 +1162,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 @@ -1262,7 +1269,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 @@ -1276,7 +1283,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 @@ -1291,8 +1301,8 @@ impl<'a> State<'a> {
fn print_expr_binary(&mut self, op: hir::BinOp, lhs: &hir::Expr<'_>, rhs: &hir::Expr<'_>) {
let assoc_op = AssocOp::from_ast_binop(op.node);
let binop_prec = assoc_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 assoc_op.fixity() {
Fixity::Left => (left_prec < binop_prec, right_prec <= binop_prec),
Expand Down Expand Up @@ -1321,7 +1331,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 @@ -1338,7 +1348,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 @@ -1476,7 +1486,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 @@ -1577,25 +1587,31 @@ impl<'a> State<'a> {
self.print_block(blk);
}
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(op.node.as_str());
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::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 @@ -1609,7 +1625,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 @@ -1623,13 +1639,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 @@ -1667,7 +1683,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 @@ -605,7 +605,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 @@ -1107,7 +1107,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::{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 @@ -18,7 +19,7 @@ use rustc_hir::def::{CtorKind, DefKind, Res};
use rustc_hir::def_id::DefId;
use rustc_hir::intravisit::Visitor;
use rustc_hir::lang_items::LangItem;
use rustc_hir::{ExprKind, HirId, QPath};
use rustc_hir::{Attribute, ExprKind, HirId, QPath};
use rustc_hir_analysis::hir_ty_lowering::{FeedConstTy, HirTyLowerer as _};
use rustc_infer::infer;
use rustc_infer::infer::{DefineOpaqueTypes, InferOk};
Expand Down Expand Up @@ -55,6 +56,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 @@ -396,7 +396,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 @@ -1374,7 +1374,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 @@ -2988,7 +2988,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 @@ -3013,7 +3013,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let len = src.trim_end_matches(&checked_ty.to_string()).len();
expr.span.with_lo(expr.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 @@ -6,6 +6,7 @@
use std::cell::Cell;
use std::{iter, slice};

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 @@ -881,6 +882,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
Loading
Loading