Reduce precedence of expressions that have an outer attr

[dtolnay]

Previously, -Zunpretty=expanded would expand this program as follows:

#![feature(stmt_expr_attributes)]

macro_rules! repro {
    ($e:expr) => {
        #[allow(deprecated)] $e
    };
}

#[derive(Default)]
struct Thing {
    #[deprecated]
    field: i32,
}

fn main() {
    let thing = Thing::default();
    let _ = repro!(thing).field;
}

#![feature(prelude_import)]
#![feature(stmt_expr_attributes)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;

struct Thing {
    #[deprecated]
    field: i32,
}

#[automatically_derived]
impl ::core::default::Default for Thing {
    #[inline]
    fn default() -> Thing {
        Thing { field: ::core::default::Default::default() }
    }
}

fn main() {
    let thing = Thing::default();
    let _ = #[allow(deprecated)] thing.field;
}

This is not the correct expansion. The correct output would have (#[allow(deprecated)] thing).field with the attribute applying only to thing, not to thing.field.

[rustbot]

r? @fee1-dead

rustbot has assigned @fee1-dead.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[fee1-dead]

r? compiler

[bors]

☔ The latest upstream changes (presumably #134788) made this pull request unmergeable. Please resolve the merge conflicts.

[dtolnay]

Rebased to resolve conflict.

[fmease]

I'm going to look at this in a few hours.

[fmease]

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).

[dtolnay]

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.

[dtolnay]

Fix: #142476

[bors]

☔ The latest upstream changes (presumably #137235) made this pull request unmergeable. Please resolve the merge conflicts.

[wesleywiser]

Hi @fmease, it looks like you marked this as waiting on review recently. Is this actually waiting on review or for the author to respond to your previous review comment?

[dtolnay]

Rebased to resolve conflicts with:

• Move methods from Map to TyCtxt, part 4. #137504
• ast: Remove token visiting from AST visitor #140450
• deduplicate the rest of AST walker functions #142240

[dtolnay]

• Rebased to resolve conflict with add extern "custom" functions #140770

[fmease]

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)

[dtolnay]

Indeed. Great catch.

[fmease]

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 🤷

[fmease]

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

[dtolnay]

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.

[fmease]

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

[fmease]

Could you remind we why we need normalization in the first place? The downstream crates seem to only compare exprs by their pretty-printed representation (granted, AST nodes don't impl Eq) and AST pretty only cares about spans for emitting comments (IINM) which these test cases don't contain.

Due to that knowledge gap I'm surprised that you can get away with not erasing all spans in visit_attribute, branch AttrKind::Normal (e.g., the ones contained in normal_attr.item)

[dtolnay]

This matters for the format!("{expr:#?}") != format!("{expr2:#?}") in this part of the test:

rust/tests/ui-fulldeps/pprust-parenthesis-insertion.rs

Lines 182 to 193 in 8da6239

	// Check for FALSE POSITIVE: pretty-printer inserting parentheses where not needed.
	// Pseudocode:
	// assert(expr == parse(print(expr)))
	let printed = &pprust::expr_to_string(&expr);
	let Some(expr2) = parse_expr(psess, printed) else {
	fail("Pretty-printer produced invalid syntax", source_code, printed);
	continue;
	};
	if format!("{expr:#?}") != format!("{expr2:#?}") {
	fail("Pretty-printer inserted unnecessary parenthesis", source_code, printed);
	continue;
	}

Without erasing the Spans and NodeIds and AttrIds, the comparison fails spuriously depending on whitespace.

Attribute {
    kind: Normal(
        NormalAttr {
            ...
            tokens: Some(
-               LazyAttrTokenStream(AttrTokenStream([Token(Token { kind: Pound, span: Span { lo: BytePos(528), hi: BytePos(529), ctxt: #0 } }, Joint), Token(Token { kind: Bang, span: Span { lo: BytePos(529), hi: BytePos(530), ctxt: #0 } }, JointHidden), Delimited(DelimSpan { open: Span { lo: BytePos(530), hi: BytePos(531), ctxt: #0 }, close: Span { lo: BytePos(535), hi: BytePos(536), ctxt: #0 } }, DelimSpacing { open: JointHidden, close: Alone }, Bracket, AttrTokenStream([Token(Token { kind: Ident("attr", No), span: Span { lo: BytePos(531), hi: BytePos(535), ctxt: #0 } }, JointHidden)]))])),
+               LazyAttrTokenStream(AttrTokenStream([Token(Token { kind: Pound, span: Span { lo: BytePos(560), hi: BytePos(561), ctxt: #0 } }, Joint), Token(Token { kind: Bang, span: Span { lo: BytePos(561), hi: BytePos(562), ctxt: #0 } }, JointHidden), Delimited(DelimSpan { open: Span { lo: BytePos(562), hi: BytePos(563), ctxt: #0 }, close: Span { lo: BytePos(567), hi: BytePos(568), ctxt: #0 } }, DelimSpacing { open: JointHidden, close: Alone }, Bracket, AttrTokenStream([Token(Token { kind: Ident("attr", No), span: Span { lo: BytePos(563), hi: BytePos(567), ctxt: #0 } }, JointHidden)]))])),
            ),

There are 2 other possible ways to express this assert(expr == parse(print(expr))) test:

• Continue using Debug ({:#?}), but instead of performing Unparenthesize.visit_expr(&mut expr) beforehand, use a regex to normalize the rendered representation, i.e. equivalent to sed 's/(BytePos|AttrId|NodeId)\([0-9]+\)//'

• Do not use Debug, use some other PartialEq-like trait to traverse two Exprs and perform the comparison ignoring spans. Syn's test suite actually has an implementation of this (for rustc's data structures! not syn's data structures) but it is 900 lines. https://github.com/dtolnay/syn/blob/2.0.103/tests/common/eq.rs

As for why normal_attr.item does not need to be handled by the code added by this PR, that is because it's handled by the mut_visit::walk_attribute(self, attr).

rust/compiler/rustc_ast/src/visit.rs

Lines 1630 to 1654 in 8da6239

	pub fn walk_attribute<$($lt,)? V: $Visitor$(<$lt>)?>(vis: &mut V, attr: &$($lt)? $($mut)? Attribute) -> V::Result {
	let Attribute { kind, id: _, style: _, span } = attr;
	match kind {
	AttrKind::Normal(normal) => {
	let NormalAttr { item, tokens: _ } = &$($mut)?**normal;
	let AttrItem { unsafety: _, path, args, tokens: _ } = item;
	try_visit!(vis.visit_path(path));
	try_visit!(walk_attr_args(vis, args));
	}
	AttrKind::DocComment(_kind, _sym) => {}
	}
	visit_span(vis, span)
	}
	pub fn walk_attr_args<$($lt,)? V: $Visitor$(<$lt>)?>(vis: &mut V, args: &$($lt)? $($mut)? AttrArgs) -> V::Result {
	match args {
	AttrArgs::Empty => {}
	AttrArgs::Delimited(args) => try_visit!(visit_delim_args(vis, args)),
	AttrArgs::Eq { eq_span, expr } => {
	try_visit!(vis.visit_expr(expr));
	try_visit!(visit_span(vis, eq_span));
	}
	}
	V::Result::output()
	}

The code here in the test only needs to deal with the attribute tokens, which are not visited by mut_visit since #140450. Prior to #140450, this code was unnecessary and we could just use const VISIT_TOKENS: bool = true.

[fmease]

Some comments incl. questions but once addressed in one way or another, r=me

[dtolnay]

@bors r=fmease

[bors]

📌 Commit 2171f89 has been approved by fmease

It is now in the queue for this repository.