Lowering unnamed fields and anonymous adt by frank-king · Pull Request #115367 · rust-lang/rust

frank-king · 2023-08-30T01:03:48Z

This implements #49804.

Goals:

lowering anonymous ADTs from AST to HIR
generating definitions of anonymous ADTs
uniqueness check of the unnamed fields
field projection of anonymous ADTs
#[repr(C)] check of the anonymous ADTs

Non-Goals (will be in the next PRs)

capturing generic params for the anonymous ADTs from the parent ADT
pattern matching of anonymous ADTs
structural expressions of anonymous ADTs
rustdoc support of anonymous ADTs

rustbot · 2023-08-30T01:03:56Z

r? @cjgillot

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

rust-timer · 2024-02-12T18:21:00Z

Finished benchmarking commit (bdc1592): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	6.1%	[0.3%, 20.7%]	9
Improvements ✅ (primary)	-1.1%	[-1.1%, -1.1%]	1
Improvements ✅ (secondary)	-0.3%	[-0.3%, -0.3%]	1
All ❌✅ (primary)	-1.1%	[-1.1%, -1.1%]	1

Max RSS (memory usage)

Results

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)	-	-	0
Improvements ✅ (primary)	-2.0%	[-2.0%, -2.0%]	1
Improvements ✅ (secondary)	-5.3%	[-5.3%, -5.3%]	1
All ❌✅ (primary)	-2.0%	[-2.0%, -2.0%]	1

Cycles

Results

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)	6.9%	[2.7%, 13.8%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.1%	[-3.2%, -3.1%]	3
All ❌✅ (primary)	-	-	0

Binary size

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

Bootstrap: 661.706s -> 662.09s (0.06%)
Artifact size: 308.30 MiB -> 308.34 MiB (0.01%)

Kobzol · 2024-02-13T07:58:36Z

There seems to be a big regression here on this benchmark, which has a struct with many fields.

Should this have affected performance of non-anonymous fields?

frank-king · 2024-02-13T08:15:02Z

The query find_field seems to be inefficient and affects FnCtxt::check_field for tuple fields as well.

Is this problem serious? I'm going to do some refactoring work on this query (to implement struct patterns and struct expressions of unnamed fields), but I'm afraid I won't have much time till next week.

Kobzol · 2024-02-13T08:19:08Z

It would be nice to improve it eventually, it's probably not that rare to have structs with many fields, e.g. generated by macros. But there's no immediate rush :)

Kobzol · 2024-02-13T08:19:58Z

Oh, I only now noticed that it's a tuple struct. Yeah, so tuple structs with many fields are probably quite rare. In any case, if you find any wins here in the future, that would be great!

frank-king · 2024-02-13T08:27:25Z

No problem, I'll look into this benchmark in the next step of implementation.

find_field does not need to be a query. The current implementation is quadratic in the number of nested fields. r? `@davidtwco` as you reviewed rust-lang#115367Fixesrust-lang#121755

find_field does not need to be a query. The current implementation is quadratic in the number of nested fields. r? `@davidtwco` as you reviewed rust-lang/rust#115367Fixesrust-lang/rust#121755

…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. Fixesrust-lang#117942Fixesrust-lang#121161Fixesrust-lang#121263Fixesrust-lang#121299Fixesrust-lang#121722Fixesrust-lang#121799Fixesrust-lang#126969Fixesrust-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)

Empty idents are also disambiguated. This was added in rust-lang#115367.

It was added in rust-lang#115367 for anonymous ADTs. Those changes were then reverted in rust-lang#131045, but `empty_disambiguator` was left behind, perhaps by mistake. It seems to be unnecessary.

…eywiser Retire the `unnamed_fields` feature for now `#![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.[^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#117942Fixes#121161Fixes#121263Fixes#121299Fixes#121722Fixes#121799Fixes#126969Fixes#131041 Tracking: * #49804 [^1]: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Unnamed.20struct.2Funion.20fields [^2]: #49804 (comment)

rustbot assigned cjgillot Aug 30, 2023