add back support for inner attributes on non-block expressions?

[nikomatsakis]

In #83312, @petrochenkov proposed the removal of inner attributes on non-block expressions. In the 2021-04-27 meeting, the @rust-lang/lang team noted a few concerns, but I didn't get around to raising those concerns on the PR itself before it was merged. I'm opening this issue to capture subsequent discussion.

The heart of the @rust-lang/lang concerns was that these attributes are in fact supported on stable and arguably working as expected. Some of us in the meeting felt like we would expect these attributes to be supported and that -- given that they are accepted -- there is only one potential interpretation (the current one). Plus we know that some crates (admittedly, not many) are using them.

Therefore, we would like to understand better the motivation for removing parser support for these features. The primary given motivation was parser performance, but on the PR thread @Aaron1011 suggested this was no longer a relevant concern.

[Mark-Simulacrum]

One thing that I had brought up during the lang team discussion, though not a concern shared by others I think, is that the meaning of the inner attribute can be somewhat ambiguous in expression positions (e.g., tuples). I don't think this merits a straight removal but we had discussed this point and I think I wasn't able to communicate my point too well, and was asked to write up a comment.

When we have a tuple like (#![foo] 3, 5) the foo attribute applies to the tuple per the current semantics. One could argue, however, that this is not necessarily the only possibility: it could equally apply to the "first element expression", i.e., the 3, if the elements are viewed as little contexts of their own. This is likely not the semantics most would expect, so it's a minor concern, but it's possible there are contexts where this is less "obviously not the semantic", I suppose.

[nikomatsakis]

Thanks @Mark-Simulacrum. I understand the point you're raising better now, although I still think the current usage feels pretty unambiguous.

It's also worth noting that the fact that these attributes work on stable is due to a bug:

Feature gating for attributes on expressions and statements is exceptionally random and buggy. As a result some of the inner attributes in question are also available on stable in some cases (the attribute must be a built-in, and the expression must be an expression statement).

[nikomatsakis]

Actually, reading @Aaron1011's comment more closely:

Note that in the final approach taken in #82608, inner attributes on expressions are now less of a problem than they once were. Custom inner attributes in expressions are banned, so we only need to pessimize token collection in the presence of #[derive] or #[cfg_attr]. However, there are still performance wins to be had in these cases, and the performance issue would likely become significant again if we decided to perform cfg-stripping for all attribute macros (in addition to derive macros).

I see that the current code works in part by detecting built-in attributes. That sounds unfortunate. I think I'm inclined to let the revert stand at present. We can always add these features back if desired.

[nikomatsakis]

We discussed this in our @rust-lang/lang meeting today! There wasn't a firm consensus, but we were concerned about the fact that we regressed 4 crates and about the fact that we failed to register a concern. However, the concerns raised in the PR are valid and there was no sense that supporting (#![foo] ..) and so forth was necessary (even if it does have some potential use in removing ambiguity). It would also be possible to add back in.

The meeting ended with us moving towards the option of adding back in inner attributes on match expressions specifically, since that covers all known breakage. @pnkfelix mentioned that they might prepare a PR, but @petrochenkov or @Aaron1011 can you speak to the potential performance impact of such a move?

[pnkfelix]

I believe this is addressed by PR #85193

[joshtriplett]

We discussed this in the @rust-lang/lang meeting today. Our conclusion was that we can close this issue as soon as #85193 is merged. That PR addresses the observed regressions.

[pnkfelix]

(close this after #85193 gets a beta backport)