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

Implement RFC 3107: #[derive(Default)] on enums with a #[default] attribute #86735

Merged
merged 4 commits into from
Jul 28, 2021

Conversation

jhpratt
Copy link
Member

@jhpratt jhpratt commented Jun 30, 2021

This PR implements RFC 3107, which permits #[derive(Default)] on enums where a unit variant has a #[default] attribute. See comments for current status.

@rustbot rustbot added A-attributes Area: #[attributes(..)] S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 30, 2021
@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 30, 2021
@rust-highfive
Copy link
Collaborator

r? @LeSeulArtichaut

(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 Jun 30, 2021
@jhpratt
Copy link
Member Author

jhpratt commented Jun 30, 2021

RFC for reference

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2021
@rust-log-analyzer

This comment has been minimized.

@LeSeulArtichaut
Copy link
Contributor

While this is in the compiler directory, I think this is technically under purview of the libs-api team as this could be done in user-provided code. Not sure who is best to review; both lang and libs-api were included for the RFC.

I think the implementation is a detail that doesn't matter to the lang or libs-api teams, this should be reviewed by the compiler team.

@LeSeulArtichaut
Copy link
Contributor

LeSeulArtichaut commented Jun 30, 2021

One other thing that needs to be done (not sure how for this one — help welcome!) is "registering" the #[default] attribute with the compiler such that it doesn't complain about its mere existence.

You'll need to create the feature gate:

  • declare the feature gate in compiler/rustc_feature/src/active.rs (see the dev-guide for more information)
  • add a derive_default_enum symbol to be used as feature gate in compiler/rustc_span/src/symbol.rs

Once this is done, you should be able to define the attribute like this:

gated!(Default, AssumedUsed, template!(Word), derive_default_enum, experimental!(derive_default_enum)),

Apparently default is a weak keyword, so it's already declared as a symbol. You need to specify derive_default_enum in the 4th argument to declare that the feature gate is different from the name of the attribute (the last argument is only the message that is displayed when the attribute is used without the feature gate).

This should take care of checking that the attribute is only used on nightly, you don't have to manually implement it. However, you might want to implement a check to emit an error if the attribute is used outside an enum variant.

@jhpratt
Copy link
Member Author

jhpratt commented Jun 30, 2021

@rustbot label -T-libs-api +T-compiler

I found the page in the dev guide last night, and had it generally in the right place. When using the line you provided (a slight difference from what I had), I get the following error in a number of places

error[E0659]: `Default` is ambiguous (built-in attribute vs any other name)
  --> library/core/src/time.rs:63:61
   |
63 | #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Default)]
   |                                                             ^^^^^^^ ambiguous name
   |
   = note: `Default` could refer to a built-in attribute
note: `Default` could also refer to the derive macro imported here
  --> library/core/src/prelude/v1.rs:32:9
   |
32 | pub use crate::default::Default;
   |         ^^^^^^^^^^^^^^^^^^^^^^^

Given that sym::Default maps to default, I'm not sure why this is the case (symbols appear to be case sensitive).

While it may work by declaring the attribute globally and erroring if it's used in certain places, it feels like it should be better if there's a way for the derive to declare the attribute belongs to it. I presume this is possible given that it's exposed for proc_macro derives, but I was unable to find where this is actually implemented.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 30, 2021
@LeSeulArtichaut
Copy link
Contributor

I guess this proves I'm not the right reviewer here, r? @petrochenkov maybe

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 2021
@petrochenkov petrochenkov 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 Jul 6, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jhpratt
Copy link
Member Author

jhpratt commented Jul 19, 2021

Changes have been made.

@jhpratt
Copy link
Member Author

jhpratt commented Jul 27, 2021

The RFC has been merged. cc @petrochenkov

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 27, 2021

📌 Commit 7f036962daeaa8210a109441112474bc9a58f6cf has been approved by petrochenkov

@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-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 27, 2021
@bors

This comment has been minimized.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 27, 2021
@jhpratt
Copy link
Member Author

jhpratt commented Jul 27, 2021

Rebased. Also bumped to 1.56 given the 1.55 beta has been cut.

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 27, 2021

📌 Commit 72465b0 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 27, 2021
@bors
Copy link
Contributor

bors commented Jul 28, 2021

⌛ Testing commit 72465b0 with merge aea2e44...

@bors
Copy link
Contributor

bors commented Jul 28, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing aea2e44 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 28, 2021
@bors bors merged commit aea2e44 into rust-lang:master Jul 28, 2021
@rustbot rustbot added this to the 1.56.0 milestone Jul 28, 2021
@jhpratt jhpratt deleted the rfc-3107 branch July 28, 2021 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: #[attributes(..)] C-feature-accepted Category: A feature request that has been accepted pending implementation. 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.

None yet

7 participants