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

fix fn/const items implied bounds and wf check #104098

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion compiler/rustc_borrowck/src/type_check/free_region_relations.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use rustc_data_structures::frozen::Frozen;
use rustc_data_structures::transitive_relation::{TransitiveRelation, TransitiveRelationBuilder};
use rustc_hir::def::DefKind;
use rustc_infer::infer::canonical::QueryRegionConstraints;
use rustc_infer::infer::outlives;
use rustc_infer::infer::outlives::env::RegionBoundPairs;
Expand Down Expand Up @@ -195,7 +196,9 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {

#[instrument(level = "debug", skip(self))]
pub(crate) fn create(mut self) -> CreateResult<'tcx> {
let span = self.infcx.tcx.def_span(self.universal_regions.defining_ty.def_id());
let tcx = self.infcx.tcx;
let defining_ty_def_id = self.universal_regions.defining_ty.def_id().expect_local();
let span = tcx.def_span(defining_ty_def_id);

// Insert the facts we know from the predicates. Why? Why not.
let param_env = self.param_env;
Expand Down Expand Up @@ -275,6 +278,26 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
normalized_inputs_and_output.push(norm_ty);
}

// Add implied bounds from impl header.
if matches!(tcx.def_kind(defining_ty_def_id), DefKind::AssocFn | DefKind::AssocConst) {
for &(ty, _) in tcx.assumed_wf_types(tcx.local_parent(defining_ty_def_id)) {
let Ok(TypeOpOutput { output: norm_ty, constraints: c, .. }) = self
.param_env
.and(type_op::normalize::Normalize::new(ty))
.fully_perform(self.infcx, span)
else {
tcx.sess.delay_span_bug(span, &format!("failed to normalize {ty:?}"));
continue;
};
constraints.extend(c);

// We currently add implied bounds from the normalized ty only.
// This is more conservative and matches wfcheck behavior.
Comment on lines +294 to +295
Copy link

Choose a reason for hiding this comment

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

Suggested change
// We currently add implied bounds from the normalized ty only.
// This is more conservative and matches wfcheck behavior.
// We currently add implied bounds from the normalized `ty` only.
// This is more conservative and matches `wfcheck` behavior.

Markup consistency edits.

let c = self.add_implied_bounds(norm_ty);
constraints.extend(c);
}
}

for c in constraints {
self.push_region_constraints(c, span);
}
Expand Down
10 changes: 10 additions & 0 deletions compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,16 @@ impl<'a, 'b, 'tcx> Visitor<'tcx> for TypeVerifier<'a, 'b, 'tcx> {
instantiated_predicates,
locations,
);

assert!(!matches!(
tcx.impl_of_method(def_id).map(|imp| tcx.def_kind(imp)),
Some(DefKind::Impl { of_trait: true })
));
self.cx.prove_predicates(
aliemjay marked this conversation as resolved.
Show resolved Hide resolved
args.types().map(|ty| ty::ClauseKind::WellFormed(ty.into())),
locations,
ConstraintCategory::Boring,
);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,16 @@ fn relate_mir_and_user_ty<'tcx>(
user_ty: Ty<'tcx>,
) -> Result<(), NoSolution> {
let cause = ObligationCause::dummy_with_span(span);
ocx.register_obligation(Obligation::new(
ocx.infcx.tcx,
cause.clone(),
param_env,
ty::ClauseKind::WellFormed(user_ty.into()),
));

let user_ty = ocx.normalize(&cause, param_env, user_ty);
ocx.eq(&cause, param_env, mir_ty, user_ty)?;

// FIXME(#104764): We should check well-formedness before normalization.
let predicate =
ty::Binder::dummy(ty::PredicateKind::Clause(ty::ClauseKind::WellFormed(user_ty.into())));
ocx.register_obligation(Obligation::new(ocx.infcx.tcx, cause, param_env, predicate));
Ok(())
}

Expand Down Expand Up @@ -113,31 +116,38 @@ fn relate_mir_and_user_args<'tcx>(
ocx.register_obligation(Obligation::new(tcx, cause, param_env, instantiated_predicate));
}

// Now prove the well-formedness of `def_id` with `substs`.
// Note for some items, proving the WF of `ty` is not sufficient because the
// well-formedness of an item may depend on the WF of gneneric args not present in the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// well-formedness of an item may depend on the WF of gneneric args not present in the
// well-formedness of an item may depend on the WF of generic args not present in the

// item's type. Currently this is true for associated consts, e.g.:
// ```rust
// impl<T> MyTy<T> {
// const CONST: () = { /* arbitrary code that depends on T being WF */ };
// }
// ```
for arg in args {
ocx.register_obligation(Obligation::new(
tcx,
cause.clone(),
param_env,
ty::ClauseKind::WellFormed(arg),
));
}

if let Some(UserSelfTy { impl_def_id, self_ty }) = user_self_ty {
ocx.register_obligation(Obligation::new(
tcx,
cause.clone(),
param_env,
ty::ClauseKind::WellFormed(self_ty.into()),
));

let self_ty = ocx.normalize(&cause, param_env, self_ty);
let impl_self_ty = tcx.type_of(impl_def_id).instantiate(tcx, args);
let impl_self_ty = ocx.normalize(&cause, param_env, impl_self_ty);

ocx.eq(&cause, param_env, self_ty, impl_self_ty)?;
let predicate = ty::Binder::dummy(ty::PredicateKind::Clause(ty::ClauseKind::WellFormed(
impl_self_ty.into(),
)));
ocx.register_obligation(Obligation::new(tcx, cause.clone(), param_env, predicate));
}

// In addition to proving the predicates, we have to
// prove that `ty` is well-formed -- this is because
// the WF of `ty` is predicated on the args being
// well-formed, and we haven't proven *that*. We don't
// want to prove the WF of types from `args` directly because they
// haven't been normalized.
//
// FIXME(nmatsakis): Well, perhaps we should normalize
// them? This would only be relevant if some input
// type were ill-formed but did not appear in `ty`,
// which...could happen with normalization...
let predicate =
ty::Binder::dummy(ty::PredicateKind::Clause(ty::ClauseKind::WellFormed(ty.into())));
ocx.register_obligation(Obligation::new(tcx, cause, param_env, predicate));
Ok(())
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,23 @@ impl<'tcx> super::QueryTypeOp<'tcx> for ProvePredicate<'tcx> {
}
}

if let ty::PredicateKind::Clause(ty::ClauseKind::WellFormed(arg)) =
key.value.predicate.kind().skip_binder()
{
match arg.as_type()?.kind() {
ty::Param(_)
| ty::Bool
| ty::Char
| ty::Int(_)
| ty::Float(_)
| ty::Str
| ty::Uint(_) => {
return Some(());
}
_ => {}
}
}

None
}

Expand Down
15 changes: 15 additions & 0 deletions tests/ui/borrowck/fn-item-check-trait-ref.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// The method `assert_static` should be callable only for static values,
// because the impl has an implied bound `where T: 'static`.

// check-fail

trait AnyStatic<Witness>: Sized {
fn assert_static(self) {}
}

impl<T> AnyStatic<&'static T> for T {}

fn main() {
(&String::new()).assert_static();
//~^ ERROR temporary value dropped while borrowed
}
12 changes: 12 additions & 0 deletions tests/ui/borrowck/fn-item-check-trait-ref.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error[E0716]: temporary value dropped while borrowed
--> $DIR/fn-item-check-trait-ref.rs:13:7
|
LL | (&String::new()).assert_static();
| --^^^^^^^^^^^^^------------------ temporary value is freed at the end of this statement
| | |
| | creates a temporary value which is freed while still in use
| argument requires that borrow lasts for `'static`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0716`.
57 changes: 57 additions & 0 deletions tests/ui/borrowck/fn-item-check-type-params.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Regression test for #104005.
//
// Previously, different borrowck implementations used to disagree here.
// The status of each is documented on `fn test_*`.

// check-fail

use std::fmt::Display;

trait Displayable {
fn display(self) -> Box<dyn Display>;
}

impl<T: Display> Displayable for (T, Option<&'static T>) {
fn display(self) -> Box<dyn Display> {
Box::new(self.0)
}
}

fn extend_lt<T, U>(val: T) -> Box<dyn Display>
where
(T, Option<U>): Displayable,
{
Displayable::display((val, None))
}

// AST: fail
// HIR: pass
// MIR: pass
pub fn test_call<'a>(val: &'a str) {
extend_lt(val);
//~^ ERROR borrowed data escapes outside of function
}

// AST: fail
// HIR: fail
// MIR: pass
pub fn test_coercion<'a>() {
let _: fn(&'a str) -> _ = extend_lt;
//~^ ERROR lifetime may not live long enough
}

// AST: fail
// HIR: fail
// MIR: fail
pub fn test_arg() {
fn want<I, O>(_: I, _: impl Fn(I) -> O) {}
want(&String::new(), extend_lt);
//~^ ERROR temporary value dropped while borrowed
}

// An exploit of the unsoundness.
fn main() {
let val = extend_lt(&String::from("blah blah blah"));
//~^ ERROR temporary value dropped while borrowed
println!("{}", val);
}
43 changes: 43 additions & 0 deletions tests/ui/borrowck/fn-item-check-type-params.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
error[E0521]: borrowed data escapes outside of function
--> $DIR/fn-item-check-type-params.rs:31:5
|
LL | pub fn test_call<'a>(val: &'a str) {
| -- --- `val` is a reference that is only valid in the function body
| |
| lifetime `'a` defined here
LL | extend_lt(val);
| ^^^^^^^^^^^^^^
| |
| `val` escapes the function body here
| argument requires that `'a` must outlive `'static`

error: lifetime may not live long enough
--> $DIR/fn-item-check-type-params.rs:39:12
|
LL | pub fn test_coercion<'a>() {
| -- lifetime `'a` defined here
LL | let _: fn(&'a str) -> _ = extend_lt;
| ^^^^^^^^^^^^^^^^ type annotation requires that `'a` must outlive `'static`

error[E0716]: temporary value dropped while borrowed
--> $DIR/fn-item-check-type-params.rs:48:11
|
LL | want(&String::new(), extend_lt);
| ------^^^^^^^^^^^^^------------- temporary value is freed at the end of this statement
| | |
| | creates a temporary value which is freed while still in use
| argument requires that borrow lasts for `'static`

error[E0716]: temporary value dropped while borrowed
--> $DIR/fn-item-check-type-params.rs:54:26
|
LL | let val = extend_lt(&String::from("blah blah blah"));
| -----------^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^-- temporary value is freed at the end of this statement
| | |
| | creates a temporary value which is freed while still in use
| argument requires that borrow lasts for `'static`

error: aborting due to 4 previous errors

Some errors have detailed explanations: E0521, E0716.
For more information about an error, try `rustc --explain E0521`.
1 change: 1 addition & 0 deletions tests/ui/higher-ranked/trait-bounds/issue-59311.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ where
v.t(|| {});
//~^ ERROR: higher-ranked lifetime error
//~| ERROR: higher-ranked lifetime error
//~| ERROR: higher-ranked lifetime error
}

fn main() {}
11 changes: 10 additions & 1 deletion tests/ui/higher-ranked/trait-bounds/issue-59311.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,15 @@ LL | v.t(|| {});
|
= note: could not prove `{closure@$DIR/issue-59311.rs:17:9: 17:11} well-formed`

error: higher-ranked lifetime error
--> $DIR/issue-59311.rs:17:5
|
LL | v.t(|| {});
| ^^^^^^^^^^
|
= note: could not prove `{closure@$DIR/issue-59311.rs:17:9: 17:11} well-formed`
= note: duplicate diagnostic emitted due to `-Z deduplicate-diagnostics=no`

error: higher-ranked lifetime error
--> $DIR/issue-59311.rs:17:9
|
Expand All @@ -14,5 +23,5 @@ LL | v.t(|| {});
|
= note: could not prove `for<'a> &'a V: 'static`

error: aborting due to 2 previous errors
error: aborting due to 3 previous errors

9 changes: 4 additions & 5 deletions tests/ui/implied-bounds/implied-bounds-on-trait-hierarchy.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,7 @@
// check-pass
// known-bug: #84591
Copy link
Contributor

Choose a reason for hiding this comment

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

#84591 is still unsound, even with this PR, is it? do we have a test which still incorrectly compiles. It feels like we're not fixing this issue but only making it harder to trigger

// issue: #84591

// Should fail. Subtrait can incorrectly extend supertrait lifetimes even when
// supertrait has weaker implied bounds than subtrait. Strongly related to
// issue #25860.
// Subtrait was able to incorrectly extend supertrait lifetimes even when
// supertrait had weaker implied bounds than subtrait.

trait Subtrait<T>: Supertrait {}
trait Supertrait {
Expand Down Expand Up @@ -34,6 +32,7 @@ fn main() {
{
let x = "Hello World".to_string();
subs_to_soup((x.as_str(), &mut d));
//~^ does not live long enough
}
println!("{}", d);
}
16 changes: 16 additions & 0 deletions tests/ui/implied-bounds/implied-bounds-on-trait-hierarchy.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error[E0597]: `x` does not live long enough
--> $DIR/implied-bounds-on-trait-hierarchy.rs:34:23
|
LL | let x = "Hello World".to_string();
| - binding `x` declared here
LL | subs_to_soup((x.as_str(), &mut d));
| ^ borrowed value does not live long enough
LL |
LL | }
| - `x` dropped here while still borrowed
LL | println!("{}", d);
| - borrow later used here

error: aborting due to previous error

For more information about this error, try `rustc --explain E0597`.
2 changes: 2 additions & 0 deletions tests/ui/lifetimes/lifetime-errors/issue_74400.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ fn f<T, S>(data: &[T], key: impl Fn(&T) -> S) {
fn g<T>(data: &[T]) {
f(data, identity)
//~^ ERROR the parameter type
//~| ERROR the parameter type
//~| ERROR the parameter type
//~| ERROR mismatched types
//~| ERROR implementation of `FnOnce` is not general
}
Loading
Loading