-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Unsoundness in type checking of trait impls. Differences in implied lifetime bounds are not considered. #80176
Comments
This might be a duplicate of #57893. |
@jyn514 |
This comment has been minimized.
This comment has been minimized.
To me this looks like a variation of #25860 , except it exploits it by implementing a trait instead of taking a function pointer. |
@SkiFire13 Oh, I see. It seems related. Taking some inspiration there for a simplified version: type Ty = Box<&'static u8>;
trait Bad {
fn f<'a>(_: &'static &'static (), y: &'a Ty) -> &'static Ty;
}
impl Bad for () {
// NOTE that this signature does _not_ match the trait definition
// (the first argument has different lifetimes)
fn f<'a>(_: &'static &'a (), y: &'a Ty) -> &'static Ty {
y
}
}
fn main() {
let v = Box::new(&0);
let r = &v;
let s = <()>::f(&&(), r);
drop(v);
let _x = Box::new(0usize);
println!("{}", s);
} And here’s a variant that compiles since rust
|
Assigning |
downgrading priority to P-high (because this is an old bug, not a regression and I would not consider it a release blocker), but also nominating for discussion because I want to talk about it at a future compiler team meeting with @nikomatsakis present. |
Discussed at today's T-compiler meeting: https://zulip-archive.rust-lang.org/238009tcompilermeetings/41447weeklymeeting2021010754818.html#221955336 Downgrading to P-medium to make consistent with #25860. (We think the right path for fixing this is something that would also resolve #25860.) Removing nomination label. |
@rustbot modify labels: +A-typesystem |
Coming back to this after the recent article on #25860
I'm not convinced anymore that's the case. If you try this (wrong) code: trait Foo {
fn f<'a>(_: &'a ());
}
impl Foo for () {
fn f(_: &'static ()) {}
} You'll get this error:
(Also, it's interesting the Note the reference to function pointers. The HRTB part is missing, but I suspect this is just the way it is printed. Edit: the code responsible for checking that methods in trait impls match the ones in the trait definition is this: rust/compiler/rustc_typeck/src/check/compare_method.rs Lines 32 to 74 in e069a71
This ends up calling compare_predicate_entailment , which is defined here:rust/compiler/rustc_typeck/src/check/compare_method.rs Lines 76 to 406 in e069a71
The interesting part is here though: rust/compiler/rustc_typeck/src/check/compare_method.rs Lines 257 to 278 in e069a71
This converts the methods defined in the trait implementation and the definition to higher order function pointers, and then checks that the one from the implementation is a subtype of the one from the definition, which brings us back to #25860 |
Is this fixed by implied_bounds_entailment? All the reproducers here seem to be, and my (admittedly rudimentary) understanding is that it would be expected to be? |
yes, it should be, cc #105572 |
this has been fixed in #117984, marking as |
already have tests added, cc #117984 |
Applies to current stable and nightly. See comment further down for a simplified example and one that works all the way since Rust
1.19
.(Playground)
Errors:
@rustbot modify labels: T-compiler, C-bug, A-lifetimes, A-traits
@rustbot prioritize
The text was updated successfully, but these errors were encountered: