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.

Sign up for GitHub

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

Jump to bottom

Move object lifetime default computation #97839

Closed
wants to merge 8 commits into from
Closed

Move object lifetime default computation #97839

wants to merge 8 commits into from

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Jun 7, 2022

Split from #97393

r? @jackh726

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 7, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 7, 2022
cjgillot
cjgillot commented Jun 7, 2022
View reviewed changes
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[&def_id];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the right index here.

jackh726
jackh726 reviewed Jun 7, 2022
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments.

compiler/rustc_resolve/src/late/lifetimes.rs Outdated
Comment on lines 775 to 777
// use the object lifetime defaulting
// rules. So e.g., `Box<dyn Debug>` becomes
// `Box<dyn Debug + 'static>`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment should be removed, I think.

compiler/rustc_typeck/src/astconv/mod.rs Show resolved Hide resolved
compiler/rustc_typeck/src/astconv/mod.rs Show resolved Hide resolved
compiler/rustc_typeck/src/collect.rs Outdated
Comment on lines 1351 to 1356
if let hir::LifetimeName::ImplicitObjectLifetimeDefault = lt.name {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Is this true? I could imagine a scenario like

struct Foo<'a, T: 'a> {}
trait Bar {}

let _: for<'x> fn(Foo<'x, dyn Bar>);

Does that apply here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is only used for function item-likes.

I agree, this is slightly wrong. However, this cannot cause a bug: ImplicitObjectLifetimeDefault can only reference a lifetime from an outer & reference, a generic argument, or 'static. So the lifetime this references has already been seen by the visitor.

However, this whole visitor will be useless once #97720 lands. All the lifetimes in a function's signature will be generic parameters, so only the loop in has_late_bound_regions will be required.

compiler/rustc_middle/src/query/mod.rs Outdated
/// Fetch the lifetimes for all the trait objects in an item-like.
query object_lifetime_map(_: LocalDefId) -> FxHashMap<ItemLocalId, Region> {
storage(ArenaCacheSelector<'tcx>)
desc { "looking up lifetime defaults for a region on an item" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this description correct?

It's probably worth elaborating a bit in a comment what the different between object_lifetime_map and object_lifetime_defaults are.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth elaborating a bit in a comment what the different between object_lifetime_map and object_lifetime_defaults are.

This is still relevant

src/test/ui/issues/issue-41139.rs Outdated
//~^ ERROR expected function, found `&dyn Fn() -> (dyn Trait + 'static)`
//~^ ERROR expected function, found `&dyn Fn() -> dyn Trait`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change? Not a huge deal, but would be nice to know why

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, do you mean the existing test is a bug? Or the new output is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new output is buggy.

@cjgillot cjgillot force-pushed the object-lifetime-default-small branch from fa5dff6 to fc2713e Compare June 8, 2022 17:56
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot cjgillot force-pushed the object-lifetime-default-small branch from fc2713e to 5d5eda8 Compare June 8, 2022 19:43
@jackh726
Copy link
Member

Gonna try to take another look at this PR this weekend.

jackh726
jackh726 approved these changes Jun 13, 2022
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking pretty good. Just two small comments. Also, can you merge some commits (pacify tidy, fix comments and query description) into other relevant commits. Not required, but would be nice. r=me after review addressed

compiler/rustc_middle/src/query/mod.rs Outdated
/// Fetch the lifetimes for all the trait objects in an item-like.
query object_lifetime_map(_: LocalDefId) -> FxHashMap<ItemLocalId, Region> {
storage(ArenaCacheSelector<'tcx>)
desc { "looking up lifetime defaults for a region on an item" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably worth elaborating a bit in a comment what the different between object_lifetime_map and object_lifetime_defaults are.

This is still relevant

compiler/rustc_typeck/src/astconv/mod.rs Outdated
let tcx = self.tcx();
if let hir::LifetimeName::ImplicitObjectLifetimeDefault = lifetime.name {
panic!(
"ImplicitObjectLifetimeDefault should not handled separately from ast_region_to_region."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/not/be?

@cjgillot cjgillot force-pushed the object-lifetime-default-small branch from 5d5eda8 to 1f535a0 Compare June 14, 2022 19:23
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cjgillot cjgillot force-pushed the object-lifetime-default-small branch from 1f535a0 to 37f42cb Compare June 14, 2022 19:44
@cjgillot
Copy link
Contributor Author

I'm having doubts whether this is the right direction. I'm marking this as blocked until I have a proper idea of where we are going.

@cjgillot cjgillot 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 14, 2022
@jackh726 jackh726 mentioned this pull request Jun 26, 2022
Closed
@bors
Copy link
Contributor

bors commented Jul 25, 2022

☔ The latest upstream changes (presumably #97313) made this pull request unmergeable. Please resolve the merge conflicts.

@cjgillot cjgillot closed this Jul 27, 2022
@cjgillot cjgillot deleted the object-lifetime-default-small branch July 27, 2022 20:10
@cjgillot cjgillot mentioned this pull request Jul 27, 2022
Merged
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 29, 2022
@Dylan-DPC
Rollup merge of rust-lang#99821 - cjgillot:ast-lifetimes-2, r=compile…
8bd80ab 
…r-errors

Remove separate indexing of early-bound regions

~Based on rust-lang#99728

This PR copies some modifications from rust-lang#97839 around object lifetime defaults.
These modifications allow to stop counting generic parameters during lifetime resolution, and rely on the indexing given by `rustc_typeck::collect`.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 29, 2022
@Dylan-DPC
Rollup merge of rust-lang#99821 - cjgillot:ast-lifetimes-2, r=compile…
5555e13 
…r-errors

Remove separate indexing of early-bound regions

~Based on rust-lang#99728

This PR copies some modifications from rust-lang#97839 around object lifetime defaults.
These modifications allow to stop counting generic parameters during lifetime resolution, and rely on the indexing given by `rustc_typeck::collect`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackh726 jackh726 jackh726 approved these changes

Assignees

@jackh726 jackh726

Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cjgillot @rust-log-analyzer @jackh726 @bors @rust-highfive @rustbot