Adding an attribute to an unsafe block changes its formatting

[tgross35]

This is the rustfmt result for an unsafe block:

fn main() {
    // #[cfg(not(fake_flag))]
    unsafe { println!() };
}

But uncomment the attribute, and it reformats it as:

fn main() {
    #[cfg(not(fake_flag))]
    unsafe {
        println!()
    };
}

Changing the attribute probably should not change the formatting.

rustfmt 1.7.0-nightly (5119208f 2024-03-02)

[ytmimi]

A similar issue has popped up in other cases too. #5901 and #5662 immediately come to mind, but there might be others in the backlog.

I haven't looked into this, but my hunch is that rustfmt considers the newline between the attribute and the block as an indication that the block can't be written on one line.

[ytmimi]

I'm assuming this affects all blocks, not just unsafe blocks? What about async blocks for example?

[tgross35]

Good call, looks like async and probably others are the same

fn main() {
    // #[cfg(not(fake_flag))]
    async { println!() };
}

fn main() {
    #[cfg(not(fake_flag))]
    async {
        println!()
    };
}

Probably indeed the same issue as #5662 so close this if you see fit. I just noticed because the diff for rust-lang/rust#121894 looked kind of funny :)

[ytmimi]

I think its the same underlying problem as #5662, but it's occurring in a different parts of the codebase. rustfmt rewrites attrs before rewriting the AST node those attrs are associated with so I think this is a distinct issue to the others I linked.

[ytmimi]

I did a little more digging into this one. I don't know if this is actually a bug, and I'm no longer convinced this is related to #5662. It seems that it might be intentional behavior. I've tracked this down to is_simple_block, which returns false if the block has attributes.

rustfmt/src/expr.rs

Lines 1219 to 1228 in fbe0424

	pub(crate) fn is_simple_block(
	context: &RewriteContext<'_>,
	block: &ast::Block,
	attrs: Option<&[ast::Attribute]>,
	) -> bool {
	block.stmts.len() == 1
	&& stmt_is_expr(&block.stmts[0])
	&& !block_contains_comment(context, block)
	&& attrs.map_or(true, |a| a.is_empty())
	}

It's not clear to me why that's the case. It seems that this logic was introduced in #2075 and merged in 0dca70f.

If we wanted to change that behavior we'd need to gate the change.

[tgross35]

Ah, thanks for the history. I have to imagine this specific case wasn't intentional - #2073 and all the tests added at 0dca70f#diff-56f48f88e4ce65cd696cc52c563399f2f751aaab2dfa3f8cd51a582b19e77070 only relate to adding attributes within let expressions. There isn't anything there for unsafe blocks without an expression, or a let (let bindings do not have the problem, see the below example).

This seems worth fixing to me. I have run into this a few times since writing this issue and it is always surprising that a noninvasive attribute affects the formatting of the thing it applies to, and that commenting the line affects the formatting.

I think an argument could be made that this is a bugfix so doesn't require gating - at least, it doesn't seem worth supporting the current behavior forever. This pattern also seems relatively rare, only a handful of uses in std https://github.com/search?q=repo%3Arust-lang%2Frust+%2F%5E%5Cs*%23%5C%5B.*%5Cn%5Cs*%28async%7Cconst%7Cunsafe%29+%5C%7B%28.*%5Cn%29%7B2%7D%5Cs*%5C%7D%2F+lang%3Arust&type=code.

Formatted example, for reference:

/* Block with `unsafe` / `async` / `const` */

// Problematic example
#[cfg(target_os = "linux")]
unsafe {
    println!("hi")
};

// Commenting the line affects the rest of the block
// #[cfg(target_os = "linux")]
unsafe { println!("hi") };

// Attribute has no effect for `let` expressions
#[cfg(not(target_os = "linux"))]
let x = unsafe { println!("hi") };

// Attribute has no effect for `let` expressions
// #[cfg(not(target_os = "linux"))]
let x = unsafe { println!("hi") };

/* Block without `unsafe` / `async` / `const` */

// Formatting with the attribute...
#[cfg(target_os = "linux")]
{
    println!("hi")
};

// ...is exactly the same as formatting without the attribute
// #[cfg(target_os = "linux")]
{
    println!("hi")
};

// Attribute has no effect for `let` expressions
#[cfg(not(target_os = "linux"))]
let x = { println!("hi") };

// Attribute has no effect for `let` expressions
// #[cfg(not(target_os = "linux"))]
let x = { println!("hi") };

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=55737cc7296d7c56d227939fca3bb669

[ytmimi]

Because of rustfmt's stability guarantee we'd need to gate this change since changing

#[cfg(target_os = "linux")]
unsafe {
    println!("hi")
};

to

#[cfg(target_os = "linux")]
unsafe { println!("hi")};

would be a breaking formatting change.