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

Add a way to add constructors for rustc_type_ir types #121703

Merged
merged 1 commit into from Mar 5, 2024

Conversation

compiler-errors
Copy link
Member

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 27, 2024
@@ -0,0 +1,19 @@
use crate::{BoundVar, ConstKind, DebruijnIndex, Interner, RegionKind, TyKind};

pub trait Ty<I: Interner<Ty = Self>> {
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 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.

Copy link
Contributor

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

@compiler-errors compiler-errors changed the title Allow a way to add constructors for rustc_type_ir types Add a way to add constructors for rustc_type_ir types Feb 27, 2024
Copy link
Contributor

@lcnr lcnr left a 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

Comment on lines 1816 to 1818
fn new(tcx: TyCtxt<'tcx>, kind: ty::TyKind<'tcx>) -> Self {
Ty::new(tcx, kind)
}
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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

}

pub trait Region<I: Interner<Region = Self>> {
fn new(interner: I, kind: RegionKind<I>) -> Self;
Copy link
Contributor

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

}

pub trait Const<I: Interner<Const = Self>> {
fn new(interner: I, kind: ConstKind<I>, ty: I::Ty) -> Self;
Copy link
Contributor

Choose a reason for hiding this comment

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

and here 😁

@jackh726
Copy link
Contributor

so...

is Ty::new_anon_bound(self.interner(), self.binder_index, var) really all that much better than self.interner().mk_bound_ty(self.binder_index, var)?

I also think that if we're going to go this route, we consider not essentially duplicating traits here, and have a single trait New like:

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>>;
}

example

@compiler-errors
Copy link
Member Author

[...] really all that much better [...]

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.

we consider not essentially duplicating traits here, and have a single trait New like [...]

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.

@jackh726
Copy link
Contributor

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.

The rustc_type_ir crate only really needs one function to create the interned type - I would expect the "core" solver code probably doesn't have a ton of places where we would need all the separate variants. Implementing crates can provide their own helper functions for the separate variants.

This is not compatible with the signature for Const constructors, which also take a type

It certainly is:

impl New<()> for Const {
    type Out = Const;
    type Data = (ConstKind<()>, Ty);
    fn new(interner: (), data: Self::Data) -> Self::Out { Ty }
}

and also won't work when we have various sprcialized constructors for various variants on ty/region/const.

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;
}

@compiler-errors
Copy link
Member Author

The rustc_type_ir crate only really needs one function to create the interned type - I would expect the "core" solver code probably doesn't have a ton of places where we would need all the separate variants.

There are 23 matches for the regex (Ty|Const|Region)::new_ in the solver code (creating error variants, projections, adts, slices, and tuples). And an additional 9 matches to \.(types|lifetimes|consts)\. (unit ty and ReStatic) which will need to go through constructors as well.

Implementing crates can provide their own helper functions for the separate variants.

Why? This is just scattering code over more crates for what, a bit less code duplication?

you can modify the trait like this if it's really necessary:

This seems really obtuse. Also, I'd rather not have to construct my types with New::new() -- the type punning here with trait Ty being named identically to Ty is intentional, since it ensures that code reads similarly between compiler-internal code and the code being abstracted here (e.g. Ty::new_unit(tcx) and Ty::new_unit(self.interner())).

@lcnr
Copy link
Contributor

lcnr commented Mar 4, 2024

given @jackh726 👍 above

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 4, 2024

📌 Commit 1eedca8 has been approved by lcnr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 4, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 4, 2024
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
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2024
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 4, 2024
…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
@bors bors merged commit e7bb224 into rust-lang:master Mar 5, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Mar 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 5, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants