-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Add a way to add constructors for rustc_type_ir
types
#121703
Conversation
@@ -0,0 +1,19 @@ | |||
use crate::{BoundVar, ConstKind, DebruijnIndex, Interner, RegionKind, TyKind}; | |||
|
|||
pub trait Ty<I: Interner<Ty = Self>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe it's feasible to share code b/w this and rustc_middle
, since I expect we don't want to import this Ty
trait into hundreds and hundreds of modules in the compiler, and also this would conflict with the naming of Ty
in rustc_middle
.
This naming conflict is actually kind of intentional, since we can use Ty::new_x(...)
in uplifted solver code without really needing to change much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
time to figure out how to create custom preludes 🙃
The duplication seems fine, though it would be neat if we could generate most of the boilerplate
rustc_type_ir
typesrustc_type_ir
types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a good approach 👍
I think longterm we may want to lobby for some sort of #[treat_as_inherent_for_lookup]
when implementing traits for types in their local crate to remove the duplication. But at least for now this seems like it's probably the best we can do
compiler/rustc_middle/src/ty/sty.rs
Outdated
fn new(tcx: TyCtxt<'tcx>, kind: ty::TyKind<'tcx>) -> Self { | ||
Ty::new(tcx, kind) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is that method needed on the trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be used for default impls later on, perhaps. I expect implementations won't need to fill all of the constructors, but just provide the ones that cant be impl'd generically. Could remove until that point tho.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please do, I think statically preventing the solver from using new
is disirable
compiler/rustc_type_ir/src/new.rs
Outdated
} | ||
|
||
pub trait Region<I: Interner<Region = Self>> { | ||
fn new(interner: I, kind: RegionKind<I>) -> Self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, should never be used
compiler/rustc_type_ir/src/new.rs
Outdated
} | ||
|
||
pub trait Const<I: Interner<Const = Self>> { | ||
fn new(interner: I, kind: ConstKind<I>, ty: I::Ty) -> Self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here 😁
so... is I also think that if we're going to go this route, we consider not essentially duplicating traits here, and have a single trait trait New<I: Interner> {
type Out;
type Data;
fn new(interner: I, data: Self::Data) -> Self::Out;
}
trait Interner: Sized {
type Ty: ... + New<Self, Out = Self::Ty, Data = TyKind<Self>>;
} |
Yes, I believe it's incentivized by the same reason we put all of the constructors onto Ty in the first place. I expect us to be adding most of the ty constructors -- for now, we only have placeholder.
This is not compatible with the signature for Const constructors, which also take a type, and also won't work when we have various sprcialized constructors for various variants on ty/region/const. |
The
It certainly is: impl New<()> for Const {
type Out = Const;
type Data = (ConstKind<()>, Ty);
fn new(interner: (), data: Self::Data) -> Self::Out { Ty }
}
you can modify the trait like this if it's really necessary: trait New<I: Interner, Data> {
type Out;
fn new(interner: I, data: Data) -> Self::Out;
} |
There are 23 matches for the regex
Why? This is just scattering code over more crates for what, a bit less code duplication?
This seems really obtuse. Also, I'd rather not have to construct my types with |
Add a way to add constructors for `rustc_type_ir` types Introduces a module called `rustc_type_ir`, in which we can place traits which are named `Ty`/`Region`/`Const`/etc. which expose constructors for the `rustc_type_ir` types. This means we can construct things `Interner::Ty` with `Ty::new_x(...)`, which is needed to uplift the new trait solver into an interner-agnostic crate. These traits are placed into a *separate* module because they're only intended to be used in interner-agnostic code, and they should mirror the constructors that are provided by the inherent constructor methods in `rustc_middle`. Putting this up for vibe-check mostly. I haven't copied over any of the type constructors, except for one to create bound types for use in the canonicalizer. r? lcnr
…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#120976 (constify a couple thread_local statics) - rust-lang#121683 (Fix LVI tests after frame pointers are enabled by default) - rust-lang#121703 (Add a way to add constructors for `rustc_type_ir` types) - rust-lang#121732 (Improve assert_matches! documentation) - rust-lang#121928 (Extract an arguments struct for `Builder::then_else_break`) - rust-lang#121939 (Small enhancement to description of From trait) - rust-lang#121968 (Don't run test_get_os_named_thread on win7) - rust-lang#121969 (`ParseSess` cleanups) - rust-lang#121977 (Doc: Fix incorrect reference to integer in Atomic{Ptr,Bool}::as_ptr.) - rust-lang#121994 (Update platform-support.md with supported musl version) r? `@ghost` `@rustbot` modify labels: rollup
…iaskrgr Rollup of 10 pull requests Successful merges: - rust-lang#120976 (constify a couple thread_local statics) - rust-lang#121683 (Fix LVI tests after frame pointers are enabled by default) - rust-lang#121703 (Add a way to add constructors for `rustc_type_ir` types) - rust-lang#121732 (Improve assert_matches! documentation) - rust-lang#121928 (Extract an arguments struct for `Builder::then_else_break`) - rust-lang#121939 (Small enhancement to description of From trait) - rust-lang#121968 (Don't run test_get_os_named_thread on win7) - rust-lang#121969 (`ParseSess` cleanups) - rust-lang#121977 (Doc: Fix incorrect reference to integer in Atomic{Ptr,Bool}::as_ptr.) - rust-lang#121994 (Update platform-support.md with supported musl version) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#121703 - compiler-errors:new, r=lcnr Add a way to add constructors for `rustc_type_ir` types Introduces a module called `rustc_type_ir`, in which we can place traits which are named `Ty`/`Region`/`Const`/etc. which expose constructors for the `rustc_type_ir` types. This means we can construct things `Interner::Ty` with `Ty::new_x(...)`, which is needed to uplift the new trait solver into an interner-agnostic crate. These traits are placed into a *separate* module because they're only intended to be used in interner-agnostic code, and they should mirror the constructors that are provided by the inherent constructor methods in `rustc_middle`. Putting this up for vibe-check mostly. I haven't copied over any of the type constructors, except for one to create bound types for use in the canonicalizer. r? lcnr
Introduces a module called
rustc_type_ir
, in which we can place traits which are namedTy
/Region
/Const
/etc. which expose constructors for therustc_type_ir
types. This means we can construct thingsInterner::Ty
withTy::new_x(...)
, which is needed to uplift the new trait solver into an interner-agnostic crate.These traits are placed into a separate module because they're only intended to be used in interner-agnostic code, and they should mirror the constructors that are provided by the inherent constructor methods in
rustc_middle
.Putting this up for vibe-check mostly. I haven't copied over any of the type constructors, except for one to create bound types for use in the canonicalizer.
r? lcnr