Refactor rustc_resolve::late::lifetimes to resolve per-item

[jackh726]

There are some changes to tests that I'd like some feedback on; so this is still WIP.

The reason behind this change will (hopefully) allow us to (as part of #76814) be able to essentially use the lifetime resolve code to resolve all late bound vars (including those of super traits). Currently, it only resolves those that are syntactically in scope. In #76814, I'm essentially finding that I would essentially have to redo the passing of bound vars through scopes (i.e. when instantiating a poly trait ref), and that's what this code does anyways. However, to be able to do this (ask super traits what bound vars are in scope), we have to be able to resolve items separately.

The first commit is actually partially orthogonal. Essentially removing one use of late bound debruijn indices.

Not exactly sure who would be best to review here.
Let r? @nikomatsakis

[jackh726]

This is because BrAnons come from an Elision scope when resolving, which get shadowed when we pass a fn boundary; BrNamed regions can refer across fn boundaries though (but we don't need the db index, we can just compare DefId)

[jackh726]

We need to resolve lifetimes per Item, because of Params and ReEarlyBounds are relevant

[jackh726]

Same error is emitted, but now at the end; so, line numbers for errors are no long sequential.

[jackh726]

So...this is actually more correct? We are trying to specially a late-bound lifetime in turbofish. It's just interesting this is now being emitted.

[jackh726]

Same thing happens as in no_introducing_in_band_in_locals.rs.

[jackh726]

Just rearranged. Order makes more sense.

[jackh726]

Just lots of errors gone here. I assume same problem as other two tests: these errors probably get emitted in the wf_checking bit, which we don't reach because of the first error.

[jackh726]

Just rearranged.

[jackh726]

So, I'm finding (as I try to use this in #76814) that this doesn't quite do enough, because of cycles. Example:
In compiler_builtins, there are:

pub trait HInt: Int {
    type D: DInt<H = Self> + Int;
}
pub trait DInt: Int {
    type H: HInt<D = Self> + Int;
}

Basically, I want to be able to ask about DInt when looking at DInt<H = Self>, but can't quite do that in this PR.

So, this is a good start, but still some iteration to do.

[petrochenkov]

Not sure how strongly it affects this PR, but when lifetime resolution was moved to rustc_resolve the plan was to move lifetime resolution at least for named lifetimes to AST and then keep lifetime resolutions directly in HIR instead of some side tables.

This would mirror path resolution being performed on AST, and path resolutions being stored in HIR paths, because for named lifetimes the resolution rules are supposed to be exactly the same as for other names.
If "resolution" for implicit unnamed lifetimes cannot be easily done on AST, then it would need to be split to a separate pass.

If "per-item" lifetime resolution is done with granularity that is too high, I suspect it would break the "exactly the same as for other names", because type parameters of item A are in scope in all A's nested items (although there may be "barriers" making their use an error).

[jackh726]

Not sure how strongly it affects this PR, but when lifetime resolution was moved to rustc_resolve the plan was to move lifetime resolution at least for named lifetimes to AST and then keep lifetime resolutions directly in HIR instead of some side tables.

Yeah, so I think the crux of this is that while lifetime resolution could almost certainly be done in AST, "building up" how you represent those lifetimes in predicates cannot (in part because currently debruijn indices are used, which requires us to not only know what lifetimes are in scope and "usable", but what "level" they sit at; also, because in #76814, we begin to think about the implicit lifetimes in scope from super traits because we have to keep things in order). The late/lifetimes pass does some of lifetime resolution (resolve_lifetime_ref), but mainly is focused on keeping track of named and anonymous lifetimes. While the resolution bit of this pass could be moved to AST, I expect that most of this to have to stay in HIR.

This would mirror path resolution being performed on AST, and path resolutions being stored in HIR paths, because for named lifetimes the resolution rules are supposed to be exactly the same as for other names.
If "resolution" for implicit unnamed lifetimes cannot be easily done on AST, then it would need to be split to a separate pass.

If "per-item" lifetime resolution is done with granularity that is too high, I suspect it would break the "exactly the same as for other names", because type parameters of item A are in scope in all A's nested items (although there may be "barriers" making their use an error).

Yeah, so I think this could be a potentially fatal flaw (or at least - until the resolution of named lifetimes moves to AST) with this PR: nested items. Take the following code as an example:

fn foo<T>() {
    fn bar(_: T) {}
}

fn baz<'a>() {
    fn buzz(_: &'a ()) {}
}

bar gives the error: "can't use generic parameters from outer function" whereas buzz gives "use of undeclared lifetime name 'a". We probably want to ideally also give the same "can't use generic parameters from outer function" when trying to use 'a too; with the PR as-is, this wouldn't be possible and lifetime resolution would have to be moved to AST.

I wanted to do this so that I could avoid another pass in #76814, but it's quite tricky. Still have to experiment a bit.

[jackh726]

Okay, after using this in my branch working on #76814, I'm convinced that this PR is necessary.

I think eventually we probably want to move named lifetime resolution to ast, like types. But overall I think the rest of lifetime resolution makes sense in hir and it should be fine to do it per item.

Additionally, I've included a somewhat-related additional query resolve_lifetimes_definition (name bikesheddable) to specifically only resolve the definition of a trait, but not it's items. The is needed for #76814 since we have to be able to get a trait's supertrait HRTBs, without potentially inducing cycles.

@petrochenkov did you want to review this? Otherwise, maybe @nikomatsakis?

[cjgillot]

resolve_lifetimes_definition appears to do a subset of the work done in resolve_lifetimes. Can one query be reuse the result of the other?

[jackh726]

@cjgillot I've thought about that. And I can't think of a good way to do it so far. Ultimately though, it ends up being a small amount of overall work that gets duplicated, since it's only run for traits, and only for the definition, not the items.

[nikomatsakis]

This makes a lot of sense to me -- but I am wondering about definition_only. As far as I can tell, it's only true when we are checking traits, and it's used to skip a bunch of stuff that only seems to apply to functions and things that are not traits. I'd prefer not to have it. =)

[nikomatsakis]

Comment!

[nikomatsakis]

This comment looks out of date, right?

[nikomatsakis]

Comment!

[nikomatsakis]

Do we ever want to do this? When we are resolving the trait, definition_only is true, and I think trait items only appear as the children of a trait, right?

[nikomatsakis]

Why skip this error for traits?

[nikomatsakis]

Again, why skip this for traits?

[nikomatsakis]

Why skip this for traits?

[nikomatsakis]

Why skip this for traits?

[bors]

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

[jackh726]

Even though we have an upper bound for potential perf regressions from #76814, we are doing slightly more work here, so I'd like to run perf for peace of mind.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

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

[bors]

⌛ Trying commit cfbd0ee with merge 21a1e699b9e794c09680c90fe7bcf6a375b53f95...

[bors]

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

[rust-timer]

Queued 21a1e699b9e794c09680c90fe7bcf6a375b53f95 with parent f5fe425, future comparison URL.

[rust-timer]

Finished benchmarking try commit (21a1e699b9e794c09680c90fe7bcf6a375b53f95): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

[nikomatsakis]

@bors r+

[bors]

📌 Commit 7cb8f51 has been approved by nikomatsakis

[bors]

⌛ Testing commit 7cb8f51 with merge e2d2189e07afb2e52b5af38eb607fe44fd64799e...

[bors]

💔 Test failed - checks-actions

[jackh726]

@bors r=nikomatsakis

[bors]

📌 Commit 44e9d20 has been approved by nikomatsakis

[bors]

⌛ Testing commit 44e9d20 with merge 52e3dff...

[bors]

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 52e3dff to master...