Add codecompletion from embedded structs
ClosedPublic

Authored by ematirov on Jun 21 2017, 3:11 PM.

Details

Summary

Add declarations from embedded structs to top level struct. See difference on screenshots: worker has embedded struct person and person has embedded struct log.Logger so worker ends up with both person and log.Logger fields and methods.
Before:


After:

Test Plan

Added tests.

Diff Detail

Repository
R59 KDevelop Go
Branch
fixing
Lint
No Linters Available
Unit
No Unit Test Coverage
ematirov created this revision.Jun 21 2017, 3:11 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJun 21 2017, 3:11 PM
brauch edited edge metadata.Jun 25 2017, 5:20 PM

I wonder -- is this the right place for that logic? Does it work in highlighting, for example?

I wonder -- is this the right place for that logic? Does it work in highlighting, for example?

Ouch. You are right. Calling methods / accessing members of embedded structs is not highlighted and is not displayed in "code browser".
Probably it could be handled manually like in that case but... I'm not sure about that, it looks a bit mess.
Can you suggest any other way please? I think that these embedded structs could be treated just as base classes. Is there any standard approach to handle them in KDevelop?

From a first look it might be you can just import the first struct's context into the second one:
second->addImportedParentContext(first)
where first and second are the internalContext()s of the declarations declaring the two stucts. Maybe that already does what you need. Otherwise we can see :)

If I do that inside

if(add)
{
    decl->addBaseClass(base);
}

block and retrieve internal context of base type by

baseClassType.constData()->declaration(topContext())->internalContext()

then it's nullptr.
Same goes for decl->internalContext() there.

After some lookup I found that there are 2 declarations added somehow with same qualifiedIdentifier.
Also, looks like it my added test visitTypeSpec is somehow called twice thus in first call declaration is present once and it doesn't contain internal context and in second call there is 2 declarations where first one still doesn't have internal context and second have one.

I'll look more into that tomorrow. Thank you for your advice!

After some lookup I found that there are 2 declarations added somehow with same qualifiedIdentifier.
Also, looks like it my added test visitTypeSpec is somehow called twice thus in first call declaration is present once and it doesn't contain internal context and in second call there is 2 declarations where first one still doesn't have internal context and second have one.

I will have to look closer later, but typically the builders run twice per pass, and in the second pass the declaration created by the first pass should be *re-used*, i.e. openDeclaration() figures out which declaration was created by the first pass and re-uses that. It uses the scope identifier and the range to do that lookup. It might be that this fails. It definitely fails in some cases -- that is why your completion items sometimes disappear while the popup is visible.

Yeah, looks like there is problem with reusing but not because of 2 runs - visitMethodDeclaration opens new declaration for type :-/
See code and resulting DU-Chain: https://paste.kde.org/pmjuqk7ld (declarations on lines 14 and 15)
I tried to:

  1. Assign internal context correctly;
  2. Reuse declaration inside method declaration;
  3. Add contexts of embedded structs as imported parent context.

but now have some problems with such types as: type mytype int; (some existed tests falls). Currently I'm looking what caused it and how to fix it.

ematirov updated this revision to Diff 15963.Jun 28 2017, 7:43 PM

Updated. Based on https://phabricator.kde.org/D6412 work. Now code highlighting works correctly too.

brauch accepted this revision.Jul 3 2017, 9:40 PM

Can't spot anything obviously wrong -- go for it ;)
Sorry for taking so long!

duchain/builders/declarationbuilder.cpp
277

Is this really necessary? Isn't that what happens by default?

This revision is now accepted and ready to land.Jul 3 2017, 9:40 PM
This revision was automatically updated to reflect the committed changes.
ematirov added inline comments.Jul 4 2017, 6:27 PM
duchain/builders/declarationbuilder.cpp
277

Do you mean that internal context is opened by default? If so, it doesn't happens for me. I'll push as it is, will fix if I didn't understand you correctly

brauch added a comment.Jul 4 2017, 6:29 PM

No, I was thinking that the next block with the openContext(...) should automatically re-use the internalContext() of the declaration if that is indeed the same. In any case, when looking at it like this, don't you need to set the new range on the context? Doesn't that become a problem when you extend the range during editing?

No, I was thinking that the next block with the openContext(...) should automatically re-use the internalContext() of the declaration if that is indeed the same. In any case, when looking at it like this, don't you need to set the new range on the context? Doesn't that become a problem when you extend the range during editing?

I tried and it doesn't re-use it. Re range: "methods" are declared outside class scope \ declaration. So, probably updating the context of struct declaration is struct declaration visiter work. TBH, I am not even sure about that openContext(node, editorFindRange(node, 0)....). It's used in cases where type is just a alias to another type but contains it's own methods. Not sure what range to specify in this case though.

Doesn't that become a problem when you extend the range during editing?

Oh, got it. Yeah, there is some problems with method declaration context if I don't update range. Syntax highlighting works fine but there is problems with code browsers, looks like it cannot find correct context by range. Updating range by currentContext()->setRange(editorFindRange(node, 0)); helps but I'm not sure that it's correct since there could be some other declarations not related to that type between type declaration and method declaration; although, looks like it works.

Thank you for your suggestions!