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.

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

Tracking issue for Captures trait #56046

Closed
3 tasks
alexreg opened this issue Nov 18, 2018 · 6 comments
Closed
3 tasks

Tracking issue for Captures trait #56046

alexreg opened this issue Nov 18, 2018 · 6 comments

Comments

@alexreg
Copy link
Contributor

@alexreg alexreg commented Nov 18, 2018

This solution was initially suggested by @nikomatsakis in this comment.

Steps:

  • Implement the feature.
  • Adjust documentation.
  • Stabilize feature.

Questions:

  • Is this a permanent solution?
  • Does this need a FCP?
  • Can we get a minimal use case for this feature?

CC @alexcrichton

@alexreg
Copy link
Contributor Author

@alexreg alexreg commented Nov 18, 2018

Maybe someone in @rust-lang/libs could kindly add the appropriate tags?

@ExpHP
Copy link
Contributor

@ExpHP ExpHP commented Nov 20, 2018

Github is making the comments a nuisance to navigate. Is there a cliff-notes version of how this is different from impl Trait + 'a, and the problem it solves?

(I imagine that, since it's a trait bound, it introduces invariance over 'a as opposed to covariance?)

@alexreg
Copy link
Contributor Author

@alexreg alexreg commented Nov 20, 2018

@ExpHP I still don't 100% Niko's comment, but I think you're roughly right at least. It's definitely related to invariance somehow. Here is the comment, reproduced.

So #49041 contains a fix for #46541, but that fix has more impact than I anticipated -- e.g., the compiler doesn't bootstrap now -- and it's giving me a measure of pause about the right course here. The problem in #49041 is that we could accidentally allow lifetimes to leak out that we were not supposed to. Here is how this manifests in the compiler. We might have a method like this:

impl TyCtxt<'cx, 'gcx, 'tcx>
where 'gcx: 'tcx, 'tcx: 'cx
{
    fn foos(self) -> impl Iterator<Item = &'tcx Foo> + 'cx {
        /* returns some type `Baz<'cx, 'gcx, 'tcx>` that captures self */
    }
}

The key thing here is that TyCtxt is invariant w/r/t 'tcx and 'gcx, so they must appear in the return type. And yet only 'cx and 'tcx appear in the impl trait bounds, so only those two lifetimes are supposed to be "captured". The old compiler was accepting this because 'gcx: 'cx, but that's not really correct if you think about the desugaring we have in mind. That desugaring would create an abstract type like this:

abstract type Foos<'cx, 'tcx>: Iterator<Item = &'tcx Foo> + 'cx;

and yet the value for this abstract type would be Baz<'cx, 'gcx, 'tcx> -- but 'gcx is not in scope!

The workaround here is that we have to name 'gcx in the bounds. This is kind of annoying to do; we can't use 'cx + 'gcx. We can I suppose make a dummy trait:

trait Captures<'a> { }
impl<T: ?Sized> Captures<'a> for T { }

and then return something like this impl Iterator<Item = &'tcx Foo> + Captures<'gcx> + Captures<'cx>.

@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 11, 2019

We do have this trait in

pub trait Captures<'a> { }

If we move it to core::marker (unstably) we can use it in the printing of async fn return types, which currently just emit nothing:

hir::TyKind::Def(..) => {},
So with Captures available in core, an async fn foo<'a>() -> Bar could emit something like fn foo<'a>() -> impl SomeGeneratorThingy<Bar> + Captures<'a>

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Jun 11, 2019

Since this trait is defined in librustc, it doesn’t appear to be part of the standard library at all. I’ve changed the team labels accordingly, please revert if this is in error.

Or is the plan to move the trait to core::marker? Why is this a tracking issue in the first place?

impl Trait in return type position is now stable. Is it unsound when this kind of lifetimes are involved?

@alexreg
Copy link
Contributor Author

@alexreg alexreg commented Jun 11, 2019

@SimonSapin Thanks. However, this issue is really superseded by #60670.

And no, there is no unsoundness. The point is it is overly-restrictive at present.

@alexreg alexreg closed this Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants