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

jackh726 · 2021-03-04T03:43:48Z

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 · 2021-03-04T03:48:23Z

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/find_anon_type.rs

@@ -97,17 +94,17 @@ impl Visitor<'tcx> for FindNestedTypeVisitor<'tcx> {
    fn visit_ty(&mut self, arg: &'tcx hir::Ty<'tcx>) {
        match arg.kind {
            hir::TyKind::BareFn(_) => {
-                self.current_index.shift_in(1);
+                if matches!(self.bound_region, ty::BrAnon(_)) {
+                    // Anonymous regions can't refer across `fn` boundaries


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)

compiler/rustc_middle/src/query/mod.rs

jackh726 · 2021-03-04T03:52:07Z

compiler/rustc_resolve/src/late/lifetimes.rs

-
-    let named_region_map = krate(tcx);
+#[tracing::instrument(level = "debug", skip(tcx))]
+fn resolve_lifetimes(tcx: TyCtxt<'_>, local_def_id: LocalDefId) -> ResolveLifetimes {


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

compiler/rustc_resolve/src/late/lifetimes.rs

jackh726 · 2021-03-04T03:57:58Z

src/test/ui/error-codes/E0106.stderr

@@ -61,6 +50,17 @@ LL |
 LL |     buzz: Buzz<'a, 'a>,
   |

+error[E0106]: missing lifetime specifier


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

jackh726 · 2021-03-04T04:03:47Z

src/test/ui/methods/method-call-lifetime-args-unresolved.stderr

@@ -1,3 +1,18 @@
+warning: cannot specify lifetime arguments explicitly if late bound lifetime parameters are present


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 · 2021-03-04T04:05:31Z

src/test/ui/mismatched_types/issue-74918-missing-lifetime.stderr

@@ -9,22 +9,6 @@ help: consider introducing a named lifetime parameter
 LL |     type Item<'a> = IteratorChunk<'a, T, S>;
   |              ^^^^                 ^^^

-error: `impl` item signature doesn't match `trait` item signature


Same thing happens as in no_introducing_in_band_in_locals.rs.

jackh726 · 2021-03-04T04:05:55Z

src/test/ui/regions/regions-name-undeclared.stderr

@@ -1,3 +1,23 @@
+error[E0261]: use of undeclared lifetime name `'a`


Just rearranged. Order makes more sense.

jackh726 · 2021-03-04T04:06:57Z

src/test/ui/suggestions/lifetimes/missing-lifetimes-in-signature.stderr

   |
 LL | fn baz<G: 'a, T>(g: G, dest: &mut T) -> impl FnOnce() + '_
   |        -  ^^ undeclared lifetime
   |        |
   |        help: consider introducing lifetime `'a` here: `'a,`

-error[E0759]: `dest` has an anonymous lifetime `'_` but it needs to satisfy a `'static` lifetime requirement


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 · 2021-03-04T04:07:21Z

src/test/ui/suggestions/missing-lifetime-specifier.stderr

@@ -142,30 +142,6 @@ help: consider using the `'static` lifetime
 LL |     static d: RefCell<HashMap<i32, Vec<Vec<&Tar<'static, 'static, i32>>>>> = RefCell::new(HashMap::new());
   |                                                 ^^^^^^^^^^^^^^^^^

-error[E0106]: missing lifetime specifier


Just rearranged.

jackh726 · 2021-03-05T03:53:59Z

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 · 2021-03-06T15:11:55Z

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 · 2021-03-06T22:55:54Z

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 · 2021-03-11T21:35:04Z

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 · 2021-03-11T22:36:35Z

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 · 2021-03-11T22:40:29Z

@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. =)

compiler/rustc_middle/src/query/mod.rs

nikomatsakis · 2021-03-12T10:17:02Z

compiler/rustc_resolve/src/late/lifetimes.rs

@@ -176,6 +174,8 @@ crate struct LifetimeContext<'a, 'tcx> {

    is_in_const_generic: bool,

+    definition_only: bool,


nikomatsakis · 2021-03-12T10:21:47Z

compiler/rustc_resolve/src/late/lifetimes.rs

+fn resolve_lifetimes_definition(tcx: TyCtxt<'_>, local_def_id: LocalDefId) -> ResolveLifetimes {
+    do_resolve(tcx, local_def_id, true)
+}
+
 /// Computes the `ResolveLifetimes` map that contains data for the


This comment looks out of date, right?

nikomatsakis · 2021-03-12T10:23:15Z

compiler/rustc_resolve/src/late/lifetimes.rs

    rl
 }

+fn resolve_lifetimes_for<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId) -> &'tcx ResolveLifetimes {


compiler/rustc_resolve/src/late/lifetimes.rs

nikomatsakis · 2021-03-12T10:38:35Z

compiler/rustc_resolve/src/late/lifetimes.rs

@@ -392,6 +469,12 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> {
    // We want to nest trait/impl items in their parent, but nothing else.
    fn visit_nested_item(&mut self, _: hir::ItemId) {}

+    fn visit_trait_item_ref(&mut self, ii: &'tcx hir::TraitItemRef) {
+        if !self.definition_only {


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 · 2021-03-12T10:39:06Z

compiler/rustc_resolve/src/late/lifetimes.rs

-                                             bound at the fn or impl level"
-                                    )
-                                    .emit();
+                                    if !self.definition_only {


Why skip this error for traits?

nikomatsakis · 2021-03-12T10:39:14Z

compiler/rustc_resolve/src/late/lifetimes.rs

@@ -897,7 +1007,9 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> {
    }

    fn visit_generics(&mut self, generics: &'tcx hir::Generics<'tcx>) {
-        check_mixed_explicit_and_in_band_defs(self.tcx, &generics.params);
+        if !self.definition_only {


Again, why skip this for traits?

nikomatsakis · 2021-03-12T10:39:30Z

compiler/rustc_resolve/src/late/lifetimes.rs

+        {
+            let _enter = span.enter();
+            f(self.scope, &mut this);
+            if !self.definition_only {


Why skip this for traits?

nikomatsakis · 2021-03-12T10:39:36Z

compiler/rustc_resolve/src/late/lifetimes.rs

@@ -1859,7 +1973,7 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
            }

            // Check for fn-syntax conflicts with in-band lifetime definitions
-            if self.is_in_fn_syntax {
+            if !self.definition_only && self.is_in_fn_syntax {


Why skip this for traits?

bors · 2021-03-18T08:24:41Z

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

This reverts commit 22ae20733515d710c1134600bc1e29cdd76f6b9b.

jackh726 · 2021-03-24T21:18:34Z

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 · 2021-03-24T21:18:35Z

Awaiting bors try build completion.

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

bors · 2021-03-24T21:18:46Z

⌛ Trying commit cfbd0ee with merge 21a1e699b9e794c09680c90fe7bcf6a375b53f95...

bors · 2021-03-24T22:08:38Z

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

rust-timer · 2021-03-24T22:08:39Z

Queued 21a1e699b9e794c09680c90fe7bcf6a375b53f95 with parent f5fe425, future comparison URL.

rust-timer · 2021-03-24T23:52:16Z

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 · 2021-03-25T13:31:53Z

@bors r+

bors · 2021-03-25T13:31:54Z

📌 Commit 7cb8f51 has been approved by nikomatsakis

bors · 2021-03-25T16:04:26Z

⌛ Testing commit 7cb8f51 with merge e2d2189e07afb2e52b5af38eb607fe44fd64799e...

bors · 2021-03-25T16:35:14Z

💔 Test failed - checks-actions

jackh726 · 2021-03-25T18:12:12Z

@bors r=nikomatsakis

bors · 2021-03-25T18:12:14Z

📌 Commit 44e9d20 has been approved by nikomatsakis

bors · 2021-03-25T19:28:21Z

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

bors · 2021-03-25T22:09:20Z

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

rust-highfive assigned nikomatsakis Mar 4, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2021

jackh726 commented Mar 4, 2021

View reviewed changes

petrochenkov self-assigned this Mar 4, 2021

petrochenkov removed their assignment Mar 6, 2021

jackh726 changed the title ~~[WIP] Refactor rustc_resolve::late::lifetimes to resolve per-item~~ Refactor rustc_resolve::late::lifetimes to resolve per-item Mar 11, 2021

jackh726 force-pushed the resolve-refactor branch from 5618452 to 51ba5c6 Compare March 11, 2021 21:29

nikomatsakis reviewed Mar 12, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

jackh726 force-pushed the resolve-refactor branch 2 times, most recently from 5a8ad2a to c6ec953 Compare March 15, 2021 18:48

jackh726 mentioned this pull request Mar 22, 2021

Refactor Binder to track bound vars #76814

Merged

jackh726 added 2 commits March 24, 2021 16:45

resolve late lifetimes by item

19ecfcd

This reverts commit 22ae20733515d710c1134600bc1e29cdd76f6b9b.

Review comments

cfbd0ee

jackh726 force-pushed the resolve-refactor branch from c6ec953 to cfbd0ee Compare March 24, 2021 20:46

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 24, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 24, 2021

write-up what is happening

7cb8f51

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 25, 2021

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 25, 2021

Bless nll test

44e9d20

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 25, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 25, 2021

bors merged commit 52e3dff into rust-lang:master Mar 25, 2021

rustbot added this to the 1.53.0 milestone Mar 25, 2021

bors mentioned this pull request Mar 25, 2021

Improve diagnostics for GATs #82272

Merged

jackh726 deleted the resolve-refactor branch March 25, 2021 22:28

		@@ -1,3 +1,18 @@
		warning: cannot specify lifetime arguments explicitly if late bound lifetime parameters are present

		@@ -1,3 +1,23 @@
		error[E0261]: use of undeclared lifetime name `'a`

		@@ -176,6 +174,8 @@ crate struct LifetimeContext<'a, 'tcx> {

		is_in_const_generic: bool,

		definition_only: bool,

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

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

Conversation

jackh726 commented Mar 4, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackh726 commented Mar 5, 2021

petrochenkov commented Mar 6, 2021 • edited Loading

jackh726 commented Mar 6, 2021

jackh726 commented Mar 11, 2021

cjgillot commented Mar 11, 2021

jackh726 commented Mar 11, 2021

nikomatsakis 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

This comment has been minimized.

bors commented Mar 18, 2021

jackh726 commented Mar 24, 2021

rust-timer commented Mar 24, 2021

bors commented Mar 24, 2021

bors commented Mar 24, 2021

rust-timer commented Mar 24, 2021

rust-timer commented Mar 24, 2021

nikomatsakis commented Mar 25, 2021

bors commented Mar 25, 2021

bors commented Mar 25, 2021

This comment has been minimized.

bors commented Mar 25, 2021

jackh726 commented Mar 25, 2021

bors commented Mar 25, 2021

bors commented Mar 25, 2021

bors commented Mar 25, 2021

jackh726 commented Mar 4, 2021 •

edited

Loading

petrochenkov commented Mar 6, 2021 •

edited

Loading