Page MenuHomePhabricator

[clangd] Fix implicit template instatiations appearing as topLevelDecls.
ClosedPublic

Authored by jvikstrom on Jul 31 2019, 6:47 AM.

Details

Summary

The parser gives implicit template instantiations to the action's HandleTopLevelDecls callback. This makes semantic highlighting highlight these templated functions multiple times. Fixed by filtering on if a Decl is an implicit function/variable/class instantiation. Also added a testcase to semantic highlighting on this.

Diff Detail

Repository
rL LLVM

Event Timeline

jvikstrom created this revision.Jul 31 2019, 6:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 31 2019, 6:47 AM
ilya-biryukov added inline comments.Jul 31 2019, 9:53 AM
clang-tools-extra/clangd/ClangdUnit.cpp
68 ↗(On Diff #212569)

We also want to skip TSK_ExplicitInstantiationDeclaration and TSK_ExplicitInstantiationDefinition here.
This covers cases like (not sure which one of the two enum values we get, though):

template <class T>
int foo(T) { ... }

template int foo(int); // we'd rather not traverse these, highlightings will run into the same problems.

Semantics I'm describing are roughly similar to isImplicitInstatiation(D) == !isExplicitInstantion(D), where isExplicitInstantiation is taken from CodeComplete.cpp. (If we ignore TSK_Undeclared, which, I believe, should never be encountered in decls passed to HandleTopLevelDecl).

Please extract the helper from code complete and this one into a separate file (e.g. clangd/AST.h) and possibly implement one through the other

hokein added inline comments.Jul 31 2019, 1:33 PM
clang-tools-extra/clangd/ClangdUnit.cpp
68 ↗(On Diff #212569)

Semantics I'm describing are roughly similar to isImplicitInstatiation(D) == !isExplicitInstantion(D),

I think there is a typo here, I believe you mean isImplicitInstantiation(D) == !isExplicitSpecialization(D) (in CodeComplete.cpp, it checks whether a Decl is an explicit specialization).

clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp
219 ↗(On Diff #212569)

instead adding a highlighting test, could we add this testcase to ClangdUnitTests.cpp instead?

ilya-biryukov added inline comments.Aug 1 2019, 5:45 AM
clang-tools-extra/clangd/ClangdUnit.cpp
68 ↗(On Diff #212569)

Yes, there's a typo thanks!

jvikstrom updated this revision to Diff 213041.Aug 2 2019, 6:33 AM

Moved test to ClangdUnitTests.

jvikstrom marked 2 inline comments as done.Aug 2 2019, 6:34 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/ClangdUnit.cpp
68 ↗(On Diff #212569)

Still want me to move this and the helper from CodeComplete to AST.h as it isn't used anywhere else? (esp. when we can't implement them through of each other)

hokein added a comment.Aug 2 2019, 7:09 AM

the change looks good to me, leave the final approval to Ilya as he knows more context.

clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
116 ↗(On Diff #213041)

nit: clang-format, the length seems > 80 column.

jvikstrom updated this revision to Diff 213331.Aug 5 2019, 5:23 AM
jvikstrom marked an inline comment as done.

Formatted.

ilya-biryukov added inline comments.Aug 5 2019, 7:46 AM
clang-tools-extra/clangd/ClangdUnit.cpp
82 ↗(On Diff #213331)

Could we expose the following non-template function instead?

bool isImplicitTemplateInstantiation(NamedDecl *);

So that the users don't need to specify which Decl they are interested in (I believe there's no use for it anyway)

68 ↗(On Diff #212569)

Yes, it's better to share this code between codeComplete and ClangdUnit. It might pop up in more places and it's not trivial.

ilya-biryukov added inline comments.Aug 5 2019, 7:48 AM
clang-tools-extra/clangd/ClangdUnit.cpp
68 ↗(On Diff #212569)

Sorry, I mean share functions from the same place between code complete and clangd unit.
They might not be implementable through each other, but they are very similar and having them in the same place means that we will have easier time keeping them both correct in case we need to change any of them.

jvikstrom updated this revision to Diff 213388.Aug 5 2019, 9:47 AM
jvikstrom marked an inline comment as done.

Move isImplicitTemplateInstantiation and isExplicitTemplateSpecialization, also share implementation.

ilya-biryukov added inline comments.Aug 6 2019, 3:22 AM
clang-tools-extra/clangd/AST.h
86 ↗(On Diff #213388)

Could you add a small comment with an example?

/// Indicates \p D is a template instantiation implicitly generated by the compiler, e.g.
///     template <class T> struct vector {};
///     vector<int> v; // 'vector<int>' is an implicit instantiation
bool isImplicitTemplateInstantiation(const NamedDecl *D);

/// Indicates \p D is an explicit template specialization, e.g.
///   template <class T> struct vector {};
///   template <> struct vector<bool> {}; // <-- explicit specialization
///
/// Note that explicit instantiations are NOT explicit specializations, albeit they look similar.
///   template struct vector<bool>; // <-- explicit instantiation, NOT an explicit specialization.
bool isExplicitTemplateSpecialization(const NamedDecl *D);
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
110 ↗(On Diff #213388)

Could you also check that:

  1. explicit specializations are present
template <>
void f(bool) {}
  1. explicit instantiations are absent
template void f(bool);
  1. partial specializations are present (they should not be affected by this change, but testing those here seems appropriate)
template <class T>
struct vector {};

template <class T>
struct vector<T*> {}; // partial specialization, should be present
  1. template variables and classes are also handled:
template <class T>
T foo = 10; // (!) requires C++17
jvikstrom updated this revision to Diff 213608.Aug 6 2019, 7:20 AM
jvikstrom marked an inline comment as done.

Added tests for making sure explicit specializations, explicit instantiations, partial instantiations, explicit declarations and template variables are in topLevelDecls. Also added a test to SemanticHighlightingTests to make sure that explicit instantiations are being traversed (so they aren't accidentaly removed from topLevelDecls).

Also added comment for the two added functions in AST.h.

jvikstrom marked 2 inline comments as done.Aug 6 2019, 7:20 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
110 ↗(On Diff #213388)

Explicit instantiations are present in topLevelDecls though, otherwise RecursiveASTVisitor would not traverse them (so adding a test to make sure explicit instantiations are included in toplevel).

Also adding a test to SemanticHighlighting to make sure that explicit instantiations are visited in that (is some other RecursiveASTVisitor usage I should add this to instead?)

ilya-biryukov marked an inline comment as done.Aug 6 2019, 7:32 AM

One important comment about somehow distinguishing multiple decls with the same name.

clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
110 ↗(On Diff #213388)

Yes, sorry, I forgot about the results of your investigation yesterday.
Having them in top-level decls seems fine, just wanted to make sure we actually test it.
LG

134 ↗(On Diff #213608)

Also add a full specialization (IIRC, they are represented completely differently in the AST):

template <>
struct V<bool> {};
137 ↗(On Diff #213608)

Also add a partial and a full template specializations for the variable declaration:

template <class T>
int foo<T*> = 0;

template <>
int foo<bool> = 0;
144 ↗(On Diff #213608)

Is there a way to check we are seeing the explicit instantiations? (e.g. by looking at template arguments?)

It's not clear whether multiple DeclNamed("foo") refer to the decls we expect.

jvikstrom updated this revision to Diff 213808.Aug 7 2019, 1:08 AM
jvikstrom marked 4 inline comments as done.

Added more ways to specialize/instantiate templates to test.

jvikstrom added inline comments.Aug 7 2019, 1:08 AM
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
144 ↗(On Diff #213608)

Well in this case all the expressions in the top level should be in topLevelDecls so unless say void f(T) {} somehow starts duplicating while void f(bool) disappears from topLevelDecls they should all be visible.
I could add a gtest matcher that also checks the types but I think that would become a pretty large matcher (would have to handle FunctionDecls with template arguments, VarDecls, TemplateClassSpecializationDecls etc.) and I'm not sure it would really add anything to the tests.

A thing we we could do is shuffle the order of the decls in the test so we never have two decls of the same name after each other (because the matcher cares about the order of the elements). Which should give us pretty high confidence we are getting the correct decls... Or maybe I'm misunderstanding you?

ilya-biryukov added inline comments.Aug 7 2019, 1:31 AM
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
144 ↗(On Diff #213608)

The tests should be easy to read and understand, we should definitely find a way to distinguish decls with different arguments.
Adding a matcher for template args should be easy enough:

AllOf(DeclNamed("f"), WithTemplateArgs("<bool>"))

Implementing the matcher is easy with printTemplateSpecializationArgs from AST.h

jvikstrom updated this revision to Diff 213878.Aug 7 2019, 7:38 AM

Changed tests to also look at template parameters.

ilya-biryukov added inline comments.Aug 7 2019, 7:48 AM
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
85 ↗(On Diff #213878)

Could you wrap the result with < and >, add , as a separator?
To make sure the output format for functions is not different for other decls

91 ↗(On Diff #213878)

NIT: remove braces

97 ↗(On Diff #213878)

NIT: remove braces

186 ↗(On Diff #213878)

NIT: do not match WithTemplateArgs for non-template names (like i and d here)?

188 ↗(On Diff #213878)

NIT: add // FIXME: this should be '<T*>', not '<>'

jvikstrom marked an inline comment as done.Aug 7 2019, 7:53 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
144 ↗(On Diff #213608)

Will fix the partial specialization in a separate CL

jvikstrom marked an inline comment as done.Aug 7 2019, 7:58 AM
jvikstrom added inline comments.
clang-tools-extra/clangd/unittests/ClangdUnitTests.cpp
188 ↗(On Diff #213878)

Already have a fix for it (it's a 2 line fix without the tests, should I just add do it in a separate CL or add it to this?)

ilya-biryukov accepted this revision.Aug 7 2019, 11:00 AM

LGTM, but please print the function template arguments uniformly before landing this (to avoid different forms of outputs inside the same test).

This revision is now accepted and ready to land.Aug 7 2019, 11:00 AM
This revision was automatically updated to reflect the committed changes.
jvikstrom marked 4 inline comments as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 12:20 AM