Refactor AliasTy. AliasTerm & UnevaluatedConst to use Alias#156538
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a630681 to
89cd50d
Compare
|
can you say more about the errors you had to work around? this feels odd to me and something we should fix in the derives instead of requiring manual impls here imo |
| )] | ||
| pub struct AliasTy<I: Interner> { | ||
| /// The parameters of the associated or opaque type. | ||
| pub struct Alias<I: Interner, K> { |
There was a problem hiding this comment.
can you move Alias and AliasTerm in a separate rustc_type_ir/ty/alias module?
There was a problem hiding this comment.
667b095 👍, Only moved Alias & AliasTerm, not AliasTy
|
Edit: I've been experimenting and think I've expanded Original MessageI've whittled it down to only a manual (comments omitted for brevity) #[derive_where(Clone, Copy, Hash, PartialEq, Debug; I: Interner, K)]
#[derive(TypeVisitable_Generic, GenericTypeVisitable, TypeFoldable_Generic, Lift_Generic)]
#[cfg_attr(
feature = "nightly",
derive(Decodable_NoContext, Encodable_NoContext, StableHash_NoContext)
)]
pub struct Alias<I: Interner, K> {
pub args: I::GenericArgs,
pub kind: K,
#[derive_where(skip(Debug))]
#[type_visitable(ignore)]
#[type_foldable(identity)]
pub(crate) _use_alias_new_instead: (),
}I get 2000+ errors of which fall into the following; A sample of errors being; And so on... This was similar to |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
667b095 to
b7965ca
Compare
This comment has been minimized.
This comment has been minimized.
|
The latest commits expand pub struct Binder<I: Interner, T> {
//
}and then pub struct Binder<I: Interner, T = SomeKind<I>> {
//
}e83546d, fbf009a Not sure if these would be best in a separate PR that also clean up Which I then used to clean up I can't get Eq Erroredit: Actually we can remove the empty implementation if we make |
This comment has been minimized.
This comment has been minimized.
…ric-capabilities, r=lcnr
Support generic params in `Lift_Generic`
Handle generic type parameters, including nested occurrences, when deriving `Lift_Generic`.
This lets types like `Binder<I, T>` use `#[derive(Lift_Generic)]` instead of requiring a manual `Lift` impl.
Concretely it can now handle structs as follows;
```rs
// Generic type parameter
struct Binder<I: Interner, T> {
// body
}
// Nested generic type parameter
pub struct Binder<I: Interner, T = SomeKind<I>> {
//
}
```
Split off from the `Alias` refactor work; rust-lang#156538
r? @lcnr
Rollup merge of #156956 - Jamesbarford:feat/extend-lift-generic-capabilities, r=lcnr Support generic params in `Lift_Generic` Handle generic type parameters, including nested occurrences, when deriving `Lift_Generic`. This lets types like `Binder<I, T>` use `#[derive(Lift_Generic)]` instead of requiring a manual `Lift` impl. Concretely it can now handle structs as follows; ```rs // Generic type parameter struct Binder<I: Interner, T> { // body } // Nested generic type parameter pub struct Binder<I: Interner, T = SomeKind<I>> { // } ``` Split off from the `Alias` refactor work; #156538 r? @lcnr
e2ba945 to
c3bdf72
Compare
This comment has been minimized.
This comment has been minimized.
71c7efb to
bf409af
Compare
| interner.debug_assert_args_compatible(kind.into(), args); | ||
| Alias { kind, args, _use_alias_new_instead: () } | ||
| } | ||
| } |
There was a problem hiding this comment.
hmm, could we change this to
trait AliasKind<I: Interner>: Copy {
fn assert_args_compatible(self, cx: I, args: I::GenericArgs);
#[inline]
fn debug_assert_args_compatible(self, cx: I, args: I::GenericArgs) {
if cfg!(debug_assertions) { self.assert... }
}
}
impl<I: Interner, K: AliasKind> Alias<I, K> {
pub fn new(
cx: I,
kind: K,
args: I::GenericArgs,
) -> Self {
kind.debug_assert_args_compatible(interner, args);
Alias { kind, args, _use_alias_new_instead: () }
}
pub fn new_from_iter(
cx: I,
kind: K,
args: impl IntoIterator<...>,
) -> Self { ... }
}There was a problem hiding this comment.
Following from @khyperia's comment I've reverted the change to the fn normalize_anon_const(...) which has led to this change I made being redundant
There was a problem hiding this comment.
(this is more of a comment to @lcnr for future work than feedback for this PR) - I do like the concept of a trait AliasKind. Here's some thoughts for a sketch for a future refactor:
enum InherentAliasTermKind { // or InherentTermKind? idk
Ty { def_id },
Const { def_id },
}
trait AliasKindWithDefId {
fn def_id(self) -> DefId;
}
impl AliasKindWithDefId for InherentAliasTermKind { ... }
// N.B: NO AliasKindWithDefId impl for AliasTermKind
impl<T: AliasKindWithDefId> AliasKindWithDefId for Alias<T> { // optional (for convenience)
fn def_id(self) -> DefId { self.kind.def_id() }
}
impl Alias<AliasTermKind> {
fn expect_ct(self) -> Alias<AliasConstKind> { .. can panic .. } // already exists today (the rest are new)
fn expect_inherent(self) -> Alias<InherentAliasTermKind> { .. can panic .. }
fn expect_anon_ct(self) -> Alias<I::UnevaluatedConstId> { .. can panic .. }
}
impl From<InherentAliasTermKind> for AliasTermKind { ... }
impl From<Alias<InherentAliasTermKind>> for Alias<AliasTermKind> { ... }
// (or a blanket impl for anything whose kind is From)
// delete fn expect_inherent_def_id(), replace with alias.expect_inherent().def_id()I should probably just make an issue for this...
| goal.predicate.alias.args, | ||
| ); | ||
| let alias = ty::AliasTerm::from(uv); | ||
| let goal = goal.with(cx, ty::NormalizesTo { alias, term: goal.predicate.term }); |
There was a problem hiding this comment.
I'm extremely on board with the style of thing that lcnr said with
with this we can now update
NormalizesToto also be generic over the kind, and e.g. fornormalize_anon_consthave the argument beGoal<I, ty::NormalizesTo<I, I::UnevaluatedConstId>>
there's a lot of places in the code that could hugely benefit from this (e.g. there's a large number of unwraps in #157653 that could be avoided), but this particular case with normalize_anon_const is a bit gross, and I would prefer keeping this as NormalizesTo<I, K = AliasTermKind<I>> (at least until additional refactorings happen that would e.g. allow evaluate_const_and_instantiate_normalizes_to_term to be more generic)
(this is just my code style opinion though, up to you!)
edit: just want to clarify, that PR has a lot of unwraps of the form:
let def_id = match inherent.kind {
ty::AliasTermKind::InherentTy { def_id } => def_id.into(),
ty::AliasTermKind::InherentConst { def_id } => def_id.into(),
kind => panic!("expected inherent alias, found {kind:?}"),
};having the Alias have a more specific Kind in all the cases that PR does that style of unwrap (it's so common that projections already have a helper method for it, expect_projection_def_id) would be super nice, but yeah
…able-generic, r=lcnr Extend capabilities of `TypeFoldable_Generic` Split from rust-lang#156538. - Lets `TypeFoldable_Generic` derive structural folding for types with extra generic parameters by adding the necessary `T: TypeFoldable<I>` bounds automatically - Means in rust-lang#156538 we can remove the manual `TypeFoldable` implementation for `NormalizesTo` - Refactors shared traversal logic between `Lift_Generic` and `TypeFoldable_Generic` into a shared helper with a callback. r? @lcnr
This comment has been minimized.
This comment has been minimized.
|
@bors delegate+ |
|
✌️ @Jamesbarford, you can now approve this pull request! If @lcnr told you to " |
| impl<I: Interner> fmt::Debug for NormalizesTo<I> { | ||
| impl<I: Interner, K> fmt::Debug for NormalizesTo<I, K> | ||
| where | ||
| Alias<I, K>: fmt::Debug, |
There was a problem hiding this comment.
| Alias<I, K>: fmt::Debug, | |
| K: fmt::Debug, |
this does not work? 🤔
does Alias implement Debug in a weird way?
There was a problem hiding this comment.
Nope 😞. TypeVisitable_Generic creates Alias<I, K>: TypeVisitable<I> and;
pub trait TypeVisitable<I: Interner>: fmt::Debug {
// ...
}So can prove Alias<I, K>: fmt::Debug but not K: Debug
Error;
error[E0277]: `K` doesn't implement `Debug`
--> compiler/rustc_type_ir/src/predicate.rs:957:12
|
957 | pub struct NormalizesTo<I: Interner, K = AliasTermKind<I>> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Debug` is not implemented for `K`
|
note: required for `predicate::NormalizesTo<I, K>` to implement `Debug`
--> compiler/rustc_type_ir/src/predicate.rs:976:22
|
976 | impl<I: Interner, K> fmt::Debug for NormalizesTo<I, K>
| ^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^
977 | where
978 | K: fmt::Debug,
| ---------- unsatisfied trait bound introduced here
note: required by a bound in `visit::TypeVisitable`
--> compiler/rustc_type_ir/src/visit.rs:62:39
|
62 | pub trait TypeVisitable<I: Interner>: fmt::Debug {
| ^^^^^^^^^^ required by this bound in `TypeVisitable`
There was a problem hiding this comment.
annoying 😭 alright, r=me if CI passes
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bf1ae1c to
30e608d
Compare
AliasTy & AliasTerm to use AliasAliasTy. AliasTerm & UnevaluatedConst to use Alias
|
@bors r=lcnr |
…liasTerm, r=lcnr Refactor `AliasTy`. `AliasTerm` & `UnevaluatedConst` to use `Alias` Refactors `AliasTy`, `AliasTerm` & `UnevaluatedConst` to use `Alias`. Part of rust-lang#156181 r? @lcnr
…liasTerm, r=lcnr Refactor `AliasTy`. `AliasTerm` & `UnevaluatedConst` to use `Alias` Refactors `AliasTy`, `AliasTerm` & `UnevaluatedConst` to use `Alias`. Part of rust-lang#156181 r? @lcnr
…uwer Rollup of 10 pull requests Successful merges: - #155535 (export symbols: support macos/windows(32/64)) - #156538 (Refactor `AliasTy`. `AliasTerm` & `UnevaluatedConst` to use `Alias`) - #156807 (Add `T: PartialEq` bounds to derived `StructuralPartialEq` impls.) - #156950 (Staticlib rename internal symbols) - #157702 (Add expansion info to implied bounds) - #155616 (constify `TryFrom<Vec>` for array) - #156226 (diagnostics: point to coroutine body on higher-ranked auto trait errors) - #157873 (mips: set llvm_args -mno-check-zero-division for all mips targets) - #157953 (fix(rustc_codegen_ssa): Use cg_operand for Freeze check) - #157958 (doc: Document `-Zlint-rust-version`)
…liasTerm, r=lcnr Refactor `AliasTy`. `AliasTerm` & `UnevaluatedConst` to use `Alias` Refactors `AliasTy`, `AliasTerm` & `UnevaluatedConst` to use `Alias`. Part of rust-lang#156181 r? @lcnr
…uwer Rollup of 13 pull requests Successful merges: - #156538 (Refactor `AliasTy`. `AliasTerm` & `UnevaluatedConst` to use `Alias`) - #156807 (Add `T: PartialEq` bounds to derived `StructuralPartialEq` impls.) - #156950 (Staticlib rename internal symbols) - #156983 (Add `io::Read::read_le` and `io::Read::read_be`) - #157306 (tests: codegen-llvm: Expect the new mangling scheme in bpf-abi-indirect-return) - #157702 (Add expansion info to implied bounds) - #155616 (constify `TryFrom<Vec>` for array) - #156226 (diagnostics: point to coroutine body on higher-ranked auto trait errors) - #157870 (std: sys: solid: clamp connect_timeout tv_sec instead of truncating) - #157952 (Configure Renovate for GitHub Actions) - #157953 (fix(rustc_codegen_ssa): Use cg_operand for Freeze check) - #157958 (doc: Document `-Zlint-rust-version`) - #157974 (Rename `errors.rs` file to `diagnostics.rs` (11/N))
View all comments
Refactors
AliasTy,AliasTerm&UnevaluatedConstto useAlias.Part of #156181
r? @lcnr