Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Oct 21, 2024
Merged

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

merged 1 commit into from
Oct 21, 2024

Conversation

Borgerr
Copy link
Contributor

@Borgerr Borgerr 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
@Borgerr
Copy link
Contributor Author

Borgerr commented Oct 17, 2024

@rustbot label +T-compiler +T-lang

@rustbot rustbot added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Oct 17, 2024
@Borgerr Borgerr 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

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

@traviscross
Copy link
Contributor

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.

@Borgerr Borgerr force-pushed the misapplied-optimize-attribute branch from a75d7e6 to 1e79649 Compare October 20, 2024 01:11
@Borgerr Borgerr 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

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

Borgerr 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

@bors r+ rollup

@bors
Copy link
Contributor

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
6 checks passed
@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.`
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, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.84.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Borgerr @rustbot @workingjubilee @jieyouxu @traviscross @bors