Rewrite `inline` attribute parser to use new infrastructure and improve diagnostics for all parsed attributes #138165

jdonszelmann · 2025-03-07T16:07:27Z

This PR:

creates a new parser for inline attributes
creates consistent error messages and error codes between attribute parsers; inline and others
as such changes a few error messages for other attributes to be (in my eyes) much more consistent
tests ast-lowering lints introduced by Infrastructure for lints during attribute parsing, specifically duplicate usages of attributes #138164 since this is now useful for the first time
Coalesce some useless error codes

Builds on top of #138164

jdonszelmann · 2025-03-08T23:37:17Z

compiler/rustc_codegen_ssa/src/codegen_attrs.rs

@@ -584,7 +546,6 @@ fn codegen_fn_attrs(tcx: TyCtxt<'_>, did: LocalDefId) -> CodegenFnAttrs {
            return OptimizeAttr::Default;
        };

-        inline_span = Some(attr.span());


This fixes #137950

bors · 2025-03-11T05:03:38Z

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

jdonszelmann · 2025-06-09T22:54:26Z

@rustbot review

jdonszelmann · 2025-06-09T23:21:16Z

ah shit, broke clippy 😭

jdonszelmann · 2025-06-10T12:03:09Z

tests/ui/feature-gates/issue-43106-gating-of-builtin-attrs-error.stderr

@@ -313,6 +303,16 @@ error[E0517]: attribute should be applied to a struct, enum, or union
 LL |     #[repr(Rust)] impl S { }
   |            ^^^^   ---------- not a struct, enum, or union

+error: valid forms for the attribute are `#[inline(always|never)]` and `#[inline]`
+  --> $DIR/issue-43106-gating-of-builtin-attrs-error.rs:44:5
+   |


error message moved

jdonszelmann · 2025-06-10T12:04:28Z

tests/ui/force-inlining/invalid.stderr

   |
-LL | fn barqux(#[rustc_force_inline] _x: u32) {}


I think this is a genuine regression, I think because check_attr now has to see this as a parsed attribute, let me fix that

jdonszelmann · 2025-06-10T12:06:27Z

tests/ui/macros/genercs-in-path-with-prettry-hir.stdout

@@ -8,7 +8,7 @@ extern crate std;
 // issue#97006

 macro_rules! m { ($attr_path: path) => { #[$attr_path] fn f() {} } }
-#[inline]
+#[attr = Inline(Hint)]


@obi1kenobi is this one relevant for CSC? I'm going to upgrade more attributes like this I'm afraid

Thanks for the ping! I think we don't SemVer-check based on it, but we do parse it so it'd be great to go back to emitting it as #[inline] in rustdoc JSON in a separate PR if possible. cc @aDotInTheVoid

mhm, I messaged Alona today as well, since it's technically also a breaking change in rustdoc json. However, it's really annoying since I'll be making about a 100 separate breaking changes like this in the coming months.... and practically few people will care

at the very least the debug representation is much more consistent nowadays. However, I think the solution is not to see this as a breaking change in the format, but just an unspecified part of the format you can't rely on at the moment until we stabilize it in a more structured way...

I think the solution is not to see this as a breaking change in the format, but just an unspecified part of the format you can't rely on at the moment

I think this is a fine mental model but I think it'll be frustrating to many users regardless, because we've given plenty of time to Hyrum's Law to kick in here.

Telling broken users "we're technically right, it was your assumptions that were wrong" will not make them any happier about it. So I think we should approach this situation with empathy for those users, and attempt to do what we can to minimize the negative impact as best as circumstances allow.

jdonszelmann · 2025-06-10T12:08:50Z

note, there are tests that tests the newly emitted lints during attr parsing. However, it works so well, none of them changed! They are emitted though, but exactly the same as they already were so it doesn't show up in the diff. Hard to check, but great!

bors · 2025-06-10T21:24:36Z

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

jdonszelmann · 2025-06-11T06:40:01Z

ugh, the rebase

bors · 2025-06-12T01:32:30Z

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

lint on duplicates during attribute parsing To do this we stuff them in the diagnostic context to be emitted after hir is constructed

…ra, r=oli-obk Infrastructure for lints during attribute parsing, specifically duplicate usages of attributes r? `@oli-obk` This PR adds a new field to OwnerInfo to buffer lints which are generated during attribute parsing and ast lowering in general. They can't be emitted at this stage because at that point there's no HIR yet, and early lints are already emitted. This also adds the generic `S: Stage` to attribute parsers. Currently we don't emit any lints during early attribute parsing, but if we ever want to that logic will be different. That's because there we don't have hir ids yet, while at the same time still having access to node ids and early lints. Even though that logic isn't completely there in this PR (no worries, we don't use it), that's why the parameter is there. With this PR, we also add 2 associated consts to `SingleAttributeParser`. Those determine what logic should be applied when finding a duplicate attribute. This PR was getting pretty large, so the first code using this logic is in rust-lang#138165. This code is all new things that weren't possible before so it also doesn't break any behaviour. However, some of it will be dead code right now. I recommend reviewing both before merging, though in some sense that doubles the size of the review again, and the other PR might be more controversial. Let me know how you want to do this `@oli-obk`

Infrastructure for lints during attribute parsing, specifically duplicate usages of attributes r? `@oli-obk` This PR adds a new field to OwnerInfo to buffer lints which are generated during attribute parsing and ast lowering in general. They can't be emitted at this stage because at that point there's no HIR yet, and early lints are already emitted. This also adds the generic `S: Stage` to attribute parsers. Currently we don't emit any lints during early attribute parsing, but if we ever want to that logic will be different. That's because there we don't have hir ids yet, while at the same time still having access to node ids and early lints. Even though that logic isn't completely there in this PR (no worries, we don't use it), that's why the parameter is there. With this PR, we also add 2 associated consts to `SingleAttributeParser`. Those determine what logic should be applied when finding a duplicate attribute. This PR was getting pretty large, so the first code using this logic is in #138165. This code is all new things that weren't possible before so it also doesn't break any behaviour. However, some of it will be dead code right now. I recommend reviewing both before merging, though in some sense that doubles the size of the review again, and the other PR might be more controversial. Let me know how you want to do this `@oli-obk`

rustbot assigned oli-obk Mar 7, 2025

rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 7, 2025