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

refactor types to fit the chalk-ty generic plan #341

Closed
1 of 3 tasks
nikomatsakis opened this issue Aug 5, 2020 · 2 comments
Closed
1 of 3 tasks

refactor types to fit the chalk-ty generic plan #341

nikomatsakis opened this issue Aug 5, 2020 · 2 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@nikomatsakis
Copy link
Contributor

Proposal

In March of 2020, we had a design meeting about the idea to create a shared library for types, specifically one that could be used for chalk, rust-analyzer, and rustc alike. This is part of a process that aims to allow us to extract trait/type-checking into distinct crates that can be reused in multiple contexts. In particular we would like chalk to be integrated into rustc without having to do any "translation" between the different representations of types.

As described in the design meeting, the expectation is that this shared library will consist of a separation between a Ty<I> wrapper type, where I is the 'interner', and a TyData or TyKind enum that contains the variants (name to be settled). A method data() or kind() will be used to go from one to the other. rustc already has a separation much like this, in the form of the Ty<'tcx> and TyKind<'tcx> types, but to match this description fully, these types would change somewhat:

  • Ty<'tcx> would eventually be an alias for ty::Ty<TyCtxt<'tcx>>, where ty::Ty is the generic definition from this shared library.
  • As you can see, the TyCtxt<'tcx> would play the role of the interner.

This MCP proposes the first few step towards that goal, which I think are simply

  • change from ty.kind to ty.kind(), which is fn kind(&self) -> &TyKind
  • change from ty.kind() to ty.kind(tcx) -- in chalk, accessing the kind requires the interner, which permits types that use integers instead of interned pointers, but we could skip this step if we wanted
  • other minor changes of this kind

Implications. There is a prototype PR in rust-lang/rust#75077 that works through some of the implications. One point to note is that match ty.kind() { .. .} produces references for the fields within, which is often not what we want, because those fields are copy, and thus sometimes match *ty.kind() { .. } is preferable (though, as you can see in rust-lang/rust#75077, not always.

Alternatives. It occurs to me that the shared type library could be setup such that Ty<I> implements Deref to some type with a field named kind for some interners, i.e., those that implement an auxiliary trait, thus avoiding the need to make this change -- we would still want to make this change for each library that aims to be reused in multiple contexts, but for rustc-specific code it wouldn't be necessary.

Not included in this MCP

This MCP does NOT include reorganizing the variants of types. In chalk, we have a much more minimal set of variants. I do think we should reorganize the rustc variants but I think it'd be a good idea to discuss those steps separately.

Mentors or Reviewers

@nikomatsakis can mentor to the extent it is needed

@LeSeulArtichaut started doing the implementation work

Process

The main points of the Major Change Process is as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@nikomatsakis nikomatsakis added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Aug 5, 2020
@rustbot
Copy link
Collaborator

rustbot commented Aug 5, 2020

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Aug 5, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Aug 5, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Aug 7, 2020

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Aug 7, 2020
@spastorino spastorino added major-change-accepted A major change proposal that was accepted and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Aug 19, 2020
@rustbot rustbot added the to-announce Announce this issue on triage meeting label Aug 19, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Aug 19, 2020
bors added a commit to rust-lang-ci/rust that referenced this issue May 29, 2022
…h726,oli-obk

Move things to `rustc_type_ir`

Finishes some work proposed in rust-lang/compiler-team#341.

r? `@ghost`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

4 participants