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

sjwang05 · 2024-01-19T09:37:59Z

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 · 2024-01-19T09:38:08Z

r? @TaKO8Ki

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

BoxyUwU · 2024-01-19T18:03:26Z

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

BoxyUwU · 2024-01-19T18:05:50Z

Could you also do this:

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

sjwang05 · 2024-01-22T00:02:37Z

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 · 2024-01-23T17:36:15Z

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 · 2024-01-23T17:36:41Z

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

sjwang05 · 2024-01-28T05:03:46Z

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 · 2024-01-29T08:03:54Z