Associated type basics & Deref support #1408
Conversation
- add support for other lang item targets, since we need the Deref lang item
crates/ra_hir/src/db.rs
Outdated
| fn normalize( | ||
| &self, | ||
| krate: Crate, | ||
| goal: crate::ty::Canonical<crate::ty::traits::ProjectionPredicate>, |
There was a problem hiding this comment.
Maybe we should make ty a facade module and use ty::Smth everywhere?
There was a problem hiding this comment.
Hmm. I agree for ProjectionPredicate, but when I started doing this for everything in traits, it feld weird for e.g. Solver and the query functions...
| let parameter_kinds = generic_params | ||
| .params_including_parent() | ||
| .into_iter() | ||
| .map(|p| chalk_ir::ParameterKind::Ty(lalrpop_intern::intern(&p.name.to_string()))) |
There was a problem hiding this comment.
I feel uneasy about lalrpop_intern: it uses threadlocals, so is not thread safe, and it leaks data.I guess nothing really bad happens, but, longer term, I wish we did something smarter here.
There was a problem hiding this comment.
Or rather, something smarter in chalk
There was a problem hiding this comment.
Heh, lalrpop_intern::InternedString is marked Send, which doesn't really makes sense because itnterner is thread local.
I guess, in theory it could be useful, if you access interner on the one thread, and pass a reference to it to another thread, but that looks like a niche use-case.
OTOH, accidentally using the same index on two different threads seems like a footgun
There was a problem hiding this comment.
Hm, actually, could this be a real bug? We can call chalk from different threads, and, if it caches InternedString inside, that would lead to a wrong result!
There was a problem hiding this comment.
Yeah, good question... I was unhappy about having to add the lalrpop dependency anyway, ideally we wouldn't need to provide this at all. I expect Chalk doesn't actually use the name except for debug output...
@nikomatsakis @scalexm Do you know whether this is a problem? Would it be possible to remove the parameter names / make them optional?
There was a problem hiding this comment.
Ah, if that's only for debug it's centrally fine short-term. Long term, yeah, it would be cool to separate debug interface from the main interface.
There was a problem hiding this comment.
Hm, actually it turns out it does use the name: When there's an 'unselected' projection like Vec::SomeAssociatedType where it's not clear from which trait the associated type actually comes. So we aren't concerned with that yet, but we'll need to handle it somehow.
Maybe it'd be better to just have some AssociatedTypeNameId in Chalk and let us do the interning ourselves...
|
Addressed most comments, the big open question is the interning now... |
|
bors r+ |
1408: Associated type basics & Deref support r=matklad a=flodiebold This adds the necessary Chalk integration to handle associated types and uses it to implement support for `Deref` in the `*` operator and autoderef; so e.g. dot completions through an `Arc` work now. It doesn't yet implement resolution of associated types in paths, though. Also, there's a big FIXME about handling variables in the solution we get from Chalk correctly. Co-authored-by: Florian Diebold <[email protected]>
Build succeeded |
|
Makes sense given that names will be hygienic some day
…On Sun, 16 Jun 2019 at 15:33, Florian Diebold ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In crates/ra_hir/src/ty/traits/chalk.rs
<#1408 (comment)>
:
> @@ -225,8 +255,29 @@ impl<'a, DB> chalk_solve::RustIrDatabase for ChalkContext<'a, DB>
where
DB: HirDatabase,
{
- fn associated_ty_data(&self, _ty: TypeId) -> Arc<AssociatedTyDatum> {
- unimplemented!()
+ fn associated_ty_data(&self, id: TypeId) -> Arc<AssociatedTyDatum> {
+ debug!("associated_ty_data {:?}", id);
+ let type_alias: TypeAlias = from_chalk(self.db, id);
+ let trait_ = match type_alias.container(self.db) {
+ Some(crate::Container::Trait(t)) => t,
+ _ => panic!("associated type not in trait"),
+ };
+ let generic_params = type_alias.generic_params(self.db);
+ let parameter_kinds = generic_params
+ .params_including_parent()
+ .into_iter()
+ .map(|p| chalk_ir::ParameterKind::Ty(lalrpop_intern::intern(&p.name.to_string())))
Hm, actually it turns out it does use the name: When there's an
'unselected' projection like Vec::SomeAssociatedType where it's not clear
from which trait the associated type actually comes. So we aren't concerned
with that yet, but we'll need to handle it somehow.
Maybe it'd be better to just have some AssociatedTypeNameId in Chalk and
let us do the interning ourselves...
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1408>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AANB3MY6M6FQOZPIAOS5YULP2YXLJANCNFSM4HYPMK5A>
.
|
The limit was introduced in rust-lang#1408 (comment) to avoid infinite cycles but it effectively caps the number of derefs to 10. Types like `ID3D12Device14` from the `windows` crate run into this because it derefs to `ID3D12Device13`, 13 to 12 and so on. Increasing it to 20 is a quick fix; a better cycle detection method would be nicer long term.
The limit was introduced in rust-lang/rust-analyzer#1408 (comment) to avoid infinite cycles but it effectively caps the number of derefs to 10. Types like `ID3D12Device14` from the `windows` crate run into this because it derefs to `ID3D12Device13`, 13 to 12 and so on. Increasing it to 20 is a quick fix; a better cycle detection method would be nicer long term.
This adds the necessary Chalk integration to handle associated types and uses it to implement support for
Derefin the*operator and autoderef; so e.g. dot completions through anArcwork now.It doesn't yet implement resolution of associated types in paths, though. Also, there's a big FIXME about handling variables in the solution we get from Chalk correctly.