Attributes on expressions become duplicated under certain conditions

[coadler]

Describe the bug

Attributes on expressions become doubled every time rustfmt is ran when preceeded by a comment and the expression contains a cast, i.e. as i64.

To Reproduce

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=2499dbd812381b73c863ad79691053fd

trait FooBar {
    fn foo(&self) -> i64 {
        // some comment
        #[allow(clippy::cast_possible_wrap)]
        1u64 as i64
    }
}

Running rustfmt under tools on the above playground link causes the attribute to double on every invocation.

Playing around with it, removing either the comment and as i64 cause this not to reproduce, so it seems like it's the combination of them both that is causing this bug.

Expected behavior

The attribute isn't doubled every time.

Meta

• rustfmt version: rustfmt 1.4.21-nightly (01f2ead 2020-09-04) and latest stable
• From where did you install rustfmt?: rustup
• How do you run rustfmt: rustfmt directly on the file

(edited by calebcartwright to include inline reproduction snippet)

[calebcartwright]

Interesting. Seems that at least as of v679 of the rustc-ap-* crates that the span is off for expression statements with attributes, at least for cast expression kinds, as the attributes are only associated to the subexpr of the cast and not the expr nor the statement.

This should be fixed upstream in rustc (if it hasn't already), though I imagine we could work around it by using the lo of the span from the first attribute in these cases (Expr statement, expr kind cast, subexpr has attributes)

rustfmt/src/formatting/spanned.rs

Lines 69 to 71 in c240f10

	ast::StmtKind::Expr(ref expr) | ast::StmtKind::Semi(ref expr) => {
	mk_sp(expr.span().lo(), self.span.hi())
	}

[chansuke]

@calebcartwright I'm trying to fix this issue and update the first attribute of ast::ExprKind::Cast() in src/formatting/expr.rs but I have no idea to how to find the subexpr has attributes case. Could you give me a suggestion for that...

[calebcartwright]

Thanks for looking into it @chansuke! The fix will need to be made within spanned as linked in my previous response. We can't mutate the actual AST nodes, so it's really more about ensuring that the correct span is used for the expression statement.

The problem in this case currently is that the attributes are associated to the cast subexpression and not the cast expression itself, but correct recognition of the presence of attributes is required for rustfmt for span derivation.

In the match arm I linked above for expression statements, we'll need to check the expression kind, and if it's ExprKind::Cast and there's attributes on that subexpression, then we'll want to use the lo of that first attribute instead of expr.span().lo() that is currently used as the lo arg in that call to mk_sp

Does that make sense?

[t-nelson]

Another, perhaps more degenerate, case

fn foo()  {
    #[allow(clippy::cast_possible_wrap)]
    {
        // some comment
    }
}

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=9b93094ef421e9f36873fbae800a17ad

Here rustfmt's output is quite mangled 🙃

[chansuke]

@calebcartwright Thank you for your comment!! I will try adding ast::ExprKind::Cast(ref subexpr) => mk_sp(subexpr.attrs[0].span.lo(), subexpr.span.hi()) to the arm.

[calebcartwright]

Another, perhaps more degenerate, case

Thanks @t-nelson. Although that presents with similar symptoms, that's a separate and distinct issue in upstream rustc that has subsequently been fixed. That should resolve itself after we update rustfmt to the next version of the rustc mods used for parsing

[calebcartwright]

Thank you for your comment!! I will try adding ast::ExprKind::Cast(ref subexpr) => mk_sp(subexpr.attrs[0].span.lo(), subexpr.span.hi()) to the arm.

Thanks @chansuke! I've gone ahead and opened a PR upstream in rustc to correct the span calculation, and would rather just pull in the fix from upstream vs. trying to work around it within rustfmt.

It'll probably be a few days before we can grab the upstream fixes, but once we do this issue will turn into adding test cases to prevent regressions if you'd be interested in working on that.