Using non-qualified identifiers as DUContext scope IDs

[Milian Wolff]

On Dienstag, 11. April 2017 06:40:49 CEST Nicolás Alvarez wrote:
> Hi,

hey, and sorry once more for the long delay

> This is a continuation of my previous email about openDeclaration.
> 
> AbstractContextBuilder::openContext also takes a QualifiedIdentifier,
> where it seems like taking a single Identifier would make more sense.
> However, while a Declaration stores an Identifier (and openDeclaration
> uses the last Identifier from a QID), a DUContext actually stores a
> QualifiedIdentifier as its "localScopeIdentifier", so it's a tricker
> situation. We need to look at that property too.
> 
> I think localScopeIdentifier should be an Identifier too. The full
> scopeIdentifier() of a DUContext is formed by walking the parent
> contexts and concatenating their Identifiers, so I don't see why each
> local scope ID should be a QID. I think if you have multiple
> components in a QID, you should have multiple nested contexts too.

Sounds good so far.

> The problem is that kdev-clang often creates DUContexts with
> multi-component QIDs (ns::class or ns1::ns2::class), as I verified by
> adding an assertion. Interestingly, many of these came from
> kdev-clang's Visitor::createContext, which has this lovely comment:
> 
> "// wtf: why is the DUContext API requesting a QID when it needs a plain
> Id?!"

Hehe :)

> Making things even more interesting, I looked at a few cases where
> kdev-clang was creating DUContexts with multi-component QIDs as scope
> identifiers, and I found they are *wrong*. Here is an example
> testcase:
> 
> namespace ns{} // necessary, don't know why
> 
> namespace ns {
>     class vec {
>     public:
>         void reserve(int n);
>     };
>     void vec::reserve(int n) {
>         int x=12;
>         x+=2; //avoid "unused variable" warning
>     }
> }
> 
> At some point this will create the internal context for the reserve()
> definition (at least that's what I think it is), with
> localScopeIdentifier="ns::vec" (ie. a QualifiedIdentifier with
> [Identifier("ns"), Identifier("vec")]). Its parent context is the
> 'foo' namespace. The full scopeIdentifier() of the context is thus
> "ns::ns::vec", which is clearly wrong. This is even visible in the
> GUI! The tooltip of the 'x' variable says "Scope:
> ns::ns::vec::reserve".

Nice catch, and your cleanup above should fix it - right?

> This is clearly a bug, but I think it's not *just* a concrete bug in
> kdev-clang that we fix and move on. I think there is *no* case where
> it would make sense for a DUContext to have multiple ID components as
> its scope QID. You should just create multiple nested contexts and
> assign each a single identifier.

Yes this sounds fair to me. Just make sure that nothing breaks for something 
like

namespace Foo { class B
class Foo::Bar {};

In this case we will have to open first the Foo ctx and then the Bar ctx. 

> I propose:
> 1. add a warning or assertion in DUContext to ensure id.count() <= 1
> 2. fix problems in kdev-clang that trigger the above warning or
> assertion (and maybe in other language plugins; I didn't try)
> 3. now that everyone passes a single-component QID, change DUContext
> to store an Identifier instead of a QualifiedIdentifier, and change
> APIs that create contexts accordingly

You forgot

0. add unit tests for the breakages you found and the case mention above

otherwise this sounds like a very good idea

Thanks!

-- 
Milian Wolff
mail at milianw.de
http://milianw.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20170420/883a226e/attachment-0001.sig>