Stream: t-compiler/wg-rls-2.0

Topic: rust-analyzer#878 - Add missing impl members


Igor Matuszewski (Mar 06 2019 at 15:19, on Zulip):

Hey @matklad, figured it'd be good to create a topic for that

Igor Matuszewski (Mar 06 2019 at 15:20, on Zulip):

I'm first trying to do the basic approach, which is to complete simple impl SomeTrait for SomeStruct block and I have a question regarding syntax nodes

Igor Matuszewski (Mar 06 2019 at 15:21, on Zulip):

if I understand correctly, we want to offer this assist when the cursor is under the WHITESPACE, ITEM_LIST, IMPL_BLOCK path (child -> parent direction), correct?

matklad (Mar 06 2019 at 15:21, on Zulip):

Yeah

matklad (Mar 06 2019 at 15:22, on Zulip):

I'd say this simpler, "cursor has ImplBlcok as an ancestor"

matklad (Mar 06 2019 at 15:22, on Zulip):

So, even if you are inside the function, the assist is applicable (if there are some non-oferriden functions)

Igor Matuszewski (Mar 06 2019 at 15:23, on Zulip):

but what if the cursor is inside: impl X for Y { fn some (&self) {<|>} }?

Igor Matuszewski (Mar 06 2019 at 15:23, on Zulip):

oh yeah

matklad (Mar 06 2019 at 15:23, on Zulip):

That is, being super-fine-grained about where assist is applicable is usually to super important

Igor Matuszewski (Mar 06 2019 at 15:23, on Zulip):

I thought we want to do that only when the cursor is in a 'freestanding' position in the impl block

matklad (Mar 06 2019 at 15:23, on Zulip):

is usually not super important

Igor Matuszewski (Mar 06 2019 at 15:23, on Zulip):

gotcha

Igor Matuszewski (Mar 06 2019 at 15:24, on Zulip):

another question - I did found the ast::ImplBlock node but I can't seem to find the path to a trait

matklad (Mar 06 2019 at 15:24, on Zulip):

the observation here is that we only show assist if there are some methods missing. So we won't create a spurious entry in assist popup

matklad (Mar 06 2019 at 15:24, on Zulip):

I think this is implemented manually in ast.rs

Igor Matuszewski (Mar 06 2019 at 15:25, on Zulip):

(the awkward moment when you browse to a Serbian site :laughter_tears: )

matklad (Mar 06 2019 at 15:25, on Zulip):
impl ImplBlock {
    pub fn target_type(&self) -> Option<&TypeRef> {
        match self.target() {
            (Some(t), None) | (_, Some(t)) => Some(t),
            _ => None,
        }
    }

    pub fn target_trait(&self) -> Option<&TypeRef> {
        match self.target() {
            (Some(t), Some(_)) => Some(t),
            _ => None,
        }
    }
matklad (Mar 06 2019 at 15:25, on Zulip):

:D

Igor Matuszewski (Mar 06 2019 at 15:25, on Zulip):

ah! I was browsing the generated impls and couldn't find it

Igor Matuszewski (Mar 06 2019 at 15:25, on Zulip):

thanks!

matklad (Mar 06 2019 at 15:26, on Zulip):

Yeah, these can't be generated, because we use a stupid "find first node of appropriate type" approach, and there are two types

Igor Matuszewski (Mar 06 2019 at 15:26, on Zulip):

but will the target_trait be the trait we implement for a given... target?

Igor Matuszewski (Mar 06 2019 at 15:26, on Zulip):

(confused about naming, want to double check)

matklad (Mar 06 2019 at 15:27, on Zulip):

yeah, target_trait should be a Trait, target_type should be a Type. references to traits and types use the same grammar

matklad (Mar 06 2019 at 15:27, on Zulip):

And I actually can't recall an easy way to resolve ast::TypeRef to a trait...

matklad (Mar 06 2019 at 15:28, on Zulip):

perhaps we just don't have one and it needs to be implemented?

matklad (Mar 06 2019 at 15:28, on Zulip):

To do this, you'll need to check that TypeRef is, in fact, a path type, and resolve it

Florian Diebold (Mar 06 2019 at 15:29, on Zulip):

perhaps we just don't have one and it needs to be implemented?

yeah I think we don't have it yet, will soon need it though ;)

Florian Diebold (Mar 06 2019 at 15:30, on Zulip):

that the target trait is a TypeRef is actually a bit weird, but I guess syntactically it is

Florian Diebold (Mar 06 2019 at 15:30, on Zulip):

though I wonder if it should maybe already be a different type in the HIR

Igor Matuszewski (Mar 06 2019 at 15:30, on Zulip):

cool, thanks! I'll look around some more then

Igor Matuszewski (Mar 06 2019 at 16:54, on Zulip):

Sorry it's going so slow, still trying to wrap my head around the structure and HIR

Igor Matuszewski (Mar 06 2019 at 16:55, on Zulip):

@matklad from what I understand I can get resolver for a current position via hir::source_binder::resolver_for_position

Igor Matuszewski (Mar 06 2019 at 16:55, on Zulip):

and I need that Resolver to resolve_path

Igor Matuszewski (Mar 06 2019 at 16:56, on Zulip):

so I think I did all that and I think I can see two approaches clearing up - HIR-based (using more concrete definitions) and syntax-based (operating solely on nodes)

Igor Matuszewski (Mar 06 2019 at 16:56, on Zulip):

you were saying you prefer syntax-oriented solution, right?

Igor Matuszewski (Mar 06 2019 at 16:57, on Zulip):

right now, basically I go from hir::Trait (obtained via Resolver::resolve_path and the inner hir::Resolution::Def(hir::ModuleDef::Trait(trait: hir::Trait))) by calling source() which gets me ast::TraitDef

Igor Matuszewski (Mar 06 2019 at 16:59, on Zulip):

I assume I need to scan the syntax children of that ast::TraitDef, get ItemList and compare the difference with the ItemList returned from our ast::ImplBlock? (Excuse the types and paths everywhere, still trying to build a mental model of everything here :smiley: )

matklad (Mar 06 2019 at 17:04, on Zulip):

Yeah, I think that's a possible approach

matklad (Mar 06 2019 at 17:04, on Zulip):

using syntax is nice, because you can copy-paste function prototype, with argument names and such

matklad (Mar 06 2019 at 17:04, on Zulip):

with hir approach, you'll have to reconstruct the signature yourself

Igor Matuszewski (Mar 06 2019 at 17:05, on Zulip):

right, makes sense

Igor Matuszewski (Mar 06 2019 at 17:05, on Zulip):

Thanks!

matklad (Mar 06 2019 at 17:08, on Zulip):

In other words, for Hir, long-term, we'll need a pretty-printing infra with the logic like "if the source is available, use that; otherwise, synthesize arbitrary names and pretty-print the code".

However, building that infra requires some effort, and we can totally live without it for now :)

Igor Matuszewski (Mar 06 2019 at 17:16, on Zulip):

yeah, figured that might be the case

Igor Matuszewski (Mar 06 2019 at 17:16, on Zulip):

Gotta dash now, got to fetching the impl items list for both trait def and the current impl block

Igor Matuszewski (Mar 06 2019 at 17:16, on Zulip):

I'll continue later today :)

matklad (Mar 06 2019 at 17:18, on Zulip):

BTW, here's the code here from IntelliJ: https://github.com/intellij-rust/intellij-rust/blob/master/src/main/kotlin/org/rust/ide/refactoring/implementMembers/impl.kt

Florian Diebold (Mar 06 2019 at 17:23, on Zulip):

we'd want to insert type parameters if you have something like impl Trait<Foo> for MyStruct though, so we won't be able to just use AST...

Florian Diebold (Mar 06 2019 at 17:23, on Zulip):

I mean, for now it's fine I'd say, but it's something to think about

matklad (Mar 06 2019 at 17:24, on Zulip):

Yeah, that the bit of that "elaborate pretty printing"

matklad (Mar 06 2019 at 17:25, on Zulip):

Ideally, hir should not care about parameter names (patterns), and ast should not care about substitutions, and there should be a separate thing to unify the two

Igor Matuszewski (Mar 06 2019 at 23:26, on Zulip):

I'm trying to figure out what node to replace in order to get proper indent

Igor Matuszewski (Mar 06 2019 at 23:26, on Zulip):

and it seems like the last "whitespace" node for the children of ItemList make sense

Igor Matuszewski (Mar 06 2019 at 23:27, on Zulip):

I'd like to dochildren().rev() but it seems that DoubleEndedIterator is not implemented for SyntaxNodeChildren

Igor Matuszewski (Mar 06 2019 at 23:27, on Zulip):

however looking at https://docs.rs/rowan/0.3.3/src/rowan/lib.rs.html#450-453 I think this should be trivial? Or am I missing anything

Igor Matuszewski (Mar 06 2019 at 23:28, on Zulip):

(I guess I might as well do .children().filter_map(ast::Whitespace::cast).last() or similar, just this popped up so figured I'd ask away)

matklad (Mar 07 2019 at 09:22, on Zulip):

I feel like there's no good answer currently for getting the proper indent :)

matklad (Mar 07 2019 at 09:22, on Zulip):

The ideal workflow would be to insert nodes somehow, and than to ask (currently non-existent) formatting infra "re-indent these newly inserted nodes"

matklad (Mar 07 2019 at 09:24, on Zulip):

So, I suggest not to handle indent at all, for the initial implementation: it would still be extremely useful, and one can reforamt the whole file

vipentti (Mar 07 2019 at 09:29, on Zulip):

I think as long as the simple case, where the impl is on the top level of the file with no indentation, works, that should be enough when it comes to indentation. Rest of the indenting should be fixed when we have basic indentation formatter.

vipentti (Mar 07 2019 at 09:30, on Zulip):

e.g.

trait Trait {
    fn thing();
}
struct Foo;
impl Trait for Foo {
    fn thing() {
    }
}
vipentti (Mar 07 2019 at 09:31, on Zulip):

But in the end you can always run rustfmt to properly format it anyway

matklad (Mar 07 2019 at 09:41, on Zulip):

That said, children should definitelly be a double-ended iterator! Could you send a PR to rowan?

Igor Matuszewski (Mar 07 2019 at 15:47, on Zulip):

Posted a PR https://github.com/rust-analyzer/rust-analyzer/pull/947 that hopefully closes this issue :tada:

Igor Matuszewski (Mar 07 2019 at 15:50, on Zulip):

And yeah, I'll send a PR :thumbs_up:

matklad (Mar 07 2019 at 15:51, on Zulip):

yep, reviewing it right now, looks good!

Last update: Nov 12 2019 at 16:20UTC