Skip to content

optimize attribute applied to things other than methods/functions/c…#131814

Merged
bors merged 1 commit intorust-lang:masterfrom
tnuha:misapplied-optimize-attribute
Oct 21, 2024
Merged

optimize attribute applied to things other than methods/functions/c…#131814
bors merged 1 commit intorust-lang:masterfrom
tnuha:misapplied-optimize-attribute

Conversation

@tnuha
Copy link
Contributor

@tnuha tnuha commented Oct 17, 2024

…losures gives an error (#128488)

Duplicate of #128943, which I had accidentally closed when rebasing.

cc. @jieyouxu @compiler-errors @nikomatsakis @traviscross @pnkfelix.

@rustbot
Copy link
Collaborator

rustbot commented Oct 17, 2024

r? @jieyouxu

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

@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 Oct 17, 2024
@tnuha
Copy link
Contributor Author

tnuha commented Oct 17, 2024

@rustbot label +T-compiler +T-lang

@rustbot rustbot added the T-lang Relevant to the language team label Oct 17, 2024
@tnuha tnuha mentioned this pull request Oct 17, 2024
Closed
jieyouxu
jieyouxu requested changes Oct 17, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the PR. I've noticed a discrepancy between what the diagnostic says and what the validation actually does. Also want to double-check with T-lang the concrete set of targets that the #[optimize] attribute should be permitted on.

@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 Oct 17, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Oct 17, 2024 •
edited by traviscross
Loading

@jieyouxu:
The RFC says:

#[optimize] attribute applied to non function-like items (such as struct) or non function-like expressions (i.e. not closures) is considered “unused” as of this RFC and should fire the unused_attribute lint (unless the same attribute was used for a function-like item or expression, via e.g. propagation). Some future RFC may assign some behaviour to this attribute with respect to such definitions.

And niko said:

In general I prefer to give a hard error so that we can change the meaning rather than silently change behavior in the future,

And speaking for T-lang, TC said:

We discussed this today in triage, and our general feeling is that conservatively giving a hard error is our preference.

We talked about how, in defense of allowing these attributes liberally, that strictly speaking this is just an optimization, and so an unheeded attribute doesn't affect things in a language specification sense. But we still preferred to just give an error for now.

So my understanding of the gestalt is that this attribute should

  1. only be accepted where it explicitly has a behavior of some kind
  2. be given a behavior in certain places according to the RFC and the whims of T-lang

The former need not await the latter.

@jieyouxu
Copy link
Member

jieyouxu commented Oct 17, 2024

Fair enough. In that sense I would recommend rejecting the mod case and only accept function-like targets.

@traviscross
Copy link
Contributor

traviscross commented Oct 17, 2024

Note also the comment from pnkfelix here:

I'll just jot a follow-up note here that one place where we probably do want to continue accepting #[optimize] is on async blocks, with the semantics that the attribute is applied to the poll method of the generated future.

(This is based on lang-team discussion here.)

This would apply to nightly gen blocks also.

@tnuha tnuha force-pushed the misapplied-optimize-attribute branch from a75d7e6 to 1e79649 Compare October 20, 2024 01:11
@tnuha tnuha requested a review from jieyouxu October 20, 2024 01:12
@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 Oct 20, 2024
jieyouxu
jieyouxu approved these changes Oct 20, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, just one missing test case then r=me.

@jieyouxu
Copy link
Member

jieyouxu commented Oct 20, 2024

@rustbot author

@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 Oct 20, 2024
@tnuha tnuha force-pushed the misapplied-optimize-attribute branch from 1e79649 to 080103f Compare October 20, 2024 15:11
@tnuha
Copy link
Contributor Author

tnuha commented Oct 20, 2024 •
edited
Loading

Thank you for your mentorship in getting this thing in. Here's to future collaboration.

@rustbot review

@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 Oct 20, 2024
jieyouxu
jieyouxu approved these changes Oct 20, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, this LGTM now.

@jieyouxu
Copy link
Member

jieyouxu commented Oct 20, 2024

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Oct 20, 2024

📌 Commit 080103f has been approved by jieyouxu

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 20, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 20, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 20, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 20, 2024
@bors
Auto merge of rust-lang#131980 - matthiaskrgr:rollup-iy5nw71, r=matth…
7ed1a51 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#131814 (`optimize` attribute applied to things other than methods/functions/c…)
 - rust-lang#131927 (Check for filecheck directives in files marked `skip-filecheck`)
 - rust-lang#131967 (Remove `lower_mono_bounds`)
 - rust-lang#131973 (fix(rustdoc-json-types): document rustc-hash feature)
 - rust-lang#131976 (feat(rustdoc-json-types): mark simple enums as copy)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2a9b6d9 into rust-lang:master Oct 21, 2024
@rustbot rustbot added this to the 1.84.0 milestone Oct 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2024
@rust-timer
Unrolled build for rust-lang#131814
1140d36 
Rollup merge of rust-lang#131814 - Borgerr:misapplied-optimize-attribute, r=jieyouxu

`optimize` attribute applied to things other than methods/functions/c…

…losures gives an error (rust-lang#128488)

Duplicate of rust-lang#128943, which I had accidentally closed when rebasing.

cc. `@jieyouxu` `@compiler-errors` `@nikomatsakis` `@traviscross` `@pnkfelix.`
@oli-obk oli-obk mentioned this pull request Aug 8, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@jieyouxu jieyouxu

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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

Milestone

1.84.0

Development

Successfully merging this pull request may close these issues.

6 participants

@tnuha @rustbot @workingjubilee @jieyouxu @traviscross @bors