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

Generic param requires bounds on call to function that is already required for calls to the caller #102611

Closed
sharnoff opened this issue Oct 3, 2022 · 3 comments · Fixed by #120019
Labels
C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@sharnoff
Copy link
Contributor

sharnoff commented Oct 3, 2022

Seems like this behavior has existed for a while, so apologies in advance if this is already a settled matter. If so, I still think the failure coulld use a better error message.

In short, the following code fails to compile:

pub struct Foo<'a, A>(&'a A);

impl<'a, A> Foo<'a, A> {
    pub fn foo() {}
    
    pub fn call_foo() {
        Self::foo();
    }
}

(playground link)

Error message:

error[E0309]: the parameter type `A` may not live long enough
 --> src/lib.rs:7:9
  |
7 |         Self::foo();
  |         ^^^^^^^^^ ...so that the type `A` will meet its required lifetime bounds
  |
help: consider adding an explicit lifetime bound...
  |
3 | impl<'a, A: 'a> Foo<'a, A> {
  |           ++++

In call_foo, the compiler (correctly) requires that A: 'a, but fails to realize that this bound is already satisfied (because this is also required for callers of call_foo).

To me, this seems like a bug. However, this behavior seems to have been present in stable for long enough that it seems highly unlikely it hasn't already been discussed at length - I couldn't find any issues referencing it though. (closest seems to be #84021).

Happy to help where I can (would need some direction).

@lcnr
Copy link
Contributor

lcnr commented Oct 3, 2022

pub struct Foo<'a, A>(&'a A);

impl<'a, A> Foo<'a, A> {
    pub fn foo() {
        Self::foo();
    }
}

yeah, implied bounds from the impl header are ignored while typechecking a function body but apparently still required somewhere 😁 i am currently working on this and should hopefully be able to fix this in the somewhat near future

@rustbot claim

@aliemjay
Copy link
Member

aliemjay commented Oct 3, 2022

but apparently still required somewhere grin

They're required for the WF-check of Self in borrowck. This is a duplicate of #98852.

@Rageking8
Copy link
Contributor

@rustbot label +T-compiler +C-bug

@rustbot rustbot added C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 4, 2022
@lcnr lcnr removed their assignment Jun 8, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 8, 2023
fix fn item implied bounds and wf check

These are two distinct changes:
1. Wf-check all fn item substs.
Fixes rust-lang#104005

2. Use implied bounds from impl header.
Fixes rust-lang#98852
Fixes rust-lang#102611

The first is a breaking change and will likely have big impact without the the second one. See the first commit for how it breaks libstd.

Landing the second one without the first will allow more incorrect code to pass. For example an exploit of rust-lang#104005 would be as simple as:
```rust
use core::fmt::Display;

trait ExtendLt<Witness> {
    fn extend(self) -> Box<dyn Display>;
}

impl<T: Display> ExtendLt<&'static T> for T {
    fn extend(self) -> Box<dyn Display> {
        Box::new(self)
    }
}

fn main() {
    let val = (&String::new()).extend();
    println!("{val}");
}
```

cc `@lcnr`
r? types
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 9, 2023
fix fn item implied bounds and wf check

These are two distinct changes:
1. Wf-check all fn item substs.
Fixes rust-lang#104005

2. Use implied bounds from impl header.
Fixes rust-lang#98852
Fixes rust-lang#102611

The first is a breaking change and will likely have big impact without the the second one. See the first commit for how it breaks libstd.

Landing the second one without the first will allow more incorrect code to pass. For example an exploit of rust-lang#104005 would be as simple as:
```rust
use core::fmt::Display;

trait ExtendLt<Witness> {
    fn extend(self) -> Box<dyn Display>;
}

impl<T: Display> ExtendLt<&'static T> for T {
    fn extend(self) -> Box<dyn Display> {
        Box::new(self)
    }
}

fn main() {
    let val = (&String::new()).extend();
    println!("{val}");
}
```

cc `@lcnr`
r? types
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 20, 2023
fix fn/const items implied bounds and wf check

These are two distinct changes (edit: actually three, see below):
1. Wf-check all fn item args. This is a soundness fix.
Fixes rust-lang#104005

2. Use implied bounds from impl header in borrowck of associated functions/consts. This strictly accepts more code and helps to mitigate the impact of other breaking changes.
Fixes rust-lang#98852
Fixes rust-lang#102611

The first is a breaking change and will likely have a big impact without the the second one. See the first commit for how it breaks libstd.

Landing the second one without the first will allow more incorrect code to pass. For example an exploit of rust-lang#104005 would be as simple as:
```rust
use core::fmt::Display;

trait ExtendLt<Witness> {
    fn extend(self) -> Box<dyn Display>;
}

impl<T: Display> ExtendLt<&'static T> for T {
    fn extend(self) -> Box<dyn Display> {
        Box::new(self)
    }
}

fn main() {
    let val = (&String::new()).extend();
    println!("{val}");
}
```

The third change is to to check WF of user type annotations before normalizing them (fixes rust-lang#104764, fixes rust-lang#104763). It is mutually dependent on the second change above: an attempt to land it separately in rust-lang#104746 caused several crater regressions that can all be mitigated by using the implied from the impl header. It is also necessary for the soundness of associated consts that use the implied bounds of impl header. See rust-lang#104763 and how the third commit fixes the soundness issue in `tests/ui/wf/wf-associated-const.rs` that was introduces by the previous commit.

cc `@lcnr`
r? types
@bors bors closed this as completed in 6bf600b Jan 17, 2024
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
5 participants