adt_const_params: check referred-to types when checking impls of ConstParamTy on refs

[sjwang05]

Previously, we didn't check if referenced-to types of impls of ConstParamTy on refs actually implemented ConstParamTy in type_allowed_to_implement_const_param_ty, leading to code like the following compiling without error (#112124):

#![feature(adt_const_params)]
#![allow(incomplete_features)]

use std::marker::ConstParamTy;

#[derive(PartialEq, Eq)]
struct Foo;

impl ConstParamTy for &Foo {}

This would lead to ICEs when the inner referenced type had fields that were not ConstParamTy and were not const-evaluatable (#119299).

This PR modifies type_allowed_to_implement_const_param_ty to check if the referred-to type and all of its fields implement ConstParamTy. It additionally adds a new variant to the ConstParamTyImplementationError enum and a new error in the case the referenced-to type does not implement ConstParamTy.

cc @rust-lang/project-const-generics

Fixes #112124

[rustbot]

r? @TaKO8Ki

(rustbot has picked a reviewer for you, use r? to override)

[BoxyUwU]

I swear I remember writing a review that we needed to check this on the original PR 😦

[BoxyUwU]

Could you also do this:

Checks should also be added for tuple,array and slice element tys just to be safe imo

[sjwang05]

Done. I split the logic into its own closure that gets called in the relevant branches, but otherwise the logic is the same as before. I also tweaked the error message slightly.

@rustbot review

[BoxyUwU]

Could you add the following two test cases:

// check-pass

struct Bar;
struct Foo(Bar);

impl ConstParamTy for Bar
where
  Foo: ConstParamTy {}

// impl checks means that this impl is only valid if `Bar: ConstParamTy` whic
// is only valid if `Foo: ConstParamTy` holds
impl ConstParamTy for Foo {}

// check that we actually take into account region constraints for `ConstParamTy` impl checks

#![feature(adt_const_params)]

use std::marker::ConstParamTy;

struct Foo<'a>(&'a u32);

struct Bar<T>(T);

impl ConstParamTy for Foo<'static> {}
impl<'a> ConstParamTy for Bar<Foo<'a>> {}
// ~^ ERROR: the trait `ConstParamTy` cannot be implemented for this type

---

Using an ObligationCtxt the way you are is slightly wrong and more complicated than what you're doing right now (see all_fields_implement_trait inside the nested loop for an example of what the logic roughly looks like for what you're trying to do).

Could you move the inside of the nested loop in all_fields_implement_trait to a new function fn field_implements_trait that returns the errors. Name feels a bit funny because we'll want to use it for things that arent strictly fields (i.e. the pointee type of references) but I think it's close enough to fit.

You can then use that function in type_allowed_to_implement_const_param_ty instead of setting up the ocx and infcx manually. You wouldn't need the closure once that's done as you can collect a list of tys when matching on the self ty then handle them all at once next to the all_fields_implement_trait call by iterating over them and calling field_implements_trait.

[BoxyUwU]

Thanks for the PR 😃 that issue has been open so long lol 😳

[sjwang05]

Thanks for the help! I added the tests and fixed the ObligationCtxt-related code as suggested, though I feel like the implementation might be a bit messy due to us having to handle both FieldDefs and types--if you have any suggestions on how it could be improved, I'd be glad to make them. I also tweaked the error messages (again) to give slightly better errors on infringing inner tys in tuples.

@rustbot review

[BoxyUwU]

There shouldn't be any need to call type_allowed_to_implement_const_param_ty (or do equivalent logic) recursively, so most of the code in the match arms should be able to be removed. I think the following changes ought to work. For impl ConstParamTy for MyType we just check all the fields implement ConstParamTy rather than checking all the fields could implement ConstParamTy so I think doing the same thing for [T]/&T/(T,) where we require T: ConstParamTy rather than T potentially being able to implement it, is okay.

[BoxyUwU]

Suggested change

	let ty = ty.peel_refs();
	if !ty.has_concrete_skeleton() {
	continue;
	}
	if let &ty::Adt(adt, args) = ty.kind() {
	adts_and_args.push((adt, args));
	inner_tys.push((ty, adt.did()));
	} else {
	type_allowed_to_implement_const_param_ty(tcx, param_env, ty, parent_cause)?;
	}
	}
	inner_tys.push(ty);

[BoxyUwU]

Suggested change

	let ty = ty.peel_refs();
	if !ty.has_concrete_skeleton() {
	return Ok(());
	}
	if let &ty::Adt(adt, args) = ty.kind() {
	adts_and_args.push((adt, args));
	inner_tys.push((ty, adt.did()));
	} else {
	type_allowed_to_implement_const_param_ty(tcx, param_env, ty, parent_cause)?;
	}
	inner_tys.push(ty);

[BoxyUwU]

Suggested change

	&ty::Adt(adt, args) => adts_and_args.push((adt, args)),
	&ty::Adt(adt, args) => {
	all_fields_implement_trait(
	tcx,
	param_env,
	self_type,
	adt,
	args,
	parent_cause,
	hir::LangItem::ConstParamTy,
	)
	.map_err(ConstParamTyImplementationError::InfrigingFields)?;
	}

[BoxyUwU]

Suggested change

	for (adt, args) in adts_and_args {
	all_fields_implement_trait(
	tcx,
	param_env,
	self_type,
	adt,
	args,
	parent_cause,
	hir::LangItem::ConstParamTy,
	)
	.map_err(ConstParamTyImplementationError::InfrigingFields)?;
	}

[BoxyUwU]

Suggested change

	for (ty, def_id) in inner_tys {
	let span = tcx.def_span(def_id);
	for ty in inner_tys {
	let span = match ty.kind() {
	TyKind::Adt(def, _) => tcx.def_span(def_id),
	_ => parent_cause.span,
	};

[bors]

☔ The latest upstream changes (presumably #121018) made this pull request unmergeable. Please resolve the merge conflicts.

[JohnCSimon]

@sjwang05
ping from triage - can you post your status on this PR? This PR has not received an update in a few months.

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

Or if you're not going to continue, please close it. Thank you!

[BoxyUwU]

I'm just going to close this, this PR has been afk for months and also I think we are going to move in the direction of forbidding references