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

[RFC] Refactor types to be a (TypeCore, Substs) pair #42340

Open
4 tasks done
nikomatsakis opened this issue May 31, 2017 · 5 comments · Fixed by #65100
Open
4 tasks done

[RFC] Refactor types to be a (TypeCore, Substs) pair #42340

nikomatsakis opened this issue May 31, 2017 · 5 comments · Fixed by #65100
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented May 31, 2017

Currently, the Ty<'tcx> type is a reference to a struct (&TyS) that packages up a big ol' enum TypeVariants. As part of chalkification, we would like to be able to write more efficient "decision tree" like procedures that branch on this type. This would be made nicer by factoring out the type into two parts, a "core" and a "substs". This core would be roughly equivalent to the existing TypeVariants, but without the recursive references to other types, which would be uniformly packaged up in the substs.

Thus I imagine that the TyS struct would change to something like this:

pub struct TyS<'tcx> {
    pub sty: TypeVariants<'tcx>, // this is the "core" part
    pub substs: &'tcx Substs<'tcx>, // this is the "substs" part, extracted from `TypeVariants`
    pub flags: TypeFlags,

    // the maximal depth of any bound regions appearing in this type.
    region_depth: u32,
}

(Incidentally, this new notion of TypeVariants could maybe subsume the existing SimplifiedType, though that's not super important.)

Steps

This transition is best made in steps:

Unknowns

It's not 100% clear that this is a good plan, though the individual refactorings above all seem like good steps regardless. Some complicating factors:

cc @eddyb, who first suggested this to me
cc @tschottdorf, who implemented #42297, a step along this path (and an important step for ATC regardless)

@nikomatsakis nikomatsakis added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 31, 2017
@tbg
Copy link
Contributor

tbg commented May 31, 2017

just a heads up - #42171 isn't closed yet (only half of it), though I'm planning on getting to the other half this week.

@nikomatsakis
Copy link
Contributor Author

@tschottdorf ah yeah, I was thinking of the "check" as "in progress", but ... seems fine

@Mark-Simulacrum Mark-Simulacrum added the C-cleanup Category: PRs that clean code up or issues documenting cleanup. label Jul 27, 2017
Centril added a commit to Centril/rust that referenced this issue Apr 27, 2019
Replace the `&'tcx List<Ty<'tcx>>` in `TyKind::Tuple` with `SubstsRef<'tcx>`

Part of the suggested refactoring for rust-lang#42340. As expected, this is a little messy, because there are many places that the components of tuples are expected to be types, rather than arbitrary kinds. However, it should open up the way for a refactoring of `TyS` itself.

r? @nikomatsakis
Centril added a commit to Centril/rust that referenced this issue Apr 27, 2019
Replace the `&'tcx List<Ty<'tcx>>` in `TyKind::Tuple` with `SubstsRef<'tcx>`

Part of the suggested refactoring for rust-lang#42340. As expected, this is a little messy, because there are many places that the components of tuples are expected to be types, rather than arbitrary kinds. However, it should open up the way for a refactoring of `TyS` itself.

r? @nikomatsakis
Centril added a commit to Centril/rust that referenced this issue Apr 27, 2019
Replace the `&'tcx List<Ty<'tcx>>` in `TyKind::Tuple` with `SubstsRef<'tcx>`

Part of the suggested refactoring for rust-lang#42340. As expected, this is a little messy, because there are many places that the components of tuples are expected to be types, rather than arbitrary kinds. However, it should open up the way for a refactoring of `TyS` itself.

r? @nikomatsakis
Centril added a commit to Centril/rust that referenced this issue Oct 4, 2019
Replace ClosureSubsts with SubstsRef

Addresses rust-lang#42340 part 3
rust-lang#59312 might benefit from this clean up.
r? @nikomatsakis
@eddyb
Copy link
Member

eddyb commented Oct 4, 2019

Existential types carry where-clauses

Could they be moved to the binder? dyn Foo -> dyn<X: Foo> X would result in Ty::Dyn having all the information in the binder and then substs would have just an anonymous type variable (bound at the aforementioned binder) or be empty for now.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Oct 4, 2019
Centril added a commit to Centril/rust that referenced this issue Oct 4, 2019
Centril added a commit to Centril/rust that referenced this issue Oct 4, 2019
tmandry added a commit to tmandry/rust that referenced this issue Oct 5, 2019
@bors bors closed this as completed in 7739f17 Oct 6, 2019
@csmoe
Copy link
Member

csmoe commented Oct 6, 2019

@nikomatsakis @eddyb not sure whether this issue should be closed or not. How about those Unknowns? (If needed, I have a lot of brandwidth to volunteer).

@eddyb
Copy link
Member

eddyb commented Oct 6, 2019

This isn't done, the checkboxes were only for prerequisites, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants