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

implied bounds from associated types may not actually get implied pt 2. #98543

Closed
lcnr opened this issue Jun 26, 2022 · 5 comments
Closed

implied bounds from associated types may not actually get implied pt 2. #98543

lcnr opened this issue Jun 26, 2022 · 5 comments
Labels
A-associated-items Area: Associated items such as associated types and consts. A-implied-bounds Area: Related to implied bounds 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-high High priority T-types Relevant to the types team, which will review and decide on the PR/issue.

Comments

@lcnr
Copy link
Contributor

lcnr commented Jun 26, 2022

revives #91068 which has been fixed by only considering implied bounds from projections if they don't normalize while typechecking the function itself

// We only add implied bounds for the normalized type as the unnormalized
// type may not actually get checked by the caller.
//
// Can otherwise be unsound, see #91068.
let TypeOpOutput { output: norm_ty, constraints: constraints1, .. } = self
.param_env
.and(type_op::normalize::Normalize::new(ty))
.fully_perform(self.infcx)
.unwrap_or_else(|_| {
self.infcx
.tcx
.sess
.delay_span_bug(DUMMY_SP, &format!("failed to normalize {:?}", ty));
TypeOpOutput {
output: self.infcx.tcx.ty_error(),
constraints: None,
error_info: None,
}
});
// Note: we need this in examples like
// ```
// trait Foo {
// type Bar;
// fn foo(&self) -> &Self::Bar;
// }
// impl Foo for () {
// type Bar = ();
// fn foo(&self) ->&() {}
// }
// ```
// Both &Self::Bar and &() are WF
let constraints_implied = self.add_implied_bounds(norm_ty);
normalized_inputs_and_output.push(norm_ty);
constraints1.into_iter().chain(constraints_implied)

This does not mean that the projection won't normalize when getting called:

trait Trait {
    type Type;
}

impl<T> Trait for T {
    type Type = ();
}

fn f<'a, 'b>(s: &'b str, _: <&'a &'b () as Trait>::Type) -> &'a str
where
    &'a &'b (): Trait, // <- adding this bound is the change from #91068 
{
    s
}

fn main() {
    let x = String::from("Hello World!");
    let y = f(&x, ());
    drop(x);
    println!("{}", y);
}

The added bound prevents <&'a &'b () as Trait>::Type from getting normalized while borrowchecking f, as we prefer param candidates over impl candidates. When calling f, we don't have the &'a &'b (): Trait in our param_env, so we can now freely use the impl candidate to normalize the projection. The caller therefore doesn't have to prove that &'a &'b () is well formed, causing unsoundness.

I am a bit surprised that the caller doesn't have to prove that the &'a &'b (): Trait obligation is well formed, which would cause this example to not be unsound. It doesn't remove the general unsoundness here though. Here's an alternative test where that isn't enough:

trait Trait {
    type Type;
}

impl<T> Trait for T {
    type Type = ();
}

fn f<'a, 'b>(s: &'b str, _: <&'a &'b () as Trait>::Type) -> &'a str
where
    for<'what, 'ever> &'what &'ever (): Trait,
{
    s
}

fn main() {
    let x = String::from("Hello World!");
    let y = f(&x, ());
    drop(x);
    println!("{}", y);
}
@lcnr lcnr added C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness I-types-nominated Nominated for discussion during a types team meeting. T-types Relevant to the types team, which will review and decide on the PR/issue. labels Jun 26, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 26, 2022
@lcnr lcnr added A-associated-items Area: Associated items such as associated types and consts. A-implied-bounds Area: Related to implied bounds labels Jun 26, 2022
@steffahn
Copy link
Member

Unlike #91068, these code examples compile “successfully” all the way since Rust 1.7.0.

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 27, 2022
@lcnr
Copy link
Contributor Author

lcnr commented Jun 27, 2022

while this currently does not work, we should also add a test for something like this. If we get stronger implied bounds, simply requiring the caller to prove wf for all projection components won't be enough anymore

#![feature(generic_associated_types)]

trait Trait<'a> {
    type Type
    where
        Self: 'a;
}

impl<'a, T> Trait<'a> for T {
    type Type = ()
    where
        Self: 'a; // once this bound gets implied, we have UB again
}

fn f<'a, 'b>(s: &'b str, _: <&'b () as Trait<'a>>::Type) -> &'a str
where
    &'b (): Trait<'a>, // <- adding this bound is the change from #91068 
{
    s
}

fn main() {
    let x = String::from("Hello World!");
    let y = f(&x, ());
    drop(x);
    println!("{}", y);
}
trait Trait<'a>: 'a {
    type Type;
}

impl<'a, T> Trait<'a> for T
where
    T: 'a // if this bound get's implied we would probably get ub here again
{
    type Type = ();
}

fn f<'a, 'b>(s: &'b str, _: <&'b () as Trait<'a>>::Type) -> &'a str
where
    &'b (): Trait<'a>, // <- adding this bound is the change from #91068 
{
    s
}

fn main() {
    let x = String::from("Hello World!");
    let y = f(&x, ());
    drop(x);
    println!("{}", y);
}

@jackh726
Copy link
Member

There's a PR open that fixes this.

@jackh726 jackh726 removed the I-types-nominated Nominated for discussion during a types team meeting. label Jul 29, 2022
bors added a commit to rust-lang-ci/rust that referenced this issue Aug 9, 2022
consider unnormalized types for implied bounds

extracted, and slightly modified, from rust-lang#98900

The idea here is that generally, rustc is split into things which can assume its inputs are well formed[^1], and things which have verify that themselves.

Generally most predicates should only deal with well formed inputs, e.g. a `&'a &'b (): Trait` predicate should be able to assume that `'b: 'a` holds. Normalization can loosen wf requirements (see rust-lang#91068) and must therefore not be used in places which still have to check well formedness. The only such place should hopefully be `WellFormed` predicates

fixes rust-lang#87748 and rust-lang#98543

r? `@jackh726` cc `@rust-lang/types`

[^1]: These places may still encounter non-wf inputs and have to deal with them without causing an ICE as we may check for well formedness out of order.
@lcnr
Copy link
Contributor Author

lcnr commented Aug 19, 2022

fixed by #99217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-associated-items Area: Associated items such as associated types and consts. A-implied-bounds Area: Related to implied bounds 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-high High priority T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants