Separate object lifetime default from lifetime resolution #97393

cjgillot · 2022-05-25T11:52:46Z

Resolution of object lifetime default is currently done during lifetime resolution.
However, this computation happens at a higher level, fetching generics_of query to be well defined.

This separation allows to remove the computation of "early index" from lifetime resolution,
relying on the computation performed by generics_of instead.
Having a completely separate computation allows to avoid cycles between lifetime resolution and generics_of.

Caveat: this implementation still replicates the nested Binder model from lifetime resolution
to correctly handle late-bound Debruijn indices. I don't know yet how to clean this up.

~~Based on #96296~~

rust-highfive · 2022-05-25T11:52:49Z

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

rust-highfive · 2022-05-25T11:52:51Z

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

petrochenkov · 2022-05-25T12:34:33Z

Blocked on #96296.

jackh726 · 2022-05-25T18:30:54Z

Neat

cjgillot · 2022-06-04T13:28:02Z

@bors try @rust-timer queue

rust-timer · 2022-06-04T13:28:04Z

Awaiting bors try build completion.

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

bors · 2022-06-04T13:28:10Z

⌛ Trying commit 5f6be7f with merge c572965bed3faeadf9101c64e711316e43d6f1d9...

bors · 2022-06-04T14:58:19Z

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

rust-timer · 2022-06-04T14:58:20Z

Queued c572965bed3faeadf9101c64e711316e43d6f1d9 with parent 4a52e0f, future comparison URL.

rust-timer · 2022-06-04T17:08:53Z

Finished benchmarking commit (c572965bed3faeadf9101c64e711316e43d6f1d9): comparison url.

Instruction count

Primary benchmarks: mixed results
Secondary benchmarks: mixed results

	mean¹	max	count²
Regressions 😿 (primary)	0.4%	0.9%	54
Regressions 😿 (secondary)	0.6%	1.0%	31
Improvements 🎉 (primary)	-0.3%	-0.3%	11
Improvements 🎉 (secondary)	-0.7%	-1.9%	27
All 😿🎉 (primary)	0.3%	0.9%	65

Max RSS (memory usage)

Results

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 😿 relevant regressions found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	1.6%	2.0%	2
Improvements 🎉 (primary)	-2.4%	-6.6%	9
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	-2.4%	-6.6%	9

Cycles

Results

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)	-2.4%	-2.4%	1
All 😿🎉 (primary)	N/A	N/A	0

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.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@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 ↩ ↩² ↩³

petrochenkov · 2022-06-04T21:07:55Z

I guess this is the part of compiler/rustc_resolve/src/late/lifetimes.rs that has nothing to do with name resolution.
r? @jackh726

cjgillot · 2022-06-04T21:45:11Z

compiler/rustc_middle/src/query/mod.rs

@@ -1591,7 +1591,13 @@ rustc_queries! {
    /// for each parameter if a trait object were to be passed for that parameter.
    /// For example, for `struct Foo<'a, T, U>`, this would be `['static, 'static]`.
    /// For `struct Foo<'a, T: 'a, U>`, this would instead be `['a, 'static]`.
-    query object_lifetime_defaults(_: LocalDefId) -> Option<&'tcx [ObjectLifetimeDefault]> {
+    query object_lifetime_default(_: DefId) -> Option<ObjectLifetimeDefault> {
+        desc { "looking up lifetime defaults for a region on an item" }


Suggested change

desc { "looking up lifetime defaults for a region on an item" }

desc { "compute object lifetime defaults for a generic parameter" }

cjgillot · 2022-06-05T11:32:57Z

compiler/rustc_typeck/src/collect/object_lifetime_defaults.rs

+                }
+            };
+            let object_lifetime_default = |i: usize| {
+                let param = generics.params.get(i)?;


This is wrong. The index of argument in HIR may not correspond to the index in ty::Generics.

cjgillot · 2022-06-05T11:33:21Z

compiler/rustc_typeck/src/collect/object_lifetime_defaults.rs

+                }
+                ObjectLifetimeDefault::Static => Some(Region::Static),
+                ObjectLifetimeDefault::Param(def_id) => {
+                    let index = *generics.param_def_id_to_index.get(&def_id)?;


This is also wrong, the other way around.

cjgillot · 2022-06-05T11:35:28Z

@jackh726 I think the handling of generic arguments could be re-implemented without touching HIR at all, and would be more robust. However, I need a bit of guidance here.

jackh726

Going through this. Is it possible that we can split out commits 1,2,3,8 , and 9 into a separate PR? That seems like an "easy" first step here.

jackh726 · 2022-06-05T20:48:44Z

compiler/rustc_middle/src/query/mod.rs

    /// for each parameter if a trait object were to be passed for that parameter.
    /// For example, for `struct Foo<'a, T, U>`, this would be `['static, 'static]`.
    /// For `struct Foo<'a, T: 'a, U>`, this would instead be `['a, 'static]`.


I don't think this comment is still true?

jackh726 · 2022-06-05T21:00:14Z

compiler/rustc_resolve/src/late/lifetimes.rs

-                    .map(set_to_region)
-                    .collect()
-            }
+            self.tcx.object_lifetime_defaults(def_id).unwrap().iter().map(set_to_region).collect()


Can the unwrap here be changed to an expect or uwrap_or_else(|| bug!())?

jackh726 · 2022-06-26T19:47:31Z

Blocked on #97839

rust-highfive assigned petrochenkov May 25, 2022

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

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

This comment has been minimized.

Sign in to view

petrochenkov added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2022

cjgillot force-pushed the object-lifetime-default branch from facb761 to 9b29e9a Compare May 29, 2022 18:42

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

cjgillot force-pushed the object-lifetime-default branch from 2caba45 to 7c0727e Compare June 3, 2022 10:05

cjgillot 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. labels Jun 3, 2022

This comment was marked as resolved.

Sign in to view

cjgillot added 12 commits June 3, 2022 22:39

Make object_lifetime_defaults a cross-crate query.

59ea28e

Separate computation of object lifetime default.

ca16d28

Create a specific ObjectLifetimeDefault enum.

ca36288

Create lifetime parameter like any other parameter.

a959cf6

Remove index from Region::EarlyBound.

bc70bb9

Remove opaque_type_parent.

be9a2a0

Avoid recomputing defaults for all arguments.

7ada5e5

Remove BinderScopeType.

830f221

Add FIXME for using ty::Region.

64e7067

Return ty::Region in object_lifetime_map.

e80a113

Pacify tidy.

d63dda2

Compute object_lifetime_default per parameter.

7c1a8d8

Bless incremental tests.

5f6be7f

cjgillot force-pushed the object-lifetime-default branch from 7c0727e to 5f6be7f Compare June 3, 2022 20:55

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

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 4, 2022

rust-highfive assigned jackh726 and unassigned petrochenkov Jun 4, 2022

cjgillot commented Jun 4, 2022

View reviewed changes

cjgillot commented Jun 5, 2022

View reviewed changes

jackh726 requested changes Jun 6, 2022

View reviewed changes

cjgillot mentioned this pull request Jun 7, 2022

Move object lifetime default computation #97839

Closed

jackh726 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2022

cjgillot closed this Jul 27, 2022

cjgillot deleted the object-lifetime-default branch July 27, 2022 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate object lifetime default from lifetime resolution #97393

Separate object lifetime default from lifetime resolution #97393

cjgillot commented May 25, 2022 •

edited

Loading

rust-highfive commented May 25, 2022

rust-highfive commented May 25, 2022

This comment has been minimized.

petrochenkov commented May 25, 2022

jackh726 commented May 25, 2022

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

cjgillot commented Jun 4, 2022

rust-timer commented Jun 4, 2022

bors commented Jun 4, 2022

bors commented Jun 4, 2022

rust-timer commented Jun 4, 2022

rust-timer commented Jun 4, 2022

petrochenkov commented Jun 4, 2022

cjgillot Jun 4, 2022

cjgillot Jun 5, 2022

cjgillot Jun 5, 2022

cjgillot commented Jun 5, 2022

jackh726 left a comment

jackh726 Jun 5, 2022

jackh726 Jun 5, 2022

jackh726 commented Jun 26, 2022

	desc { "looking up lifetime defaults for a region on an item" }
	desc { "compute object lifetime defaults for a generic parameter" }

Separate object lifetime default from lifetime resolution #97393

Separate object lifetime default from lifetime resolution #97393

Conversation

cjgillot commented May 25, 2022 • edited Loading

rust-highfive commented May 25, 2022

rust-highfive commented May 25, 2022

This comment has been minimized.

petrochenkov commented May 25, 2022

jackh726 commented May 25, 2022

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

cjgillot commented Jun 4, 2022

rust-timer commented Jun 4, 2022

bors commented Jun 4, 2022

bors commented Jun 4, 2022

rust-timer commented Jun 4, 2022

rust-timer commented Jun 4, 2022

Footnotes

petrochenkov commented Jun 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjgillot commented Jun 5, 2022

jackh726 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackh726 commented Jun 26, 2022

cjgillot commented May 25, 2022 •

edited

Loading