Page MenuHomePhabricator

[AST] Test RecursiveASTVisitor's current traversal of templates.
Needs ReviewPublic

Authored by sammccall on Feb 24 2022, 9:35 AM.

Details

Reviewers
hokein
nridge
Summary

There are several logical inconsistencies and traps in the current traversal.

This doesn't fix any of them, but

  • describes the general expectations of how templates should be traversed
  • (mostly) exhaustively records current behavior as a baseline for changes
  • lists the apparent bugs in current behavior

See https://github.com/clangd/clangd/issues/1034 for an example of reasonable
code that falls into one of the traps, and some discussion.

Diff Detail

Event Timeline

sammccall created this revision.Feb 24 2022, 9:35 AM
sammccall requested review of this revision.Feb 24 2022, 9:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 24 2022, 9:35 AM
hokein added inline comments.Feb 25 2022, 1:21 AM
clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp
12

I think this is a very useful documentation about the RAV API, instead of staying in the test file, I would suggest moving it to the RAV.h file.

18

Conceptually, an instantiations create a specific {Class, Var}TemplateSpecializationDecl, functionDecl.

template <class T> void foo(T) { }
template void foo<char>(); // Line 2

For the statement on line 2 (called explicit instantiation definition I think)

  • there is no corresponding node in the clang AST, this is a missing feature in clang
  • it creates a FunctionDecl, that is void foo(char) { },

does the "declaration part" in the comment refer to the declaration part of the explicit instantiation definition (I think so), or the declaration part of the FunctionDecl void foo(char) {}? The same question to the definition, my understanding from the comment is the function body of the void foo(char) {}, which is {}.

We describe the expected behavior, but there are some bugs or unimplemented features in clang, which creates discrepancies. I think this is confusing and hard to follow, not sure how to do here (encoding a bunch of FIXMEs here is not good, not mentioning them at all is not good neither, can we mention only critical ones?)

sammccall added inline comments.Feb 25 2022, 4:23 AM
clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp
12

Good idea. I suppose it belongs on shouldVisitTemplateInstantiations().

18

Conceptually, an instantiations create a specific {Class, Var}TemplateSpecializationDecl, functionDecl.

template <class T> void foo(T) { }
template void foo<char>(); // Line 2

For the statement on line 2 (called explicit instantiation definition I think)

  • there is no corresponding node in the clang AST, this is a missing feature in clang
  • it creates a FunctionDecl, that is void foo(char) { },

This is a plausible explanation (for functions and variables, but not classes). I don't think it's the best model - I'd prefer to say that the instantiated decl *corresponds* to the explicit instantiation definition.

In this example there are three decls: the template, the templated decl, and the instantiation.
For functions these are {FunctionTemplateDecl, FunctionDecl, FunctionDecl}.
For variables these are {VarTemplateDecl, VarDecl, VarTemplateSpecializationDecl}
For classes these are {ClassTemplateDecl, CXXRecordDecl, ClassTemplateSpecializationDecl}.
(Sometimes in -dump-ast they appear multiple times, but there are only 3 distinct pointers).

There are inconsistencies here:

  • function specializations don't have a distinct node type. I'm not sure this is terribly significant. (I suspect it's related to the fact they can't be partially specialized).
  • class specializations have locations that point to the explicit instantiation where possible (primary location, template parameter locations etc), and the template body otherwise. Variable and function specializations point at the template always.
  • in -dump-ast output it's inconsistent whether specializations are shown at the top level, under the template, or both. But I don't think there's any significant meaning behind this.

The second inconsistency (locations) is the closest to answering the question "do they correspond"?
Possible answers:

  • specializations should correspond to the explicit instantiations, so function + var are deficient in not reporting the locations
  • specializations should not correspond, and class specializations are buggy in reporting explicit instantiation locations
  • class specializations should correspond to the explicit instantiations, but others not (this seems silly)

I think #1 is the most plausible here, especially given that the specializations do know (getTemplateSpecializationKind()) if they were produced from an explicit instantiation.

does the "declaration part" in the comment refer to the declaration part of the explicit instantiation definition (I think so), or the declaration part of the FunctionDecl void foo(char) {}? The same question to the definition, my understanding from the comment is the function body of the void foo(char) {}, which is {}.

This comment is talking about attributing AST nodes to source code, so it's talking about the FunctionDecl. By "declaration part" I mean the FunctionTypeLoc etc. I'll clarify this.

We describe the expected behavior, but there are some bugs or unimplemented features in clang, which creates discrepancies. I think this is confusing and hard to follow, not sure how to do here (encoding a bunch of FIXMEs here is not good, not mentioning them at all is not good neither, can we mention only critical ones?)

I think we should say "there are some bugs, see FIXMEs below".
The concepts are complicated and I think trying to mix them in with the implementation limitations will make a mess.

sammccall updated this revision to Diff 411640.Feb 26 2022, 1:00 PM

Move the description of shouldVisitTemplateInstantiations() to RAV.

will leave approval to hokein, but LGTM

clang/include/clang/AST/RecursiveASTVisitor.h
172 ↗(On Diff #411640)

nit: stray "of"

clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp
284

Is this a FIXME (body is traversed is an instantiation declaration)?

Thought about this some more, and talked to @hokein offline.

I want to make sure there's consensus on "desired behavior" before documenting it, to avoid too much confusion.
https://discourse.llvm.org/t/template-representation-in-ast-and-rav-with-explicit-instantiations/60606

clang/unittests/Tooling/RecursiveASTVisitorTests/Templates.cpp
284

Possibly - it seems like a body shouldn't even exist, right? But maybe the instantiation is performed anyway if the definition is visible, I don't really know the semantics.

If it does exist, traversing it seems wrong: this is almost certainly a double-traversal of the body if there's also an instantiation definition for the same decl.

I'll look into it...

Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 11:01 PM