Update `ty::VariantDef` to use `IndexVec<FieldIdx, FieldDef>` #109762

scottmcm · 2023-03-30T07:14:37Z

And while doing the updates for that, also uses FieldIdx in ProjectionKind::Field and TypeckResults::field_indices.

There's more places that could use it (like rustc_const_eval and LayoutS), but I tried to keep this PR from exploding to even more places.

Part 2/? of rust-lang/compiler-team#606

rustbot · 2023-03-30T07:14:43Z

r? @WaffleLapkin

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

rustbot · 2023-03-30T07:14:47Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

fbstj · 2023-03-30T09:14:20Z

compiler/rustc_const_eval/src/const_eval/valtrees.rs

+                            ty::Adt(def, substs) => def.variant(FIRST_VARIANT).fields
+                                [FieldIdx::from_usize(i)]
+                            .ty(tcx, substs),


this seems like it might be a wonky formatting?

I agree, but this is what rustfmt did -- I didn't do any manual reformattings, just ran x.py fmt to make tidy happy.

Hmm, maybe I'll do some shadowing for these cases to try to get something less weird...

Sometimes rustfmt just does literally no formatting, for example when there are if-let-chains, maybe this is the case here.

Nope, this is definitely rustfmt doing it -- all I did was add the FieldIdx::from_usize inline, without adding any newlines. Then tidy made me change it to that weirdness 🤷

WaffleLapkin

r=me with formatting fixed (@fbstj's comment) and minor comments addressed.

I think one interesting thing for future work may be to search for FieldIdx::from_usize and FieldIdx::from_u32 (and maybe ::new and .index) and see if they can be removed via changing the caller.

WaffleLapkin · 2023-03-30T11:56:35Z

compiler/rustc_hir_typeck/src/expr.rs

@@ -2238,7 +2237,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                                self.apply_adjustments(base, adjustments);
                                self.register_predicates(autoderef.into_obligations());

-                                self.write_field_index(expr.hir_id, index);
+                                self.write_field_index(expr.hir_id, FieldIdx::from_usize(index));


Makes me wonder why do we need such cruel string manipulation few lines above O_o

It looks like it's because field is an Ident. So I guess the fstr == index.to_string() is basically just checking that it's not tup.0001? (It has an as_u32, but that's the index in the string interner, so would be very, very wrong here.)

aha, found a little tweak that should be clearer and avoid the String for every .0, .1, through .9:

field.name == sym::integer(index)

compiler/rustc_hir_typeck/src/upvar.rs

WaffleLapkin · 2023-03-30T12:29:00Z

compiler/rustc_middle/src/ty/layout.rs

-                            TyMaybeWithLayout::Ty(def.variant(index).fields[i].ty(tcx, substs))
-                        }
+                        Variants::Single { index } => TyMaybeWithLayout::Ty(
+                            def.variant(index).fields[FieldIdx::new(i)].ty(tcx, substs),


Why does this use ::new instead of ::from_usize?

Suggested change

def.variant(index).fields[FieldIdx::new(i)].ty(tcx, substs),

def.variant(index).fields[FieldIdx::from_usize(i)].ty(tcx, substs),

Because that file already had VariantIdx::new, apparently. I'll change it since I do like the concrete one better.

(It's kinda annoying that new (from Idx), from_usize and .into() (from newtype_index!) all exist and work so there's nothing pushing code to consistency.)

WaffleLapkin · 2023-03-30T12:30:34Z

compiler/rustc_mir_dataflow/src/elaborate_drops.rs

+        let unique_ty = adt.non_enum_variant().fields[FieldIdx::new(0)].ty(self.tcx(), substs);
+        let nonnull_ty = unique_ty.ty_adt_def().unwrap().non_enum_variant().fields
+            [FieldIdx::new(0)]


Suggested change

let unique_ty = adt.non_enum_variant().fields[FieldIdx::new(0)].ty(self.tcx(), substs);

let nonnull_ty = unique_ty.ty_adt_def().unwrap().non_enum_variant().fields

[FieldIdx::new(0)]

let unique_ty = adt.non_enum_variant().fields[FieldIdx::from_u32(0)].ty(self.tcx(), substs);

let nonnull_ty = unique_ty.ty_adt_def().unwrap().non_enum_variant().fields

[FieldIdx::from_u32(0)]

Also [] on the new line looks a bit weird, is this rustfmt's formatting? 🤔

Yup, this is rustfmt's choice 🤷

scottmcm · 2023-03-30T15:28:31Z

compiler/rustc_borrowck/src/type_check/mod.rs

@@ -1725,7 +1725,8 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
            AggregateKind::Adt(adt_did, variant_index, substs, _, active_field_index) => {
                let def = tcx.adt_def(adt_did);
                let variant = &def.variant(variant_index);
-                let adj_field_index = active_field_index.unwrap_or(field_index);
+                let adj_field_index =
+                    FieldIdx::from_usize(active_field_index.unwrap_or(field_index));


This is the kind of place that will hopefully be temporary -- changing AggregateKind::Adt to hold a FieldIdx and the function to use field_index: FieldIdx instead of field_index: usize should get rid of this in an upcoming PR in this series, but I needed to stop the tendrils somewhere 🙃

And while doing the updates for that, also uses `FieldIdx` in `ProjectionKind::Field` and `TypeckResults::field_indices`. There's more places that could use it (like `rustc_const_eval` and `LayoutS`), but I tried to keep this PR from exploding to *even more* places. Part 2/? of rust-lang/compiler-team#606

scottmcm · 2023-03-30T17:11:32Z

Added some extra lets to avoid the weird rustfmt formatting, and addressed comments
@bors r=WaffleLapkin

bors · 2023-03-30T17:11:34Z

📌 Commit 4abb455 has been approved by WaffleLapkin

It is now in the queue for this repository.

bors · 2023-03-31T03:36:21Z

⌛ Testing commit 4abb455 with merge eb3e9c1...

bors · 2023-03-31T06:09:59Z

☀️ Test successful - checks-actions
Approved by: WaffleLapkin
Pushing eb3e9c1 to master...

rust-timer · 2023-03-31T09:05:56Z

Finished benchmarking commit (eb3e9c1): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

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)	-	-	0
Improvements ✅ (primary)	-0.5%	[-0.6%, -0.5%]	3
Improvements ✅ (secondary)	-0.4%	[-0.5%, -0.3%]	8
All ❌✅ (primary)	-0.5%	[-0.6%, -0.5%]	3

Max RSS (memory usage)

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

Cycles

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

rustbot assigned WaffleLapkin Mar 30, 2023

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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Mar 30, 2023

fbstj reviewed Mar 30, 2023

View reviewed changes

WaffleLapkin approved these changes Mar 30, 2023

View reviewed changes

scottmcm commented Mar 30, 2023

View reviewed changes

scottmcm force-pushed the variantdef-indexvec branch from b123407 to 4abb455 Compare March 30, 2023 16:24

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 30, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 31, 2023

bors merged commit eb3e9c1 into rust-lang:master Mar 31, 2023

rustbot added this to the 1.70.0 milestone Mar 31, 2023

scottmcm deleted the variantdef-indexvec branch March 31, 2023 06:10

bors mentioned this pull request Mar 31, 2023

Reformulate point_at_expr_source_of_inferred_type to be more accurate #108687

Merged

lcnr removed the WG-trait-system-refactor The Rustc Trait System Refactor Initiative label Apr 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `ty::VariantDef` to use `IndexVec<FieldIdx, FieldDef>` #109762

Update `ty::VariantDef` to use `IndexVec<FieldIdx, FieldDef>` #109762

scottmcm commented Mar 30, 2023

rustbot commented Mar 30, 2023

rustbot commented Mar 30, 2023

fbstj Mar 30, 2023 •

edited

Loading

scottmcm Mar 30, 2023

scottmcm Mar 30, 2023

WaffleLapkin Mar 30, 2023

scottmcm Mar 30, 2023

WaffleLapkin left a comment

WaffleLapkin Mar 30, 2023

scottmcm Mar 30, 2023

scottmcm Mar 30, 2023

WaffleLapkin Mar 30, 2023

scottmcm Mar 30, 2023

WaffleLapkin Mar 30, 2023

WaffleLapkin Mar 30, 2023

scottmcm Mar 30, 2023

scottmcm Mar 30, 2023

scottmcm commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 31, 2023

bors commented Mar 31, 2023

rust-timer commented Mar 31, 2023

	def.variant(index).fields[FieldIdx::new(i)].ty(tcx, substs),
	def.variant(index).fields[FieldIdx::from_usize(i)].ty(tcx, substs),

Update ty::VariantDef to use IndexVec<FieldIdx, FieldDef> #109762

Update ty::VariantDef to use IndexVec<FieldIdx, FieldDef> #109762

Conversation

scottmcm commented Mar 30, 2023

rustbot commented Mar 30, 2023

rustbot commented Mar 30, 2023

fbstj Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WaffleLapkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Mar 30, 2023

bors commented Mar 30, 2023

bors commented Mar 31, 2023

bors commented Mar 31, 2023

rust-timer commented Mar 31, 2023

Overall result: ✅ improvements - no action needed

Update `ty::VariantDef` to use `IndexVec<FieldIdx, FieldDef>` #109762

Update `ty::VariantDef` to use `IndexVec<FieldIdx, FieldDef>` #109762

fbstj Mar 30, 2023 •

edited

Loading