Type Alias Template fix
ClosedPublic

Authored by craigt on Sep 13 2017, 12:12 PM.

Details

Summary

Since Clang 3.8, libclang began exposing an CXCursor_TypeAliasTemplateDecl cursor as a result of work done by a KDevelop developer. Ironically, this feature was never actually integrated into KDevelop's clang support, and because of how cursors are dispatched in clang's DUChain builder, the default action is to recurse into any cursor enumeration not explicitly listed. This, in turn, recurses into CXCursor_TypeAliasTemplateDecl, and while that does eventually pick up a child CXCursor_TypeAliasDecl cursor, ensuring that the declaration is added to the DUChain, the foremost descendants of CXCursor_TypeAliasTemplateDecl tend to be template parameters, effectively leaking these parameters into the surrounding context.

Moreover, since the addition of CXCursorTypeAliasTemplateDecl, references from CXCursor_TemplateRef point directly to these cursors which, unfortunately, have a spelling range which covers the using keyword that introduces the name which will refer to this alias, but not the name itself. Additionally, unlike typedef cursors or CXCursor_TypeAliasDecl, CXCursor_TypeAliasTemplateDecl yields no useful information about the type it is aliasing; interestingly, CXCursor_TypeAliasDecl provides this information without fault, even if it refers to templates with template parameters. Ultimately, this means that the actual CXCursor_TypeAliasTemplateDecl cursor, for the moment, is good for little more than signalling the beginning of a new context to capture template parameters and possibly new types, and the actual, declarative unit is still a CXCursor_TypeAliasDecl cursor. Sadly, this means the procedure for building uses from cursors needs to be tweaked so lookups will make the correct association (they presently do not). Ultimately, the real fix should be in investigating the upstream implementation of CXCursor_TypeAliasTemplateDecl to see if it can be made to function more like a traditional declaration cursor.

BUG: 384580
FIXED-IN: 5.2

Test Plan

Modified extant testTypeAliasTemplate test in test_duchain-clang to verify this patch works as expected, namely, that it doesn't leak template parameters into the surrounding context, and that uses of the type alias template are correctly tracked in the DUChain. If you simply apply the changes to test_duchain.cpp on their own, the breakage of the current code model becomes apparent.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
craigt created this revision.Sep 13 2017, 12:12 PM
craigt edited the summary of this revision. (Show Details)Sep 13 2017, 12:17 PM
apol added a subscriber: apol.Sep 13 2017, 1:03 PM

Cool!
Can we have a unit test too? It shouldn't be hard to adapt one of the current ones.

In D7799#145435, @apol wrote:

Cool!
Can we have a unit test too? It shouldn't be hard to adapt one of the current ones.

In fact, I'm going to make some alterations to the extant testTypeAliasTemplate in test_duchain-clang; I somehow missed this when I was looking through the source file earlier.

mwolff requested changes to this revision.Sep 13 2017, 1:24 PM

overall lgtm, I'd also like to see some unit test coverage please

plugins/clang/duchain/builder.cpp
76

Please remove the get prefix. Just call it embeddedTypeAlias or use a find prefix

78

Just to ensure it's properly initialized, I'd write:

auto result = clang_getNullCursor();
80

move this into the if branch below, it's only used there

This revision now requires changes to proceed.Sep 13 2017, 1:24 PM
craigt updated this revision to Diff 19508.Sep 14 2017, 8:27 AM
craigt edited edge metadata.
craigt edited the test plan for this revision. (Show Details)

Addressed milian's comments, modified unit tests to provide better coverage.

mwolff accepted this revision.Sep 14 2017, 11:05 AM

thanks, lgtm - do you have commit rights?

This revision is now accepted and ready to land.Sep 14 2017, 11:05 AM

Nope, could you please commit this for me?

This revision was automatically updated to reflect the committed changes.