Warn on codegen attributes on required trait methods

[JonathanBrouwer]

This PR turns applying the following attributes on required trait methods (that is, trait methods without a default implementation) into a FCW:

• #[cold]
• #[link_section]
• #[linkage] (unstable)
• #[rustc_allow_const_fn_unstable] (internal attribute)

These attributes already had no effect when applied to a required trait method, this PR only adds a warning.

Furthermore, it adds a comment in the code that the following codegen attributes are inherited when applied to a required trait method:

• #[track_caller]
• #[align] (unstable)

@rustbot labels +I-lang-nominated
@rust-lang/lang

Two questions for the lang team:

• Is adding this warning ok?
• Does the current behaviour of these attributes align with that you would expect them to be?

Fixes #147432

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

[rustbot]

r? @jackh726

rustbot has assigned @jackh726.
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

[jdonszelmann]

r? jdonszelmann

[traviscross]

@rfcbot fcp merge lang

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

Concerns:

• jackh726-concern (Warn on codegen attributes on required trait methods #148756 (comment))

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[tmandry]

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[jackh726]

So, I'd like to file a concern here that we should do a crater run on this before landing. Here's my reasoning:

This is landing as a FCW, which means that the team wants to remove this behavior, In my mind, this means that the team should know what the barriers are for that. If, for example, crater says that nobody is relying on it, it may mean that we want to move rather fast on that. If crater shows that a bunch of crates are relying on this, we should know that to be able to plan (I think Niko mentioned e.g. making the change over an edition in that case would be a possibility).

Essentially, the concern from me is that a FCW here indicates it's important enough for people to think about, and I think that the lang team should have the data about who it effects. And, not blocking landing this on that data runs the (high) risk of it falling through the cracks.

[nikomatsakis]

@rfcbot concern jackh726-concern

[jdonszelmann]

@JonathanBrouwer could you make a parallel PR that makes this a full error, then we crater that. Or at least I believe we wouldn't see this warning in a crate run right?

[JonathanBrouwer]

Crater run will happen here: #149137

[jdonszelmann]

@rustbot blocked

[JonathanBrouwer]

The crater results are here https://crater-reports.s3.amazonaws.com/pr-149137/index.html

• 259 crates fail to build because they depend on rhai, which has a #[cold] attribute on a required trait method
• 7 crates (including rhai itself) fail to build because they themselves have a #[cold] attribute on a required trait method
• 1 spurious

That's more results than I expected, mostly due to one popular crate making this mistake. I'm still convinced we need to add the warning here, since putting a cold attribute on a required trait method currently has no effect and users might expect it to. But this means we might need to be more careful in transitioning this into an error.

@nikomatsakis @jackh726 does this address your concern?

[scottmcm]

I still think we should do this, as it's consistent with other things that we've done previously (like breaking struct Foo { #[inline] x: u32 }).

Given that this has never worked and the fix is removing something that doesn't do anything -- and thus has no semver concerns -- I don't have concerns. It would be nice to send PRs to rhai (and any of the others that are "real", at least), of course.

@rfcbot reviewed

[jackh726]

Great - thank you @JonathanBrouwer for doing that.

The results certainly make it clear that we couldn't make this a hard error immediately, but also that it definitely makes sense as a FCW. I do think it'd be pretty quick and easy to make PRs to the affected crates and then switch this to be an error or deny-by-default FCW, but I don't think we typically do that for FCWs. For rhai, it's pretty reasonable given it makes up to large majority of affected cases, I'm happy to send a PR though myself if you aren't able to @JonathanBrouwer.

@nikomatsakis will have to resolve the concern for me, but happy from my side here

[JonathanBrouwer]

I'll make a PR for rhai later tonight :)
Edit: rhaiscript/rhai#1057

[bors]

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

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.