Skip to content

Lower attributes for functions without bodies - #162761

Open
JonathanBrouwer wants to merge 3 commits into
rust-lang:mainfrom
JonathanBrouwer:lower-param-attrs
Open

JonathanBrouwer wants to merge 3 commits into
rust-lang:mainfrom
JonathanBrouwer:lower-param-attrs

Conversation

@JonathanBrouwer

@JonathanBrouwer JonathanBrouwer commented Sep 14, 2026

Copy link
Copy Markdown
Member

View all comments

Fixes #162639

This is a temporary solution to get the bug fixed.
I'm going to see if we can move the function hir::Params from hir::Body to hir::FnSig, this would cleanly fix this bug and would undo most of this PR. That is a large project which might take me some time to complete, so I'd prefer to get the bug fixed first with this workaround.

Maybe r? @jdonszelmann since you have context?
cc @mejrs cause you're probably interested

@rustbot rustbot added 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 Sep 14, 2026
@rustbot

rustbot commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

jdonszelmann is currently at their maximum review capacity.
They may take a while to respond.

@rust-log-analyzer

This comment has been minimized.

Comment thread compiler/rustc_ast_lowering/src/lib.rs Outdated
Comment thread compiler/rustc_ast_lowering/src/lib.rs Outdated
@JonathanBrouwer
JonathanBrouwer force-pushed the lower-param-attrs branch 3 times, most recently from 29274ef to 5bf5ae2 Compare September 14, 2026 12:28
Comment thread compiler/rustc_ast_lowering/src/lib.rs

warning: the `must_use` attribute cannot be used on function params
--> $DIR/param-attrs-builtin-attrs.rs:40:7
--> $DIR/param-attrs-builtin-attrs.rs:29:7

@jdonszelmann jdonszelmann Sep 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the check for these warnings should go, since it causes double emissions

View changes since the review

@JonathanBrouwer JonathanBrouwer Sep 14, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix this in a follow-up PR, it will be quite a big diff to remove this error and replace it by target checking consistently

Comment thread tests/ui/rfcs/rfc-2565-param-attrs/param-attrs-builtin-attrs.stderr
@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 14, 2026
@JonathanBrouwer

Copy link
Copy Markdown
Member Author

@rust-lang/lang Is this a PR that you would want to take a look at?
This makes the following code, which was incorrectly allowed, no longer compile.
I believe there is some decision that for attribute fixes a ping was enough? I can't find that tho

fn help(x: fn(#[rustc_splat] usize)) {}

@JonathanBrouwer

Copy link
Copy Markdown
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 14, 2026
@jdonszelmann

Copy link
Copy Markdown
Contributor

r=me then after some acknowledgement from t-lang. I think it falls under the same rule as earlier breaking changes in attrs (i.e. notify t-lang but do the change) but let them acknowledge that. Notably, this attribute is rather new, it's unlikely anyone depends on this.

@JonathanBrouwer JonathanBrouwer added the S-waiting-on-t-lang Status: Awaiting decision from T-lang label Sep 14, 2026
@JonathanBrouwer

Copy link
Copy Markdown
Member Author

Just in case
@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 14, 2026
@rust-bors

This comment has been minimized.

rust-bors Bot pushed a commit that referenced this pull request Sep 14, 2026
Lower attributes for functions without bodies

@teor2345 teor2345 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the comment tweak, this looks like what I'd expect to happen for splat. Thank you for this fix!

View changes since this review

Comment thread compiler/rustc_ast_lowering/src/lib.rs Outdated

@teor2345 teor2345 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we also want to backport this to beta or stable?

Sorry for the multiple notifications 🙂

View changes since this review

//~| ERROR allow, cfg, cfg_attr, deny, expect, forbid, and warn are the only allowed built-in attributes
}

trait Test {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it's worth duplicating the trait, type, and extern tests in the splat feature gate test, up to you.

https://github.com/rust-lang/rust/blob/ada41e1ce81819f01577c0ee40ccbd6fc41384e8/tests/ui/feature-gates/feature-gate-splat.rs

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not worth duplicating the test

@rust-bors

rust-bors Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

☀️ Try build successful (CI)
Build commit: ed70b09 (ed70b094daccd48a86a95d8b0acdc0dfb125b3d0)
Base parent: ed77b7b (ed77b7b8699e342c2dc842c83cb5fe4025252ee8)

@rust-timer

This comment has been minimized.

@JonathanBrouwer JonathanBrouwer added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 14, 2026
@JonathanBrouwer

Copy link
Copy Markdown
Member Author

Don't think a stable backport is worth it. We can consider a beta backport, I opened a thread for it

@rust-timer

Copy link
Copy Markdown
Collaborator

Finished benchmarking commit (ed70b09): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.2% [-0.2%, -0.2%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This perf run didn't have relevant results for this metric.

Cycles

Results (primary 1.5%, secondary -8.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

mean range count
Regressions ❌
(primary)
1.5% [0.8%, 2.3%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-8.5% [-8.7%, -8.4%] 2
All ❌✅ (primary) 1.5% [0.8%, 2.3%] 2

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 495.332s -> 494.947s (-0.08%)
Artifact size: 406.96 MiB -> 406.95 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 14, 2026
@traviscross traviscross added I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang I-lang-radar Items that are on lang's radar and will need eventual work or consideration. T-lang Relevant to the language team labels Sep 15, 2026
@traviscross

Copy link
Copy Markdown
Contributor

I looked, and I could not find a delegation that would cover this.1 The default rule is that breaking changes to the language require a lang FCP.

In this PR, we're breaking:

fn help(x: fn(#[rustc_splat] usize)) {}

That makes sense to me. I propose we do it.

@rfcbot fcp merge lang

Footnotes

  1. As negative results, see, e.g., https://github.com/rust-lang/rust/issues/142838 and https://github.com/rust-lang/rust/pull/155193#issuecomment-4389641640.

@rust-rfcbot

rust-rfcbot commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

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

No concerns currently listed.

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.

@rust-rfcbot rust-rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 15, 2026
@JonathanBrouwer JonathanBrouwer removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 16, 2026
@teor2345

Copy link
Copy Markdown
Member

In this PR, we're breaking:

fn help(x: fn(#[rustc_splat] usize)) {}

For FCP context, this is an unstable incomplete feature, that was never meant to be usable on stable. (It currently ICEs on stable 1.98, and on 1.99 beta it works but isn't really usable, because splatted function items can only be declared on nightly.)

@tmandry

tmandry commented Sep 16, 2026

Copy link
Copy Markdown
Member

This is a bugfix.

@rfcbot reviewed

@rust-bors

rust-bors Bot commented Sep 23, 2026

Copy link
Copy Markdown
Contributor

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

@rust-rfcbot rust-rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Sep 23, 2026
@rust-rfcbot

Copy link
Copy Markdown
Collaborator

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

@traviscross traviscross added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. I-lang-nominated Nominated for discussion during a lang team meeting. P-lang-drag-1 Lang team prioritization drag level 1. https://rust-lang.zulipchat.com/#narrow/channel/410516-t-lang labels Sep 23, 2026
@scottmcm

Copy link
Copy Markdown
Member

Agreed that this was clearly never supposed to be stable.

@rfcbot reviewed

@teor2345

Copy link
Copy Markdown
Member

Agreed that this was clearly never supposed to be stable.

@rfcbot reviewed

Triage note: after an hour, @scottmcm's box hasn't been ticked by the bot yet, so I ticket it manually.
(This doesn't change whether the FCP is over the threshold.)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. I-lang-radar Items that are on lang's radar and will need eventual work or consideration. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-t-lang Status: Awaiting decision from T-lang T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rustc_splat is not feature gated in functions without bodies