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

Allow hidden lifetimes in impl Trait #57870

Closed
wants to merge 1 commit into from

Conversation

cramertj
Copy link
Member

This is a revert of fc3c90c, which made it an error
to have a hidden lifetime inside an impl Trait return
type even when the hidden lifetime outlived other lifetimes
mentioned by name in the impl Trait bound.

Partial revert of #49041.
cc @alexreg

r? @nikomatsakis

This is a revert of fc3c90c, which made it an error
to have a hidden lifetime inside an `impl Trait` return
type even when the hidden lifetime outlived other lifetimes
mentioned by name in the `impl Trait` bound.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2019
@alexreg
Copy link
Contributor

alexreg commented Jan 24, 2019

Looks good. Should we include a comment with some theoretical justification for this behaviour, as per @nikomatsakis's explanation on Zulip (implicit existential quantification over hidden lifetimes, etc.)?

@Centril Centril added T-lang Relevant to the language team, which will review and decide on the PR/issue. relnotes Marks issues that should be documented in the release notes of the next release. labels Jan 24, 2019
@Centril
Copy link
Contributor

Centril commented Jan 24, 2019

@cramertj @nikomatsakis This seems right; but do we want to discuss this on a meeting / fcp-merge since it affects user observable type system behavior? Should also document this in the reference or some place less ephemeral than the code?

EDIT: I've tentatively added needs-fcp but if for some reason you don't think so (+ add that reason) feel free to remove it.

@Centril Centril added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. I-nominated labels Jan 24, 2019
@matthewjasper
Copy link
Contributor

matthewjasper commented Jan 24, 2019

This isn't sound:

trait Swap: Sized {
    fn swap(self, other: Self);
}

impl<T> Swap for &mut T {
    fn swap(self, other: Self) {
        std::mem::swap(self, other);
    }
}

fn hide<'a, 'b, T: 'static>(x: &'a mut &'b T) -> impl Swap + 'a {
    x
}

fn dangle() -> &'static [i32; 3] {
    let mut res = &[4, 5, 6];
    let x = [1, 2, 3];
    hide(&mut res).swap(hide(&mut &x));
    res
}

fn main() {
    println!("{:?}", dangle()); // prints nonsense values
}

@Centril Centril removed I-nominated relnotes Marks issues that should be documented in the release notes of the next release. labels Jan 24, 2019
@alexreg
Copy link
Contributor

alexreg commented Jan 24, 2019

I think we need to add the condition that the hidden lifetime outlives the lifetime mentioned in the opaque type?

@matthewjasper
Copy link
Contributor

No, we need the currently implemented condition: 'b: 'a in the above example.

@alexreg
Copy link
Contributor

alexreg commented Jan 24, 2019

@matthewjasper Huh? Isn't that exactly what I said?

@@ -2011,53 +2011,6 @@ a (non-transparent) struct containing a single float, while `Grams` is a
transparent wrapper around a float. This can make a difference for the ABI.
"##,

E0700: r##"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we outright remove error code descriptions we don't emit anymore (at least not if they have hit stable, I'm sure).

@nikomatsakis
Copy link
Contributor

This isn't sound:

Yep. So, my "basis" for believing that we can make something similar to (but presumably not quite the same as) this PR work is that we are able to "hide" lifetimes in a dyn Trait existential. But those types always have a bound and all the hidden lifetimes must outlive that bound. (Effectively, it is the intersection of all the hidden lifetimes). I guess we would want some similar-ish concept for impl Trait.

(In fact, if I recall, we do kind of compute such a region today -- that is, I think we forbid you from doing impl Trait + 'a + 'b unless either 'a: 'b or 'b: 'a, right?)

@Dylan-DPC-zz
Copy link

ping from triage @nikomatsakis what's the update on this?

@Dylan-DPC-zz
Copy link

ping from triage can anyone from @rust-lang/lang review this?

@Centril Centril 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 Mar 5, 2019
@Centril
Copy link
Contributor

Centril commented Mar 5, 2019

This is blocked on fixing the soundness hole per #57870 (comment).

@alexreg
Copy link
Contributor

alexreg commented Mar 5, 2019

@Centril Is "blocked" the right word? There's nothing that needs to be fixed outside of this PR.

@Centril
Copy link
Contributor

Centril commented Mar 5, 2019

@alexreg Idk... I debated whether to mark it as S-waiting-on-author or S-blocked... not sure it matters; but this way we won't retriage it all the time at least.

@Dylan-DPC-zz
Copy link

s-blocked or s-waiting-for-team should be fine at this point

@matthewjasper
Copy link
Contributor

Surely this should just be closed? The soundness issue doesn't seem fixable in a way that doesn't fundamentally change this pr.

@cramertj
Copy link
Member Author

cramertj commented Mar 5, 2019

@matthewjasper Yup, this is no longer just the simple revert offered by this PR given that we have to address those soundness issues. I'll close this particular PR out and we can open a new one when we figure out the correct approach.

@cramertj cramertj closed this Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants