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

Treat types in unnormalized function signatures as well-formed #88312

Merged
merged 1 commit into from
Aug 29, 2021
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,9 @@ impl UniversalRegionRelationsBuilder<'cx, 'tcx> {
let constraint_sets: Vec<_> = unnormalized_input_output_tys
.flat_map(|ty| {
debug!("build: input_or_output={:?}", ty);
// We add implied bounds from both the unnormalized and normalized ty
// See issue #87748
let constraints_implied_1 = self.add_implied_bounds(ty);
let TypeOpOutput { output: ty, constraints: constraints1, .. } = self
.param_env
.and(type_op::normalize::Normalize::new(ty))
Expand All @@ -271,9 +274,21 @@ impl UniversalRegionRelationsBuilder<'cx, 'tcx> {
canonicalized_query: None,
}
});
let constraints2 = self.add_implied_bounds(ty);
// 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_2 = self.add_implied_bounds(ty);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we need this set

Copy link
Contributor

Choose a reason for hiding this comment

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

In principle, it should be a subset of the other, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not exactly sure. I'm thinking of some variation of

trait Foo {
  type Bar<'a> where Self: 'a;
  fn foo(_: Self::Bar<'a>);
}
struct S<T> { ... }
impl<T> Foo for S<T> {
  type Bar<'a> where Self: 'a = &'a T;
  fn foo(_: Self::Bar<'a>) {}
}

and trying to imagining if we could somehow run into a situation where T: 'a. But I suppose we would get that from Self: 'a

I can try to remove this (and other places where we add normalized inputs to WF tys list) and see if anything breaks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so we can't remove this, because of cases like

trait Foo {
  type Bar;
  fn foo(&self) -> &Self::Bar;
}
impl Foo for () {
  type Bar = ();
  fn foo(&self) ->&() {}
}

unsafe fn as_inner(&mut self) -> &mut S {
fails, for example.

There might be some combination of reverting the changes here that allow things to work fine, but for consistency, I think it's better to treat these all the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. That example makes sense. I really want to do that "how do WF rules in a formal sense" write up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I would say -- the example makes sense, but really it points to a shortcoming elsewhere in our system. That is, in some sense, we ought to be able to in the assumption that Self::Source: 'a and then normalize that assumption to get that S: 'a. I expect this is largely equivalent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment with this example?

normalized_inputs_and_output.push(ty);
constraints1.into_iter().chain(constraints2)
constraints1.into_iter().chain(constraints_implied_1).chain(constraints_implied_2)
})
.collect();

Expand Down
12 changes: 11 additions & 1 deletion compiler/rustc_typeck/src/check/compare_method.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,8 @@ fn compare_predicate_entailment<'tcx>(
// Compute placeholder form of impl and trait method tys.
let tcx = infcx.tcx;

let mut wf_tys = vec![];

let (impl_sig, _) = infcx.replace_bound_vars_with_fresh_vars(
impl_m_span,
infer::HigherRankedType,
Expand All @@ -260,10 +262,18 @@ fn compare_predicate_entailment<'tcx>(
let impl_fty = tcx.mk_fn_ptr(ty::Binder::dummy(impl_sig));
debug!("compare_impl_method: impl_fty={:?}", impl_fty);

// First liberate late bound regions and subst placeholders
let trait_sig = tcx.liberate_late_bound_regions(impl_m.def_id, tcx.fn_sig(trait_m.def_id));
let trait_sig = trait_sig.subst(tcx, trait_to_placeholder_substs);
// Next, add all inputs and output as well-formed tys. Importantly,
// we have to do this before normalization, since the normalized ty may
// not contain the input parameters. See issue #87748.
wf_tys.extend(trait_sig.inputs_and_output.iter());
let trait_sig =
inh.normalize_associated_types_in(impl_m_span, impl_m_hir_id, param_env, trait_sig);
// Also add the resulting inputs and output as well-formed.
// This probably isn't strictly necessary.
wf_tys.extend(trait_sig.inputs_and_output.iter());
let trait_fty = tcx.mk_fn_ptr(ty::Binder::dummy(trait_sig));

debug!("compare_impl_method: trait_fty={:?}", trait_fty);
Expand Down Expand Up @@ -388,7 +398,7 @@ fn compare_predicate_entailment<'tcx>(
// Finally, resolve all regions. This catches wily misuses of
// lifetime parameters.
let fcx = FnCtxt::new(&inh, param_env, impl_m_hir_id);
fcx.regionck_item(impl_m_hir_id, impl_m_span, trait_sig.inputs_and_output);
fcx.regionck_item(impl_m_hir_id, impl_m_span, &wf_tys);

Ok(())
})
Expand Down
16 changes: 12 additions & 4 deletions compiler/rustc_typeck/src/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ fn typeck_with_fallback<'tcx>(

let typeck_results = Inherited::build(tcx, def_id).enter(|inh| {
let param_env = tcx.param_env(def_id);
let fcx = if let Some(hir::FnSig { header, decl, .. }) = fn_sig {
let (fcx, wf_tys) = if let Some(hir::FnSig { header, decl, .. }) = fn_sig {
let fn_sig = if crate::collect::get_infer_ret_ty(&decl.output).is_some() {
let fcx = FnCtxt::new(&inh, param_env, body.value.hir_id);
<dyn AstConv<'_>>::ty_of_fn(
Expand All @@ -383,17 +383,25 @@ fn typeck_with_fallback<'tcx>(

check_abi(tcx, id, span, fn_sig.abi());

// When normalizing the function signature, we assume all types are
// well-formed. So, we don't need to worry about the obligations
// from normalization. We could just discard these, but to align with
// compare_method and elsewhere, we just add implied bounds for
// these types.
let mut wf_tys = vec![];
// Compute the fty from point of view of inside the fn.
let fn_sig = tcx.liberate_late_bound_regions(def_id.to_def_id(), fn_sig);
wf_tys.extend(fn_sig.inputs_and_output.iter());
let fn_sig = inh.normalize_associated_types_in(
body.value.span,
body_id.hir_id,
param_env,
fn_sig,
);
wf_tys.extend(fn_sig.inputs_and_output.iter());

let fcx = check_fn(&inh, param_env, fn_sig, decl, id, body, None, true).0;
fcx
(fcx, wf_tys)
} else {
let fcx = FnCtxt::new(&inh, param_env, body.value.hir_id);
let expected_type = body_ty
Expand Down Expand Up @@ -443,7 +451,7 @@ fn typeck_with_fallback<'tcx>(

fcx.write_ty(id, expected_type);

fcx
(fcx, vec![])
};

let fallback_has_occurred = fcx.type_inference_fallback();
Expand All @@ -467,7 +475,7 @@ fn typeck_with_fallback<'tcx>(
fcx.select_all_obligations_or_error();

if fn_sig.is_some() {
fcx.regionck_fn(id, body);
fcx.regionck_fn(id, body, span, &wf_tys);
} else {
fcx.regionck_expr(body);
}
Expand Down
11 changes: 10 additions & 1 deletion compiler/rustc_typeck/src/check/regionck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,20 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// rest of type check and because sometimes we need type
/// inference to have completed before we can determine which
/// constraints to add.
pub fn regionck_fn(&self, fn_id: hir::HirId, body: &'tcx hir::Body<'tcx>) {
pub(crate) fn regionck_fn(
&self,
fn_id: hir::HirId,
body: &'tcx hir::Body<'tcx>,
span: Span,
wf_tys: &[Ty<'tcx>],
) {
debug!("regionck_fn(id={})", fn_id);
let subject = self.tcx.hir().body_owner_def_id(body.id());
let hir_id = body.value.hir_id;
let mut rcx = RegionCtxt::new(self, hir_id, Subject(subject), self.param_env);
// We need to add the implied bounds from the function signature
rcx.outlives_environment.add_implied_bounds(self, wf_tys, fn_id, span);
rcx.outlives_environment.save_implied_bounds(fn_id);

if !self.errors_reported_since_creation() {
// regionck assumes typeck succeeded
Expand Down
11 changes: 10 additions & 1 deletion compiler/rustc_typeck/src/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -908,6 +908,7 @@ fn check_where_clauses<'tcx, 'fcx>(
}
}

#[tracing::instrument(level = "debug", skip(fcx, span, hir_decl))]
fn check_fn_or_method<'fcx, 'tcx>(
fcx: &FnCtxt<'fcx, 'tcx>,
span: Span,
Expand All @@ -918,6 +919,11 @@ fn check_fn_or_method<'fcx, 'tcx>(
) {
let sig = fcx.tcx.liberate_late_bound_regions(def_id, sig);

// Unnormalized types in signature are WF too
implied_bounds.extend(sig.inputs());
// FIXME(#27579) return types should not be implied bounds
implied_bounds.push(sig.output());

// Normalize the input and output types one at a time, using a different
// `WellFormedLoc` for each. We cannot call `normalize_associated_types`
// on the entire `FnSig`, since this would use the same `WellFormedLoc`
Expand Down Expand Up @@ -967,9 +973,11 @@ fn check_fn_or_method<'fcx, 'tcx>(
ObligationCauseCode::ReturnType,
);

// FIXME(#25759) return types should not be implied bounds
// FIXME(#27579) return types should not be implied bounds
implied_bounds.push(sig.output());

debug!(?implied_bounds);

check_where_clauses(fcx, span, def_id, Some((sig.output(), hir_decl.output.span())));
}

Expand Down Expand Up @@ -1116,6 +1124,7 @@ const HELP_FOR_SELF_TYPE: &str = "consider changing to `self`, `&self`, `&mut se
`self: Rc<Self>`, `self: Arc<Self>`, or `self: Pin<P>` (where P is one \
of the previous types except `Self`)";

#[tracing::instrument(level = "debug", skip(fcx))]
fn check_method_receiver<'fcx, 'tcx>(
fcx: &FnCtxt<'fcx, 'tcx>,
fn_sig: &hir::FnSig<'_>,
Expand Down
30 changes: 30 additions & 0 deletions src/test/ui/generic-associated-types/issue-87748.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Checks that we properly add implied bounds from unnormalized projections in
// inputs when typechecking functions.

// check-pass

#![feature(generic_associated_types)]

trait MyTrait {
type Assoc<'a, 'b> where 'b: 'a;
fn do_sth(arg: Self::Assoc<'_, '_>);
}

struct A;
struct B;
struct C;

impl MyTrait for A {
type Assoc<'a, 'b> where 'b: 'a = u32;
fn do_sth(_: u32) {}
}
impl MyTrait for B {
type Assoc<'a, 'b> where 'b: 'a = u32;
fn do_sth(_: Self::Assoc<'_, '_>) {}
}
impl MyTrait for C {
type Assoc<'a, 'b> where 'b: 'a = u32;
fn do_sth(_: Self::Assoc<'static, 'static>) {}
}

fn main () {}