Correctly check auto traits on generator interiors #92449

compiler-errors · 2021-12-30T23:36:52Z

Take 2 at Fixing #71723

Took a stab at fixing #71723, since it was annoying me while I was writing async code with futures::stream.

(note: edited to reflect the new solution I wrote.)

The issue

The issue here is when a struct has a projection type in it. After we erase the regions within a generator-interior type, the projection type may no longer hold, since solving/normalizing that projection type may depend on region relations that are now erased from the type.

The solution

I created a helper type called StructuralPredicateElaborator that walks through the generator-interior types structurally (e.g. recurses on a type's constituents). Every time it sees a projection type, it turns that into a ProjectionPredicate that corresponds to the normalized type.

We then later augment the param-env during auto trait confirmation with these projection types, so that we always normalize a projection type to the type that it would've normalized to inside of the generator.

Previous solution

In my previous solution, I was replacing the placeholders with new region variables. This ended up being unsound, because there was no way of enforcing dropck on the new variables we instantiated.

r? @nikomatsakis
Feel free to reassign, chose you because you currently own #71723.

Repo tracking some issue code: https://github.com/compiler-errors/pr-92449
Fixes #60658
Fixes #71723
Fixes #79648
Fixes #82921
Fixes #90696

That being said, this attempt number 2 does not fix:
#64552
#87425
#92415
#92096

compiler-errors · 2021-12-30T23:37:35Z

Maybe I'm totally off the mark with what I'm doing in this diff. I would love feedback and help towards getting a correct solution, if there are significant issues with my implementation. :)

src/test/ui/generator/auto-trait-regions.rs

Aaron1011 · 2022-01-06T18:05:40Z

cc @rust-lang/wg-traits

compiler-errors · 2022-01-06T18:40:53Z

src/test/ui/generator/auto-trait-regions.rs

-    assert_foo(gen);
-    //~^ ERROR not general enough
-    //~| ERROR not general enough
+    assert_foo(gen); // bad -- why does this pass?


So I was thinking about this test case recently.

I think why this doesn't fail is because we don't actually enforce any dropck... So there's nothing enforcing the lifetime needs to be shorter than 'static.

Wonder if there's any way of hooking that information in during auto trait confirmation.

If I'm understanding this right, this seems concerning. So the issue is that a is !Foo because of the No field included in the struct. Then a obviously lives across the yield point since we explicitly drop it afterwards. This means a needs to be saved in the generator, so the resulting generator should be !Foo as well, but yet assert_foo(gen) passes...

Oh, but then we explicitly impl Foo for A, which overrides the auto rules, right? However, the impl shouldn't apply because the Cell::new(&mut y) field is not bound by 'static since y ends at the end of the generator.

I agree with you, it seems like this shouldn't pass, and it seems likely to permit undefined behavior if it does...

Maybe the compiler is doing something smart elsewhere by recognizing that a is destructed at the end of the generator as well so it can automatically shorten the lifetimes? That seems like that'd be extremely difficult to do safely though, so that seems unlikely to me.

Yeah, this is because of the fact we refresh the late bound regions with totally new inference variables. While stashing away all of the region constraints we collect in generator_interior is okay to enforce most borrowchecky things, we still can't enforce anything like requiring one local variable living for longer than another (or static) in the generator interior with my implementation as-is.

But yeah, this one's the case that I need someone who knows borrowck/dropck better to help with.

bors · 2022-01-10T15:45:01Z

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

jackh726 · 2022-01-10T16:05:53Z

@compiler-errors does this happen to fix #92096?

compiler-errors · 2022-01-10T17:38:42Z

@jackh726 it does fix it, I'll add it to the list.

jackh726 · 2022-01-10T18:23:48Z

😃🎉 wow nice

eholk · 2022-01-10T19:42:23Z

I read over this PR since it touches generator_interior.rs and I've been working in that file a lot lately too. I'm not an expert in most of the areas this PR touches, but from at least a surface level it looks good. It's a minor change, but I appreciate that you make the use of inner more consistent throughout the code. That test case issue you pointed out is weird though, as I mentioned in the other comment.

bors · 2022-01-11T18:10:50Z

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

compiler-errors · 2022-01-14T05:02:37Z

I wonder if we could/need to limit this to purely structural impls for Send and Sync (and perhaps other auto traits with no implied lifetime obligations), because of the limitations that I pointed out in that weird test case.

I think that would still address all of these async+Future/Stream combinator cases without introducing unsoundness...

jackh726 · 2022-01-14T15:37:30Z

This is on my list of things to look at...just need to find time.

nikomatsakis · 2022-01-14T21:59:02Z

I have this on my list to review, sorry for being slow, I'm trying to free up more time for reviewing and the like!

compiler-errors · 2022-01-17T05:40:13Z

No worries, and thanks for the followup comment @nikomatsakis. I understand you're probably quite busy after the holidays, so take your time getting around to review this PR. Looking forward to your review. 😃

That being said, I reworked this PR with an alternative idea that doesn't use fresh region variables, to address the unsoundness that @eholk and I talked about in previous review comments.

NOTE: While I think this approach is far more solid, some of the issues I had originally claimed to fix are no longer passing. I have a pretty good understanding of why each of these tests continue to fail, though I probably need some help brainstorming how to modify this PR to make them pass.

bors · 2022-01-18T13:53:15Z

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

compiler/rustc_infer/src/infer/region_constraints/mod.rs

nikomatsakis

Didn't get all the way through yet -- first round of interim comments!

nikomatsakis · 2022-01-28T18:51:10Z

compiler/rustc_middle/src/ty/sty.rs

@@ -2308,3 +2308,10 @@ impl<'tcx> VarianceDiagInfo<'tcx> {
        }
    }
 }
+
+#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]


Pre-existing, but we really need a comment here. It'd be good to explain the role of this type and each of its fields.

compiler/rustc_middle/src/ty/sty.rs

compiler/rustc_middle/src/ty/walk.rs

nikomatsakis · 2022-01-28T18:58:05Z

compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs

-                    self.tcx().erase_late_bound_regions(*tys).iter().map(|t| (t, depth + 1)),
+                ty::GeneratorWitness(interior) => stack.extend(
+                    self.tcx()
+                        .erase_late_bound_regions(*interior)


probably makes sense to "select" the types and erase only those -- but maybe we want to include the predicates here too? not obvious to me that we don't

I actually rewrote this code in #93028, so this comment no longer applies I think. I'll rebase this PR.

nikomatsakis · 2022-01-28T18:58:55Z

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

+
+                    let param_env = obligation.param_env;
+                    // Augment the param env with the projections that we know hold structurally
+                    // in the generator interiors' types.


What do you mean by "hold structurally"?

compiler/rustc_middle/src/ty/sty.rs

compiler-errors · 2022-01-28T21:30:32Z

One interesting case that this PR still doesn't work is when our generator has a BTreeMap in it, since the nodes inside of the BTreeMap have an unsafe impl Send which relates some regions we know nothing about during generator interior analysis.

compiler-errors · 2022-01-28T22:22:41Z

rebase and add a few more comments.

nikomatsakis

OK, round two of review done. I now understand what you're doing at a high-level. Quite clever! I hadn't really considered this approach. I have to think about it and do some local experimentation. I expect to return to this PR on Wednesday for further review.

nikomatsakis · 2022-01-31T16:07:02Z

compiler/rustc_typeck/src/check/generator_interior.rs

+    // Extract the projection types that we know hold in the interior types, so we can reapply them
+    // later. This step is important because when we erase regions, we might no longer be able to
+    // apply some projections (since they depend on region knowledge we erased). But since we know
+    // they hold now, they should hold forever, so we'll add these projections to the param-env later.


It's not clear to me that this is true. The region relationships aren't really checked now, they are checked later, in MIR borrow check. I have to ponder whether I can make a failing test case, though, it may be that we wind up enforcing the resulting constraints in MIR borrow check as well. If not, I think we could fix it by "reproving" these predicates.

it may be that we wind up enforcing the resulting constraints in MIR borrow check as well

Yeah that's what I thought would happen here. What I thought was: when we normalize the type, it registers obligations into the fcx with the local (non-erased) region variables. If we had a bad projection type, then we would have a borrowck error regardless, so it's fine to treat this as "solved" when we check the generator interior later on.

nikomatsakis · 2022-01-31T16:07:33Z

compiler/rustc_typeck/src/check/generator_interior/structural_predicate.rs

+    /// Bar<i32> where struct Bar<T> { x: T, y: u32 } -> [i32, u32]
+    /// Zed<i32> where enum Zed { A(T), B(u32) } -> [i32, u32]
+    /// ```
+    fn constituent_types_for_auto_trait(&self, t: Ty<'tcx>) -> Vec<Ty<'tcx>> {


This is a duplicate of logic in trait checking, which doesn't seem great.

Yeah, it's mostly a duplicate. I just needed some way of walking thru the types inside of a type (and into an ADT's field). Maybe I can simplify this into just a type visitor that pushes more types onto a stack if it sees an ADT.

nikomatsakis · 2022-01-31T16:11:40Z

compiler/rustc_trait_selection/src/traits/select/confirmation.rs

+                        // or just make a new param_env from these predicates...
+                        // Some tests fail if we do the former, but doing the latter doesn't
+                        // sound like it works 100% either...
+                        let new_param_env = ty::ParamEnv::new(


This logic of being able to handle implication is one of the things that I wanted to add to the compiler via chalk, fwiw, but it may be possible to do it locally this way.

bors · 2022-02-08T02:54:50Z

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

Dylan-DPC · 2022-03-21T02:05:01Z

@compiler-errors what's the status on this? would be nice if you can resolve the conflicts as well

compiler-errors · 2022-03-21T02:06:57Z

still waiting on review technically, though i'm not sure if it's the correct approach. i can rebase it though, it's been quite a while since i've touched this code.

compiler-errors · 2022-03-24T04:50:31Z

I'm gonna mark this S-experimental for now...

@rustbot label -S-waiting-on-review +S-experimental

compiler-errors · 2022-06-03T13:47:08Z

Closing this since it needs to be reworked like crazy.

rust-highfive assigned nikomatsakis Dec 30, 2021

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 30, 2021

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

compiler-errors changed the title ~~Check auto traits on generator interiors on non-~~ Check auto traits on generator interiors Dec 30, 2021

compiler-errors changed the title ~~Check auto traits on generator interiors~~ Correctly check auto traits on generator interiors Dec 30, 2021

compiler-errors commented Dec 31, 2021

View reviewed changes

src/test/ui/generator/auto-trait-regions.rs Outdated Show resolved Hide resolved

compiler-errors force-pushed the generator-auto-trait branch from ac0b1b4 to 38827e5 Compare December 31, 2021 03:20

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the generator-auto-trait branch from 38827e5 to 2d7a0bf Compare December 31, 2021 03:38

compiler-errors mentioned this pull request Jan 1, 2022

Implementation of trait is not general enough when boxing future returning <T as Foo<'a>>::Foo #92415

Open

compiler-errors commented Jan 6, 2022

View reviewed changes

compiler-errors mentioned this pull request Jan 7, 2022

Compiler sometimes does not have generic_const_exprs and await play nice together #92634

Closed

compiler-errors force-pushed the generator-auto-trait branch from 2d7a0bf to 465c97f Compare January 10, 2022 18:14

compiler-errors force-pushed the generator-auto-trait branch from 465c97f to 4926cb3 Compare January 11, 2022 18:18

compiler-errors force-pushed the generator-auto-trait branch from 4926cb3 to c44c2d6 Compare January 17, 2022 05:34

estebank reviewed Jan 20, 2022

View reviewed changes

compiler/rustc_infer/src/infer/region_constraints/mod.rs Outdated Show resolved Hide resolved

nikomatsakis reviewed Jan 28, 2022

View reviewed changes

compiler-errors added 3 commits January 28, 2022 12:02

Fix projection types when checking generator auto-traits

cdb1f13

Fixup and add new tests for generator auto-trait

87d1317

Generalize the kinds of predicates we stash in the generator interior

8e1cabc

Add comments, adjust names

9e72ef4

compiler-errors force-pushed the generator-auto-trait branch from 8e963ea to 9e72ef4 Compare January 28, 2022 22:22

nikomatsakis reviewed Jan 31, 2022

View reviewed changes

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 20, 2022

rustbot added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 24, 2022

compiler-errors closed this Jun 3, 2022

compiler-errors deleted the generator-auto-trait branch November 2, 2022 02:51

zirconium-n mentioned this pull request Nov 15, 2022

async block with futures::stream::Buffered is not Send #104382

Open

This was referenced Apr 14, 2023

Tracking issue for incorrect lifetime bound errors in async #110338

Open

"Implementation not general enough" with async and projection, fixed by wrapping with async move #110339

Open

Correctly check auto traits on generator interiors #92449

Correctly check auto traits on generator interiors #92449

Conversation

compiler-errors commented Dec 30, 2021 • edited Loading

Take 2 at Fixing #71723

The issue

The solution

Previous solution

compiler-errors commented Dec 30, 2021

This comment has been minimized.

Aaron1011 commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jan 10, 2022

jackh726 commented Jan 10, 2022

compiler-errors commented Jan 10, 2022

jackh726 commented Jan 10, 2022

eholk commented Jan 10, 2022

bors commented Jan 11, 2022

compiler-errors commented Jan 14, 2022

jackh726 commented Jan 14, 2022

nikomatsakis commented Jan 14, 2022

compiler-errors commented Jan 17, 2022 • edited Loading

bors commented Jan 18, 2022

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

compiler-errors commented Jan 28, 2022 • edited Loading

compiler-errors commented Jan 28, 2022

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

bors commented Feb 8, 2022

Dylan-DPC commented Mar 21, 2022

compiler-errors commented Mar 21, 2022

compiler-errors commented Mar 24, 2022

compiler-errors commented Jun 3, 2022

compiler-errors commented Dec 30, 2021 •

edited

Loading

compiler-errors commented Jan 17, 2022 •

edited

Loading

compiler-errors commented Jan 28, 2022 •

edited

Loading