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.

Sign up for GitHub

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

Jump to bottom

Tracking issue: Move to Chalk IR #8313

Closed
14 tasks done
flodiebold opened this issue Apr 3, 2021 · 7 comments · Fixed by #8419
Closed
14 tasks done

Tracking issue: Move to Chalk IR #8313

flodiebold opened this issue Apr 3, 2021 · 7 comments · Fixed by #8419
Labels
A-ty type system / type inference / traits / method resolution E-has-instructions Issue has some instructions and pointers to code to get started S-actionable Someone could pick this issue up and work on it right now

Comments

@flodiebold
Copy link
Member

flodiebold commented Apr 3, 2021 •
edited
Loading

We want to get rid of our own representations of types and related data in hir_ty (like Ty, TraitRef and so on), and instead use the versions from chalk_ir everywhere. This is a rather large refactoring, and to achieve it without doing a gigantic PR that takes months, we're gradually aligning our own representations to look like the Chalk versions, with the goal that in the end we can mostly just define type Ty = chalk_ir::Ty<Interner> and be done.

We're actually getting close to this goal; I tried it to see where we are currently and we're at about 400 errors when doing the switch. Going through those errors, I found the following remaining tasks:

  • remove interner parameter from Substitution::interned (I added it wrongly)
  • move Ty inherent methods to free-standing, extension trait or builder (except those that exist on Chalk's Ty as well)
    • all the is_xxx and as_xxx and similar accessors should move to extension trait
    • equals_ctor and builtin_deref can be free-standing in the modules where they're used
    • substs and substs_mut would ideally be removed
  • same for Substitution (just prefix and suffix remaining)
  • rename TyKind::Unknown -> Error
  • rename TyKind::ForeignType -> Foreign
  • adjust Lifetime to be more like Chalk (i.e. there's an interned method that contains the actual enum)
  • self_type_parameter takes Interner (on TraitRef and ProjectionTy)
  • remove all Binders value accesses, use VariableKinds instead of just a usize
    • make helper function for construction of VariableKinds with a number of type vars, since that's all we ever do right now
  • align Binders::subst with Chalk's substitute
  • hide Substitution internals, which are still getting accessed in lots of places
  • add lifetimes/consts to TyKind::Ref, TyKind::Array, DynTy
  • FnPointer: remove num_args, add num_binders (those are not the same), add FnSubst, substs -> substitution
  • remove SolutionVariables
  • move InferenceVar to types module and align it with the Chalk version (to_inner / from_inner should probably become free-standing functions in unify.rs

Note that lots of these can be easily done with structural search & replace!
@flodiebold
Copy link
Member Author

I will start moving out the Ty creation methods to a dedicated builder next.

@Veykril Veykril mentioned this issue Apr 3, 2021
Closed
6 tasks
@flodiebold flodiebold added A-ty type system / type inference / traits / method resolution E-has-instructions Issue has some instructions and pointers to code to get started S-actionable Someone could pick this issue up and work on it right now labels Apr 3, 2021
@flodiebold flodiebold mentioned this issue Apr 4, 2021
Merged
bors bot added a commit that referenced this issue Apr 4, 2021
@bors @flodiebold
Merge #8327
0829960 
8327: Move `Ty` creation methods out of `Ty` (Chalk move preparation) r=flodiebold a=flodiebold

When we'll move to using `chalk_ir::Ty` (#8313), we won't be able to have our own inherent methods on `Ty` anymore, so we need to move the helpers elsewhere.
This adds a `TyBuilder` that allows easily constructing `Ty` and related types (`TraitRef`, `ProjectionTy`, `Substitution`). It also replaces `SubstsBuilder`. `TyBuilder` can construct different things based on its type parameter; e.g. if it has an `AdtId`, we're constructing an ADT type, but if it has a `TraitId`, we're constructing a `TraitRef`. The common thing for all of them is that we need to build a `Substitution`, so the API stays the same for all of them except at the beginning and end.

We also use `TyBuilder` to house various one-shot methods for constructing types, e.g. `TyBuilder::unit()`.

Co-authored-by: Florian Diebold <[email protected]>
@lnicola lnicola mentioned this issue Apr 5, 2021
Merged
bors bot added a commit that referenced this issue Apr 5, 2021
@bors @lnicola
Merge #8342
8a18f23 
8342: Rename `TyKind::Unknown` and `TyKind::ForeignType` (Chalk move) r=flodiebold a=lnicola

CC #8313

Co-authored-by: Laurențiu Nicola <[email protected]>
@lnicola lnicola mentioned this issue Apr 5, 2021
Merged
@flodiebold
Copy link
Member Author

I'll go for Binders next.

bors bot added a commit that referenced this issue Apr 5, 2021
@bors @lnicola
Merge #8344
013cc7d 
8344: Pass interner to `ProjectionTy::self_type_parameter` and `TraitRef::self_type_parameter` r=flodiebold a=lnicola

CC #8313

changelog skip

Co-authored-by: Laurențiu Nicola <[email protected]>
@flodiebold flodiebold mentioned this issue Apr 5, 2021
Merged
bors bot added a commit that referenced this issue Apr 5, 2021
@bors @flodiebold
Merge #8348
c91b537 
8348: Make `Binders` more like Chalk r=flodiebold a=flodiebold

Working towards #8313.
 - hide `value`
 - use `VariableKinds`
 - adjust `subst` to be like Chalk's `substitute`
 - also clean up some other `TypeWalk` stuff to prepare for it being replaced by Chalk's `Fold`

Co-authored-by: Florian Diebold <[email protected]>
@Veykril
Copy link
Member

Veykril commented Apr 5, 2021

I'll tackle add lifetimes/consts to TyKind::Ref, TyKind::Array, DynTy

@flodiebold flodiebold mentioned this issue Apr 5, 2021
Merged
bors bot added a commit that referenced this issue Apr 5, 2021
@bors @flodiebold
Merge #8356
7ac1c8f 
8356: Align more methods to Chalk r=flodiebold a=flodiebold

Related to #8313.

Move some inherent methods that don't exist in Chalk to an extension trait, remove some others.

Co-authored-by: Florian Diebold <[email protected]>
@flodiebold flodiebold mentioned this issue Apr 5, 2021
Merged
@flodiebold
Copy link
Member Author

@Veykril Great, keep in mind that for now we'll just always want to make them 'static 🙂

bors bot added a commit that referenced this issue Apr 5, 2021
@bors @flodiebold
Merge #8356
f25c1e7 
8356: Align more methods to Chalk r=flodiebold a=flodiebold

Related to #8313.

Move some inherent methods that don't exist in Chalk to an extension trait, remove some others.

Co-authored-by: Florian Diebold <[email protected]>
@flodiebold
Copy link
Member Author

I'll do FnPointer.

@flodiebold flodiebold mentioned this issue Apr 5, 2021
Merged
bors bot added a commit that referenced this issue Apr 5, 2021
@bors @flodiebold
Merge #8358
7ce0e9c 
8358: Align FnPointer with Chalk r=flodiebold a=flodiebold

CC #8313

Co-authored-by: Florian Diebold <[email protected]>
@Veykril Veykril mentioned this issue Apr 5, 2021
Merged
bors bot added a commit that referenced this issue Apr 6, 2021
@bors @Veykril
Merge #8359
047b531 
8359: Add Lifetime to TyKind::Ref and DynTy r=flodiebold a=Veykril

CC #8313

Co-authored-by: Lukas Wirth <[email protected]>
@Veykril Veykril mentioned this issue Apr 6, 2021
Merged
@Veykril
Copy link
Member

Veykril commented Apr 6, 2021

I'll move the accessors to the extension trait

@Veykril Veykril mentioned this issue Apr 6, 2021
Merged
bors bot added a commit that referenced this issue Apr 6, 2021
@bors @Veykril
Merge #8366
002e72a 
8366: Add chalk_ir::Const to TyKind::Array r=flodiebold a=Veykril

CC #8313

Co-authored-by: Lukas Wirth <[email protected]>
bors bot added a commit that referenced this issue Apr 6, 2021
@bors @Veykril
Merge #8368
12e8643 
8368: Move Ty accessors to TyExt r=flodiebold a=Veykril

CC #8313

Co-authored-by: Lukas Wirth <[email protected]>
@flodiebold flodiebold mentioned this issue Apr 6, 2021
Merged
@Veykril Veykril mentioned this issue Apr 7, 2021
Merged
bors bot added a commit that referenced this issue Apr 7, 2021
@bors @Veykril
Merge #8396
45510ae 
8396: Uncouple Ty::builtin_deref and Ty::def_crates from Ty r=Veykril a=Veykril

bors r+
CC #8313

Co-authored-by: Lukas Wirth <[email protected]>
@flodiebold
Copy link
Member Author

I'm doing the remaining substs, substs_mut and equals_ctor.

@flodiebold flodiebold mentioned this issue Apr 7, 2021
Merged
bors bot added a commit that referenced this issue Apr 7, 2021
@bors @flodiebold
Merge #8409
3191a93 
8409: Various remaining fixes for Chalk IR move r=flodiebold a=flodiebold

CC #8313

Co-authored-by: Florian Diebold <[email protected]>
@flodiebold flodiebold mentioned this issue Apr 8, 2021
Merged
@bors bors bot closed this as completed in 72ad5cb Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ty type system / type inference / traits / method resolution E-has-instructions Issue has some instructions and pointers to code to get started S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Move hir_ty to Chalk IR flodiebold/rust-analyzer
2 participants
@flodiebold @Veykril