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

Retire the unnamed_fields feature for now #131045

Merged
merged 3 commits into from
Oct 11, 2024

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Sep 30, 2024

#![feature(unnamed_fields)] was implemented in part in #115131 and #115367, however work on that feature has (afaict) stalled and in the mean time there have been some concerns raised (e.g.12) about whether unnamed_fields is worthwhile to have in the language, especially in its current desugaring. Because it represents a compiler implementation burden including a new kind of anonymous ADT and additional complication to field selection, and is quite prone to bugs today, I'm choosing to remove the feature.

However, since I'm not one to really write a bunch of words, I'm specifically not going to de-RFC this feature. This PR essentially rolls back the state of this feature to "RFC accepted but not yet implemented"; however if anyone wants to formally unapprove the RFC from the t-lang side, then please be my guest. I'm just not totally willing to summarize the various language-facing reasons for why this feature is or is not worthwhile, since I'm coming from the compiler side mostly.

Fixes #117942
Fixes #121161
Fixes #121263
Fixes #121299
Fixes #121722
Fixes #121799
Fixes #126969
Fixes #131041

Tracking:

Footnotes

  1. https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Unnamed.20struct.2Funion.20fields

  2. https://github.com/rust-lang/rust/issues/49804#issuecomment-1972619108

@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2024

r? @fee1-dead

rustbot has assigned @fee1-dead.
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 Sep 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2024

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@compiler-errors
Copy link
Member Author

I'm nominating this for T-lang somewhat out of courtesy; my understanding is that whether or not an RFC-approved unstable implementation of a feature moves forwards or backwards really is a T-compiler judgement call, but I would like to hear T-lang's take on this.

I generally think that "let's go back to the drawing board" is the right call here, and in the mean time, I'd like to remove this from the compiler since there's a fair chance that a future design could be implemented some other way, and it's always easier to add things back into the compiler -- and I would be glad to either mentor or implement that work myself when that time comes, of course.

@rustbot label: +I-lang-nominated

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Sep 30, 2024
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Sep 30, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@fee1-dead
Copy link
Member

currently capacity deficient, so

r? compiler

@scottmcm
Copy link
Member

scottmcm commented Oct 2, 2024

Especially given that the tracking issue for this is marked S-tracking-design-concerns and S-tracking-impl-incomplete, I agree that T-compiler has free rein to remove an implementation with which they're not happy.

@traviscross
Copy link
Contributor

@rustbot labels -I-lang-nominated

We discussed this in the meeting today, and we appreciated the courtesy, and agree that it's OK as far as lang is concerned to rip this out.

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Oct 2, 2024
@michaelwoerister
Copy link
Member

r? compiler (cc rust-lang/team#1565)

@wesleywiser
Copy link
Member

I agree, removing this implementation is well motivated. Thanks @compiler-errors for doing this and also notifying T-lang of the removal.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2024

📌 Commit 6628bba has been approved by wesleywiser

It is now in the queue for this repository.

@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 Oct 11, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 11, 2024
…s, r=wesleywiser

Retire the `unnamed_fields` feature for now

`#![feature(unnamed_fields)]` was implemented in part in rust-lang#115131 and rust-lang#115367, however work on that feature has (afaict) stalled and in the mean time there have been some concerns raised (e.g.[^1][^2]) about whether `unnamed_fields` is worthwhile to have in the language, especially in its current desugaring. Because it represents a compiler implementation burden including a new kind of anonymous ADT and additional complication to field selection, and is quite prone to bugs today, I'm choosing to remove the feature.

However, since I'm not one to really write a bunch of words, I'm specifically *not* going to de-RFC this feature. This PR essentially *rolls back* the state of this feature to "RFC accepted but not yet implemented"; however if anyone wants to formally unapprove the RFC from the t-lang side, then please be my guest. I'm just not totally willing to summarize the various language-facing reasons for why this feature is or is not worthwhile, since I'm coming from the compiler side mostly.

Fixes rust-lang#117942
Fixes rust-lang#121161
Fixes rust-lang#121263
Fixes rust-lang#121299
Fixes rust-lang#121722
Fixes rust-lang#121799
Fixes rust-lang#126969
Fixes rust-lang#131041

Tracking:
* rust-lang#49804

[^1]: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Unnamed.20struct.2Funion.20fields
[^2]: rust-lang#49804 (comment)
@bors
Copy link
Contributor

bors commented Oct 11, 2024

⌛ Testing commit 6628bba with merge 80325f2...

@bors
Copy link
Contributor

bors commented Oct 11, 2024

⌛ Testing commit 6628bba with merge f496659...

@bors
Copy link
Contributor

bors commented Oct 11, 2024

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing f496659 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 11, 2024
@bors bors merged commit f496659 into rust-lang:master Oct 11, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 11, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f496659): comparison URL.

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

@rustbot label: -perf-regression

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

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

Max RSS (memory usage)

Results (secondary 0.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

Results (primary -2.0%, secondary 3.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 780.481s -> 780.255s (-0.03%)
Artifact size: 332.12 MiB -> 332.07 MiB (-0.01%)

flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 18, 2024
…s, r=wesleywiser

Retire the `unnamed_fields` feature for now

`#![feature(unnamed_fields)]` was implemented in part in rust-lang#115131 and rust-lang#115367, however work on that feature has (afaict) stalled and in the mean time there have been some concerns raised (e.g.[^1][^2]) about whether `unnamed_fields` is worthwhile to have in the language, especially in its current desugaring. Because it represents a compiler implementation burden including a new kind of anonymous ADT and additional complication to field selection, and is quite prone to bugs today, I'm choosing to remove the feature.

However, since I'm not one to really write a bunch of words, I'm specifically *not* going to de-RFC this feature. This PR essentially *rolls back* the state of this feature to "RFC accepted but not yet implemented"; however if anyone wants to formally unapprove the RFC from the t-lang side, then please be my guest. I'm just not totally willing to summarize the various language-facing reasons for why this feature is or is not worthwhile, since I'm coming from the compiler side mostly.

Fixes rust-lang#117942
Fixes rust-lang#121161
Fixes rust-lang#121263
Fixes rust-lang#121299
Fixes rust-lang#121722
Fixes rust-lang#121799
Fixes rust-lang#126969
Fixes rust-lang#131041

Tracking:
* rust-lang#49804

[^1]: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Unnamed.20struct.2Funion.20fields
[^2]: rust-lang#49804 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-unnamed_fields `#![feature(unnamed_fields)]` 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