-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Convert some things from chalk_ir types to rustc_type_ir types #20454
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
Convert some things from chalk_ir types to rustc_type_ir types #20454
Conversation
e0fa509
to
d12e875
Compare
hir_fmt_generic_arguments(f, parameters_to_write, self_)?; | ||
write!(f, ">")?; | ||
} | ||
impl<'db> HirDisplay for crate::next_solver::Ty<'db> { |
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 you should replace the chalk ty display impl to converting to new solver ty then displaying it, to reduce duplicate code and maintenance efforts.
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.
It will also help us better test the code.
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 forwarded these impls. Associated type printing changes (only in tests). Had to add some salsa::attach
calls, but otherwise no problems.
mut check_alias: impl FnMut(&Name, TraitRef<'db>, TypeAliasId) -> Option<R>, | ||
) -> Option<R> { | ||
let db = interner.db; | ||
let mut search = |t: TraitRef<'db>| -> Option<R> { |
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.
You basically inlined all_super_trait_refs()
? Why? Isn't it better to extract it to its own function?
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.
Basically just moving things around here, not adding anything new.
IIRC, all_super_trait_refs
does not work because of cycles, but I don't quite remember. Personally, I would not want to try to abstract this out in this PR since it's just moving things.
|
||
// we're _in_ the impl -- the binders get added back later. Correct, | ||
// but it would be nice to make this more explicit | ||
search(trait_ref.skip_binder()) |
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 did you not add the check for being inside an impl method?
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 remember, I actually ported this months ago. I'll add a FIXME to come back to it, but don't want to figure this out here.
})) | ||
} | ||
|
||
pub(crate) fn associated_ty_item_bounds<'db>( |
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 not querify this? Although I see you haven't put the interner's one a query either. But at least reuse the code from DbInterner::item_bounds()
?
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.
The equivalent code from chalk_db was not querified either. I added a FIXME to reuse with explicit_item_bounds
.
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.
Overall LGTM but I have some comments.
152571b
to
77cd3dd
Compare
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.
Thanks!
…redicates_without_parent_query
77cd3dd
to
596a6bf
Compare
Rebased - the test from #20462 had its output change (associated type printing in tests) |
Each commit compiles, so these could land in parts if desired.