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.

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

Inherit #[stable(..)] annotations in enum variants and fields from its item #71481

Merged
merged 3 commits into from
Mar 5, 2021

Conversation

estebank
Copy link
Contributor

Lint changes for #65515. The stdlib will have to be updated once this lands in beta and that version is promoted in master.

@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 23, 2020
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. A-stability Area: `#[stable]`, `#[unstable]` etc. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 2, 2020
@estebank
Copy link
Contributor Author

estebank commented May 7, 2020

CC @rust-lang/compiler who might be interested in this.

@petrochenkov
Copy link
Contributor

I still don't think this is a good idea.

@petrochenkov
Copy link
Contributor

Items in trait impls don't currently require stability attributes.
This PR at least should use the same mechanism.

@estebank
Copy link
Contributor Author

estebank commented May 7, 2020

I still don't think this is a good idea.

Does this refer to this implementation, the general idea of stability stable attributes flowing down in general or the specific case of them flowing down from ADTs to their variants and fields?

From the first diff section in https://github.com/rust-lang/rust/pull/71481/files#diff-6bfe08b41853465a10867dc22a4a548c it seems to me that we originally intended this new behavior to be the correct one.

It's also making me realize that after landing this when we update the used beta in this repo we'll also need to in the same commit update the current usages, as having two different nested stable attributes with the same value in the feature field is denied by this PR (in currently existing code) and would require a more significant change.

Items in trait impls don't currently require stability attributes.
This PR at least should use the same mechanism.

The mechanism used for impls is a visitor boolean field. This PR adds an argument and can be changed to be a field, but I find using fields tracking current state harder to follow, at least when trying things out first. It should be easy to change it to use a field if necessary.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2020
@estebank

This comment has been minimized.

@Elinvynia Elinvynia added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2020
#![feature(staged_api)]
#![stable(feature = "test", since = "0")]

#[stable(feature = "test", since = "0")]
pub struct Reverse<T>(pub T); //~ ERROR field has missing stability attribute
pub struct Reverse<T>(pub T);
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this is potentially dangerous change that also limits our options in the future.

I can see an use-case in adding an unstable new field to a struct and not impacting any stable uses of the struct.

Similarly for #[non_exhaustive] enums (the only kind of enum to which it is valid to add a variant), though perhaps significantly more debatable depending on the specific case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see an use-case in adding an unstable new field to a struct and not impacting any stable uses of the struct.

I'm not sure I follow. This PR makes the linked code semantically the same as the current

#[stable(feature = "test", since = "0")]
pub struct Reverse<T>(#[stable(feature = "test", since = "0")] pub T);

but you can still add unstable (and stable with different feature name) annotations to individual fields as you add to them. You currently get a compile error with the presented code. I don't see how this change limits us?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess the point is more that we might accidentally stabilize struct fields without intending to?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, the reason this comment ended up being here is because I’m expressing my observation based on the removed error message, not because the struct is in any way interesting.

@nikomatsakis summarized my point well.

@Dylan-DPC-zz
Copy link

@matthewjasper any updates on this?

@nikomatsakis nikomatsakis added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 30, 2020
@nikomatsakis
Copy link
Contributor

I'm nominating to discuss in a triage meeting and decide next steps here.

@spastorino
Copy link
Member

Removed I-nominated by mistake and re-added it again, we didn't have time to discuss this issue during this week triage meeting. So keeping it nominated for the next one.

@spastorino
Copy link
Member

This was discussed in today's weekly meeting. Removing nomination.

@bors
Copy link
Contributor

bors commented Mar 3, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 3, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@estebank
Copy link
Contributor Author

estebank commented Mar 3, 2021

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 3, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 4, 2021
…atsakis

Inherit `#[stable(..)]` annotations in enum variants and fields from its item

Lint changes for rust-lang#65515. The stdlib will have to be updated once this lands in beta and that version is promoted in master.
@bors
Copy link
Contributor

bors commented Mar 4, 2021

⌛ Testing commit 49310ce with merge d81b0db9e9fc6a3ecbf3d00f9652e8c33aa953d7...

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Removing intermediate container befbf413490f
 ---> 0862050eff83
Step 5/10 : RUN npm install es-check -g
 ---> Running in 019c2ce940b3
/node-v14.4.0-linux-x64/bin/es-check -> /node-v14.4.0-linux-x64/lib/node_modules/es-check/index.js

> spawn-sync@1.0.15 postinstall /node-v14.4.0-linux-x64/lib/node_modules/es-check/node_modules/spawn-sync
> node postinstall
+ es-check@5.2.1
added 95 packages from 44 contributors in 3.844s
Removing intermediate container 019c2ce940b3
 ---> 85daf0875536
---
Cloning into 'rust-toolstate'...
<Nothing changed>
+ es-check es5 ../src/librustdoc/html/static/main.js ../src/librustdoc/html/static/settings.js ../src/librustdoc/html/static/source-script.js ../src/librustdoc/html/static/storage.js

Cannot read property 'includes' of undefined

@bors
Copy link
Contributor

bors commented Mar 4, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 4, 2021
@JohnTitor
Copy link
Member

Unrelated CI failure, @bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 5, 2021
…atsakis

Inherit `#[stable(..)]` annotations in enum variants and fields from its item

Lint changes for rust-lang#65515. The stdlib will have to be updated once this lands in beta and that version is promoted in master.
@bors
Copy link
Contributor

bors commented Mar 5, 2021

⌛ Testing commit 49310ce with merge a0d66b5...

@bors
Copy link
Contributor

bors commented Mar 5, 2021

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing a0d66b5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 5, 2021
@bors bors merged commit a0d66b5 into rust-lang:master Mar 5, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 5, 2021
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Aug 14, 2022
…stab, r=estebank

Enum variant ctor inherits the stability of the enum variant

Fixes rust-lang#100399
Fixes rust-lang#100420

Context rust-lang#71481 for why enum variants don't need stability
@estebank estebank deleted the inherit-stability branch November 9, 2023 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stability Area: `#[stable]`, `#[unstable]` etc. merged-by-bors This PR was explicitly merged by bors. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.