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

Complete assoc. items on type parameters #4162

Merged
merged 6 commits into from
Apr 29, 2020
Merged

Complete assoc. items on type parameters #4162

merged 6 commits into from
Apr 29, 2020

Conversation

jonas-schievink
Copy link
Contributor

This is fairly messy and seems to leak a lot through the ra_hir abstraction (TypeNs, AssocItemId, ...), so I'd be glad for any advise for how to improve this.

Copy link
Member

@flodiebold flodiebold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we shouldn't expose those types, I've got some suggestions how to avoid that.

@@ -68,8 +68,9 @@ pub use hir_def::{
nameres::ModuleSource,
path::{ModPath, Path, PathKind},
type_ref::Mutability,
AssocItemId,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we don't really want to expose that

@@ -40,6 +41,35 @@ pub enum PathResolution {
AssocItem(AssocItem),
}

impl PathResolution {
pub fn in_type_ns(self) -> Option<TypeNs> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

@@ -694,6 +673,56 @@ pub fn callable_item_sig(db: &dyn HirDatabase, def: CallableDef) -> PolyFnSig {
}
}

pub fn associated_items<R>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔

Copy link
Member

@matklad matklad Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😄 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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -91,8 +93,21 @@ pub(super) fn complete_qualified_path(acc: &mut Completions, ctx: &CompletionCon
}
}
}
Some(res @ PathResolution::TypeParam(_)) | Some(res @ PathResolution::SelfType(_)) => {
if let Some(typens) = res.in_type_ns() {
associated_items(ctx.db, typens, |_, _, assoc_id| {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jonas-schievink
Copy link
Contributor Author

Thanks for the review, I've pushed a new version that's much cleaner.

PathResolution::Def(ModuleDef::Function(_)) => None,
PathResolution::Def(ModuleDef::Module(_)) => None,
PathResolution::Def(ModuleDef::Static(_)) => None,
PathResolution::Def(ModuleDef::Trait(_)) => None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor, but I'd probably do a nested match and a bunch of | here

db: &dyn HirDatabase,
mut cb: impl FnMut(TypeAlias) -> Option<R>,
) -> Option<R> {
if let Some(res) = self.clone().in_type_ns() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

let scope = ctx.scope();
let context_module = scope.module();

let res = if let Some(res) = scope.resolve_hir_path(&path) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@flodiebold
Copy link
Member

LGTM otherwise.

bors d+

@bors
Copy link
Contributor

bors bot commented Apr 29, 2020

✌️ jonas-schievink can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@jonas-schievink
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 29, 2020

@bors bors bot merged commit 913eff5 into rust-lang:master Apr 29, 2020
@jonas-schievink jonas-schievink deleted the complete-assoc-on-params branch April 29, 2020 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants