Simplify universal impl trait lowering #97598

spastorino · 2022-05-31T20:41:51Z

compiler/rustc_ast_lowering/src/lib.rs

cjgillot

Thanks @spastorino! I left a few questions.

cjgillot · 2022-06-01T17:14:33Z

compiler/rustc_ast_lowering/src/lib.rs

    /// Treat `impl Trait` as shorthand for a new universal generic parameter.
    /// Example: `fn foo(x: impl Debug)`, where `impl Debug` is conceptually
    /// equivalent to a fresh universal parameter like `fn foo<T: Debug>(x: T)`.
    ///
    /// Newly generated parameters should be inserted into the given `Vec`.
-    Universal(&'b mut Vec<hir::GenericParam<'a>>, &'b mut Vec<hir::WherePredicate<'a>>, LocalDefId),
+    Universal(LocalDefId),


Is this LocalDefId always equal to self.current_hir_id_owner, or can there be a mismatch?

I think so but maybe worth checking. Do you think I should check that in this PR? or should we leave this for a followup?

Ok for follow-up.

Ok, I've just checked this and yes, it's always equal. We are always creating Universal passing self.current_hir_id_owner to it.

cjgillot · 2022-06-01T17:14:42Z

compiler/rustc_ast_lowering/src/lib.rs

@@ -290,18 +293,6 @@ enum ImplTraitPosition {
    ImplReturn,
 }

-impl<'a> ImplTraitContext<'_, 'a> {
-    fn reborrow<'this>(&'this mut self) -> ImplTraitContext<'this, 'a> {


compiler/rustc_ast_lowering/src/lib.rs

compiler/rustc_ast_lowering/src/item.rs

Co-authored-by: Camille Gillot <[email protected]>

cjgillot · 2022-06-01T19:47:25Z

@bors try @rust-timer queue

rust-timer · 2022-06-01T19:47:26Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-06-01T19:47:38Z

⌛ Trying commit 15a82d6 with merge 769c5ef1e28f2107770e476529d9aa7305a01e2c...

bors · 2022-06-01T21:22:46Z

☀️ Try build successful - checks-actions
Build commit: 769c5ef1e28f2107770e476529d9aa7305a01e2c (769c5ef1e28f2107770e476529d9aa7305a01e2c)

rust-timer · 2022-06-01T21:22:48Z

Queued 769c5ef1e28f2107770e476529d9aa7305a01e2c with parent 8256e97, future comparison URL.

rust-timer · 2022-06-01T22:43:03Z

Finished benchmarking commit (769c5ef1e28f2107770e476529d9aa7305a01e2c): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-0.2%	-0.2%	1
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvement found
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	2.5%	2.5%	1
Improvements 🎉 (primary)	-3.6%	-3.6%	1
Improvements 🎉 (secondary)	-8.3%	-8.3%	1
All 😿🎉 (primary)	-3.6%	-3.6%	1

Cycles

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	2.5%	2.5%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-2.6%	-2.6%	1
All 😿🎉 (primary)	2.5%	2.5%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

cjgillot · 2022-06-02T17:12:25Z

@bors r+

bors · 2022-06-02T17:12:30Z

📌 Commit 15a82d6 has been approved by cjgillot

bors · 2022-06-02T21:37:41Z

⌛ Testing commit 15a82d6 with merge 42bcd41...

bors · 2022-06-03T00:21:23Z

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 42bcd41 to master...

rust-timer · 2022-06-03T02:26:41Z

Finished benchmarking commit (42bcd41): comparison url.

Instruction count

Primary benchmarks: no relevant changes found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	0.6%	0.8%	6
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	N/A	N/A	0

Max RSS (memory usage)

Results

Primary benchmarks: mixed results
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	2.7%	2.7%	1
Regressions 😿 (secondary)	1.1%	1.1%	1
Improvements 🎉 (primary)	-2.3%	-2.3%	1
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	0.2%	2.7%	2

Cycles

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: no relevant changes found

	mean¹	max	count²
Regressions 😿 (primary)	2.0%	2.0%	1
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	2.0%	2.0%	1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

spastorino added 10 commits May 31, 2022 12:40

Move defs and bounds from Universal to LoweringContext

3d6eae8

Take and restore fields in with_hir_id_owner calls

1a71103

Add itctx: ImplTraitContext arg to add_implicit_generics

190b4a7

Replace calls to lower_generics with calls to add_implicit_generics

e080298

Rename add_implicit_generics to lower_generics

d5ab8b2

Move lower_generics definition to item.rs

bd3a097

derive Copy, Clone, PartialEq, Eq for ImplTraitContext

208ffbb

Remove ImplTraitContext::reborrow

d327db9

Inline lower_generics_mut and remove GenericsCtor

2ade55d

Fix lower_generics rustdocs

15f2b11

rust-highfive assigned cjgillot May 31, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 31, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 31, 2022

fbstj reviewed Jun 1, 2022

View reviewed changes

compiler/rustc_ast_lowering/src/lib.rs Outdated Show resolved Hide resolved

cjgillot reviewed Jun 1, 2022

View reviewed changes

spastorino and others added 2 commits June 1, 2022 14:43

Add debug_assert comment

b3bc438

Co-authored-by: Camille Gillot <[email protected]>

Split extend + inner chain into to extend calls

2de2520

This comment has been minimized.

Sign in to view

instrument lower_fn_decl

67deaf9

spastorino force-pushed the simplify-universal-impl-trait-lowering branch from 793aca9 to 67deaf9 Compare June 1, 2022 18:02

Always assert that impl_trait_def|bounds are empty at start

15a82d6

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 1, 2022

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 1, 2022

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 Jun 2, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 3, 2022

bors merged commit 42bcd41 into rust-lang:master Jun 3, 2022

rustbot added this to the 1.63.0 milestone Jun 3, 2022

This was referenced Jun 3, 2022

Make lowering a query #95573

Merged

fix: impl Trait desugaring in trait objects' assoc constraints #97335

Closed

gurry mentioned this pull request Oct 17, 2023

Duplicate in this macro invocation labels #116836

Closed

matthiaskrgr mentioned this pull request Feb 25, 2024

ICE: range end index 1 out of range for slice of length 0 #121613

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify universal impl trait lowering #97598

Simplify universal impl trait lowering #97598

spastorino commented May 31, 2022

cjgillot left a comment

cjgillot Jun 1, 2022

spastorino Jun 1, 2022

cjgillot Jun 1, 2022

spastorino Jun 1, 2022

cjgillot Jun 1, 2022

This comment has been minimized.

cjgillot commented Jun 1, 2022

rust-timer commented Jun 1, 2022

bors commented Jun 1, 2022

bors commented Jun 1, 2022

rust-timer commented Jun 1, 2022

rust-timer commented Jun 1, 2022

cjgillot commented Jun 2, 2022

bors commented Jun 2, 2022

bors commented Jun 2, 2022

bors commented Jun 3, 2022

rust-timer commented Jun 3, 2022

Simplify universal impl trait lowering #97598

Simplify universal impl trait lowering #97598

Conversation

spastorino commented May 31, 2022

cjgillot 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

This comment has been minimized.

cjgillot commented Jun 1, 2022

rust-timer commented Jun 1, 2022

bors commented Jun 1, 2022

bors commented Jun 1, 2022

rust-timer commented Jun 1, 2022

rust-timer commented Jun 1, 2022

Footnotes

cjgillot commented Jun 2, 2022

bors commented Jun 2, 2022

bors commented Jun 2, 2022

bors commented Jun 3, 2022

rust-timer commented Jun 3, 2022

Footnotes