Complete assoc. items on type parameters#4162
Complete assoc. items on type parameters#4162bors[bot] merged 6 commits intorust-lang:masterfrom jonas-schievink:complete-assoc-on-params
Conversation
flodiebold
left a comment
flodiebold
left a comment
There was a problem hiding this comment.
Yeah, we shouldn't expose those types, I've got some suggestions how to avoid that.
crates/ra_hir/src/lib.rs
Outdated
| nameres::ModuleSource, | ||
| path::{ModPath, Path, PathKind}, | ||
| type_ref::Mutability, | ||
| AssocItemId, |
There was a problem hiding this comment.
yeah, we don't really want to expose that
crates/ra_hir/src/semantics.rs
Outdated
| } | ||
|
|
||
| impl PathResolution { | ||
| pub fn in_type_ns(self) -> Option<TypeNs> { |
There was a problem hiding this comment.
hmm I wouldn't really want to have this public -- PathResolution is a 'simplified' type for the facade that loses the namespace information, so trying to infer the namespace again might be lossy. It'll be good enough for this purpose, but might lead to problems otherwise. (Apart from the fact that we don't want to expose TypeNs.)
crates/ra_hir_ty/src/lower.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn associated_items<R>( |
There was a problem hiding this comment.
we're really only interested in associated types here, so why not make it only iterate those, and pass TypeAliasIds?
also how about calling this iterate_associated_type_shorthand_candidates or something like that... though that's admittedly a bit long 🤔
There was a problem hiding this comment.
something like that... though that's admittedly a bit long thinking
something something when writing a command line compiler, you get tyctx, when writing an IDE, you get iterate_associated_type_shorthand_candidates
There was a problem hiding this comment.
😄 I did think about adding, "... but we have completion"
| associated_items(ctx.db, typens, |_, _, assoc_id| { | ||
| match assoc_id { | ||
| AssocItemId::ConstId(c) => acc.add_const(ctx, c.into()), | ||
| AssocItemId::FunctionId(f) => acc.add_function(ctx, f.into(), None), |
There was a problem hiding this comment.
I don't think we should try to handle associated consts or functions with this, that's bound to get edge cases wrong. Those should be done through the normal Type methods as above.
| } | ||
| Some(res @ PathResolution::TypeParam(_)) | Some(res @ PathResolution::SelfType(_)) => { | ||
| if let Some(typens) = res.in_type_ns() { | ||
| associated_items(ctx.db, typens, |_, _, assoc_id| { |
There was a problem hiding this comment.
API-wise, maybe we should just put a wrapper for associated_items on PathResolution. Then we don't need a public in_type_ns method. Also the wrapper can turn the TypeAliasIds into TypeAliases, which is the proper HIR facade wrapper. Finally, I think we don't even need to match here -- we can just unconditionally call that method on res, and it'll return items when it's the right kind of resolution.
|
Thanks for the review, I've pushed a new version that's much cleaner. |
crates/ra_hir/src/semantics.rs
Outdated
| PathResolution::Def(ModuleDef::Function(_)) => None, | ||
| PathResolution::Def(ModuleDef::Module(_)) => None, | ||
| PathResolution::Def(ModuleDef::Static(_)) => None, | ||
| PathResolution::Def(ModuleDef::Trait(_)) => None, |
There was a problem hiding this comment.
Super minor, but I'd probably do a nested match and a bunch of | here
crates/ra_hir/src/semantics.rs
Outdated
| db: &dyn HirDatabase, | ||
| mut cb: impl FnMut(TypeAlias) -> Option<R>, | ||
| ) -> Option<R> { | ||
| if let Some(res) = self.clone().in_type_ns() { |
| let scope = ctx.scope(); | ||
| let context_module = scope.module(); | ||
|
|
||
| let res = if let Some(res) = scope.resolve_hir_path(&path) { |
There was a problem hiding this comment.
|
LGTM otherwise. bors d+ |
|
✌️ jonas-schievink can now approve this pull request. To approve and merge a pull request, simply reply with |
|
bors r+ |
|
Build succeeded: |
3 participants
This is fairly messy and seems to leak a lot through the
ra_hirabstraction (TypeNs,AssocItemId, ...), so I'd be glad for any advise for how to improve this.