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

repeat expression do not honor lt bounds of copy #95477

Closed
lcnr opened this issue Mar 30, 2022 · 10 comments · Fixed by #95819
Closed

repeat expression do not honor lt bounds of copy #95477

lcnr opened this issue Mar 30, 2022 · 10 comments · Fixed by #95819
Assignees
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Mar 30, 2022

#![feature(nll)]

#[derive(Clone)]
struct Foo<'a>(fn(&'a ()) -> &'a ());

impl Copy for Foo<'static> {}

fn mk_foo<'a>() -> Foo<'a> {
    println!("mk_foo");
    Foo(|x| x)
}

fn foo<'a>() -> [Foo<'a>; 100] {
    [mk_foo::<'a>(); 100]
}

fn main() {
    foo();
}

this compiles both with and without feature(nll). Note that mk_foo creates a value of type Foo<'a> which may not be copied.

Moving mk_foo::<'a>() into a local errors as expected:

#![feature(nll)]

#[derive(Clone)]
struct Foo<'a>(fn(&'a ()) -> &'a ());

impl Copy for Foo<'static> {}

fn mk_foo<'a>() -> Foo<'a> {
    println!("mk_foo");
    Foo(|x| x)
}

fn foo<'a>() -> [Foo<'a>; 100] {
    let x = mk_foo::<'a>();
    [x; 100]
}

this results in

error: lifetime may not live long enough
  --> src/main.rs:15:6
   |
13 | fn foo<'a>() -> [Foo<'a>; 100] {
   |        -- lifetime `'a` defined here
14 |     let x = mk_foo::<'a>();
15 |     [x; 100]
   |      ^ copying this value requires that `'a` must outlive `'static`
@lcnr lcnr added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness C-bug Category: This is a bug. labels Mar 30, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Mar 30, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Mar 30, 2022

considering that

fn pass<'a: 'a>(x: Foo<'a>) -> Foo<'a> {
    x
}

fn duplicate<'a>(x: Foo<'a>) -> [Foo<'a>; 100] {
    [pass::<'a>(x); 100]
}

also errors it seems like we somehow treat functions without any arguments in a special (and incorrect) way? @RalfJung I know that we've recently stabilized some additional exprs for repeat expressions, do you know more here?

@lcnr
Copy link
Contributor Author

lcnr commented Mar 30, 2022

Ah no, we just decide whether to copy or move things depending on whether they implement Copy while ignoring regions. That kinda sucks :/

here's a version which duplicates Foo<'a> without it being copy

fn inner<'a, F: FnOnce() -> Foo<'a>>(f: F) -> [Foo<'a>; 2] {
    [f(); 2]
}

struct NeverCopy<T>(T);
impl<T> NeverCopy<T> {
    fn inner(self) -> T {
        self.0
    }
}

fn duplicate<'a>(foo: NeverCopy<Foo<'a>>) -> [Foo<'a>; 2] {
    inner(move || foo.inner())
}

but that isn't really too relevant considering that optionally Copyable types are apparently completely unusable anyways 😅

@RalfJung
Copy link
Member

I think that is a general issue with Copy treatment also for other parts of MIR generation. But elsewhere we actually double-check and make sure to error if we later notice there was a mistake. Looks like for array repeat expressions we don't do that...

here's a version which duplicates Foo<'a> without it being copy

This looks exactly the same to me as the original example?

@RalfJung
Copy link
Member

Cc @oli-obk @nikomatsakis who might know more about MIR generation and Copy. I agree this is a soundness issue...

@RalfJung
Copy link
Member

Actually this might be a duplicate of #88901 ?

@oli-obk
Copy link
Contributor

oli-obk commented Mar 30, 2022

Yea it's probably a duplicate or variant of that issue.

This modulo-regions scheme is such a compiler footgun... I keep encountering it.

@lcnr
Copy link
Contributor Author

lcnr commented Mar 31, 2022

This looks exactly the same to me as the original example?

The original code requires the creation of a new Foo while that example copies an existing one. My concern was that we somehow treat the function as if it were const, which isn't the case.

@nikomatsakis
Copy link
Contributor

Yes, our general strategy for Copy is:

  • Check (without respect to lifetimes) whether the type implements Copy;
  • If so, generate a Copy;
  • Enforce in MIR that the lifetime bounds hold.

If I had my druthers, we would have required that Copy impls were always applicable, but we did not, and so we opted for the above compromise (I could dredge up the issue, but it hardly matters). We do generally consider it a soundness issue to fail to enforce those rules.

(That said, I've been working on a revised type system that I think might make the whole "modulo-regions" scheme far less necessary or important... but that's for another time.)

@RalfJung
Copy link
Member

Enforce in MIR that the lifetime bounds hold.

Yeah, that seems to not be working in some cases.

@oli-obk oli-obk self-assigned this Apr 8, 2022
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 14, 2022
@apiraino
Copy link
Contributor

Assigning priority as discussed in the Zulip thread of the Prioritization Working Group.

@rustbot label -I-prioritize +P-low

@rustbot rustbot added P-low Low priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Apr 14, 2022
@bors bors closed this as completed in a707f40 Apr 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-low Low priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants