[clang][ExtractAPI] Refactor ExtractAPIVisitor to make it more extensible

[dang]

Summary

Use CRTP to enable creating statically dispatched subclasses of
ExtractAPIVisitor.
This enables adding extension points and customising the behavior more
easily.

This is used in CXExtractAPI.cpp to create a specialized visitor for
Libclang as well as streamlining the batch implementation in ExtractAPIConsumer.cpp

[clang][ExtractAPI] Improve tests for clang_getSymbolGraphForCursor

Adds a new mode to c-index-test that can fetch a single symbol symbol
graph for a given source location. This way we can be more precise when
writing tests for clang_getSymbolGraphForCursor.
Additionaly this makes it easier to debug the function.

[bnbarham]

clang/lib/ExtractAPI/ExtractAPIConsumer.cpp

170 ↗

(On Diff #507469)

Accidental?

clang/test/Index/extract-api-cursor.m

36–120

I personally like to put the run lines next to what's being checked and use [[@LINE+1]]. Up to you though

[zixuw]

Good to see refactoring to make ExtractAPI easier to extend and use. Thanks Daniel!
Some comments on the ExtractAPIVisitor part. I'll leave the libclang part to the experts.

clang/include/clang/ExtractAPI/ExtractAPIVisitor.h

32 ↗

(On Diff #507469)

Would it be better to call this ExtractAPIVisitorImpl because it provides the visitor implementation, and the ExtractAPIVisitor is actually the base for the second level CRTP for actual visitors?

32 ↗

(On Diff #507469)

Should this be : public RecursiveASTVisitor<ExtractAPIVisitorBase<Derived>> instead?

34 ↗

(On Diff #507469)

Should the constructor be made protected for a CRTP base?

57 ↗

(On Diff #507469)

Why does comment-fetching need to be dispatched?

102 ↗

(On Diff #507469)

I see that the RecursiveASTVisitor already provides a getDerived() for the top-level CRTP.
My head is still bending trying to get a clear view of the multi-level CRTP here 🙃 , but I'm guessing that this is to avoid the conflict with that method.
In that case could we be more verbose to make clear the purpose of this one? I'm thinking something like getDerivedExtractAPIVisitor(), to communicate that this is for getting the derived/concrete ExtractAPIVisitor subclass.

clang/lib/ExtractAPI/ExtractAPIConsumer.cpp

170 ↗

(On Diff #507469)

Missed change?

228–234 ↗

(On Diff #507469)

Couldn't find the original logic in this patch. Could you remind me what are these for? Also good to have more comments here to explain things.

[zixuw]

clang/include/clang/ExtractAPI/ExtractAPIVisitor.h

628 ↗

(On Diff #507469)

Same as above, should the constructor be protected? I guess it depends if we want people to just instantiate and use a ExtractAPIVisitor.

[dang]

clang/include/clang/ExtractAPI/ExtractAPIVisitor.h

32 ↗

(On Diff #507469)

I chose to name it "Base" as this seems to be the convention used for ADT, where they use the "Base" suffix for types clients shouldn't be using.

As with the base class, we need to provide RecursiveASTVisitor the most derived class so that when it does the getDerived() dance it can use the most derived methods.

34 ↗

(On Diff #507469)

agreed

57 ↗

(On Diff #507469)

This is for the libclang use case. If you request symbol graph data for a symbol in an implementation block for example you should get back the doc comment from the header. The main reason for doing this refactoring is so that we can avoid dynamic dispatch when doing source location lookups and now fetching documentation comments.

102 ↗

(On Diff #507469)

Not fully sure myself, but I think that using the one from RecursiveASTVisitor would work fine here. I will give it a go and change it if it works.

628 ↗

(On Diff #507469)

The aim was for clients to be able to instantiate this one, hence why we go through a fair amount of effort to provide base class the most derived type with the std::conditional_t usage above.

clang/lib/ExtractAPI/ExtractAPIConsumer.cpp

170 ↗

(On Diff #507469)

yup definitely accidental will fix.

228–234 ↗

(On Diff #507469)

I moved this behavior out of the base type into here, we used to do these checks in the individual VisitXXX methods and bail early, but I needed this behavior to be configurable for the libclang work. I figured it semantically belonged here.

clang/test/Index/extract-api-cursor.m

36–120

Oh I didn't know about this that sounds like it's definitely more readable, will make the swap.

[dang]

Addressing code review feedback

[dang]

Adding back missing diffs.

[zixuw]

LGTM for the ExtractAPIVisitor part.
Remaining:

• update test with @LINE
• the libclang side

[dang]

In D146656#4220022, @zixuw wrote:

LGTM for the ExtractAPIVisitor part.
Remaining:

• update test with @LINE
• the libclang side

I have decided against doing that, because we can't specify @LINE in the c-index-test command line.
@bnbarham are you happy with the libclang bits?

clang/include/clang/ExtractAPI/ExtractAPIVisitor.h

102 ↗

(On Diff #507469)

turns out it is needed but not sure why, I will rename to make things clearer.

clang/test/Index/extract-api-cursor.m

36–120

I tried it out and I actually struggled to follow along so I am leaving it as is.

[bnbarham]

I have decided against doing that, because we can't specify @LINE in the c-index-test command line.

FWIW you can do:

// RUN: .... -pos=%(line+1):7
let bar = Bar()
// CHECK ... [[@LINE-1]]:7

But I don't think this is particularly common, it's just how I write tests. What you have is perfectly reasonable 👍

clang/test/Index/extract-api-cursor.m

+// Test is line- and column-sensitive. Run lines are below
+
 /// Foo docs
 struct Foo {
     /// Bar docs
     int bar;
 };
 
 /// Base docs
 @interface Base

 @end
 
 /// Derived docs
 @interface Derived: Base
 /// Derived method docs
 - (void)derivedMethodWithValue:(id<Protocol>)value;
 @end
 
-/// This won't show up in docs because we can't serialize it
-@interface Derived ()
-/// Derived method in category docs, won't show up either.
-- (void)derivedMethodInCategory;
+@implementation Derived
+- (void)derivedMethodWithValue:(id<Protocol>)value {
+    int a = 5;
+}
 @end
 
-// RUN: c-index-test -single-symbol-sgfs local %s | FileCheck %s
-
-// Checking for Foo
-// CHECK: "parentContexts":[]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[]
-// CHECK-SAME: "text":"Foo docs"
-// CHECK-SAME: "kind":{"displayName":"Structure","identifier":"objective-c.struct"}
-// CHECK-SAME: "title":"Foo"
-
-// Checking for bar
-// CHECK-NEXT: "parentContexts":[{"kind":"objective-c.struct","name":"Foo","usr":"c:@S@Foo"}]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[{"kind":"memberOf","source":"c:@S@Foo@FI@bar","target":"c:@S@Foo"
-// CHECK-SAME: "text":"Bar docs"
-// CHECK-SAME: "kind":{"displayName":"Instance Property","identifier":"objective-c.property"}
-// CHECK-SAME: "title":"bar"
-
-// Checking for Base
-// CHECK-NEXT: "parentContexts":[]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[]
-// CHECK-SAME: "text":"Base docs"
-// CHECK-SAME: "kind":{"displayName":"Class","identifier":"objective-c.class"}
-// CHECK-SAME: "title":"Base"
-
-// Checking for baseProperty
-// CHECK-NEXT: "parentContexts":[{"kind":"objective-c.class","name":"Base","usr":"c:objc(cs)Base"}]
-// CHECK-SAME: "relatedSymbols":[{"accessLevel":"public","declarationLanguage":"objective-c"
-// CHECK-SAME: "isSystem":false
-// CHECK-SAME: "usr":"c:@S@Foo"}]
-// CHECK-SAME: "relationships":[{"kind":"memberOf","source":"c:objc(cs)Base(py)baseProperty","target":"c:objc(cs)Base"
-// CHECK-SAME: "text":"Base property docs"
-// CHECK-SAME: "kind":{"displayName":"Instance Property","identifier":"objective-c.property"}
-// CHECK-SAME: "title":"baseProperty"
-
-// Checking for baseMethodWithArg
-// CHECK-NEXT: "parentContexts":[{"kind":"objective-c.class","name":"Base","usr":"c:objc(cs)Base"}]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[{"kind":"memberOf","source":"c:objc(cs)Base(im)baseMethodWithArg:","target":"c:objc(cs)Base"
-// CHECK-SAME: "text":"Base method docs"
-// CHECK-SAME: "kind":{"displayName":"Instance Method","identifier":"objective-c.method"}
-// CHECK-SAME: "title":"baseMethodWithArg:"
-
-// Checking for Protocol
-// CHECK-NEXT: "parentContexts":[]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[]
-// CHECK-SAME: "text":"Protocol docs"
-// CHECK-SAME: "kind":{"displayName":"Protocol","identifier":"objective-c.protocol"}
-// CHECK-SAME: "title":"Protocol"
-
-// Checking for protocolProperty
-// CHECK-NEXT: "parentContexts":[{"kind":"objective-c.protocol","name":"Protocol","usr":"c:objc(pl)Protocol"}]
-// CHECK-SAME: "relatedSymbols":[{"accessLevel":"public","declarationLanguage":"objective-c"
-// CHECK-SAME: "isSystem":false
-// CHECK-SAME: "usr":"c:@S@Foo"}]
-// CHECK-SAME: "relationships":[{"kind":"memberOf","source":"c:objc(pl)Protocol(py)protocolProperty","target":"c:objc(pl)Protocol"
-// CHECK-SAME: "text":"Protocol property docs"
-// CHECK-SAME: "kind":{"displayName":"Instance Property","identifier":"objective-c.property"}
-// CHECK-SAME: "title":"protocolProperty"
-
-// Checking for Derived
-// CHECK-NEXT: "parentContexts":[]
-// CHECK-SAME: "relatedSymbols":[{"accessLevel":"public","declarationLanguage":"objective-c"
-// CHECK-SAME: "isSystem":false
-// CHECK-SAME: "usr":"c:objc(cs)Base"}]
-// CHECK-SAME: "relationships":[{"kind":"inheritsFrom","source":"c:objc(cs)Derived","target":"c:objc(cs)Base"
-// CHECK-SAME: "text":"Derived docs"
-// CHECK-SAME: "kind":{"displayName":"Class","identifier":"objective-c.class"}
-// CHECK-SAME: "title":"Derived"
-
-// Checking for derivedMethodWithValue
-// CHECK-NEXT: "parentContexts":[{"kind":"objective-c.class","name":"Derived","usr":"c:objc(cs)Derived"}]
-// CHECK-SAME: "relatedSymbols":[]
-// CHECK-SAME: "relationships":[{"kind":"memberOf","source":"c:objc(cs)Derived(im)derivedMethodWithValue:","target":"c:objc(cs)Derived"
-// CHECK-SAME: "text":"Derived method docs"
-// CHECK-SAME: "kind":{"displayName":"Instance Method","identifier":"objective-c.method"}
-// CHECK-SAME: "title":"derivedMethodWithValue:"
-
-// CHECK-NOT: This won't show up in docs because we can't serialize it
-// CHECK-NOT: Derived method in category docs, won't show up either.
+// RUN: c-index-test -single-symbol-sgf-at=%s:4:9 local %s | FileCheck -check-prefix=CHECK-FOO %s
+// CHECK-FOO: "parentContexts":[]
+// CHECK-FOO: "relatedSymbols":[]
+// CHECK-FOO: "relationships":[]
+// CHECK-FOO: "text":"Foo docs"
+// CHECK-FOO: "kind":{"displayName":"Structure","identifier":"objective-c.struct"}
+// CHECK-FOO: "title":"Foo"
+
+// RUN: c-index-test -single-symbol-sgf-at=%s:6:9 local %s | FileCheck -check-prefix=CHECK-BAR %s
+// CHECK-BAR: "parentContexts":[{"kind":"objective-c.struct","name":"Foo","usr":"c:@S@Foo"}]
+// CHECK-BAR: "relatedSymbols":[]
+// CHECK-BAR: "relationships":[{"kind":"memberOf","source":"c:@S@Foo@FI@bar","target":"c:@S@Foo"
+// CHECK-BAR: "text":"Bar docs"
+// CHECK-BAR: "kind":{"displayName":"Instance Property","identifier":"objective-c.property"}
+// CHECK-BAR: "title":"bar"
+
+// RUN: c-index-test -single-symbol-sgf-at=%s:10:11 local %s | FileCheck -check-prefix=CHECK-BASE %s
+// CHECK-BASE: "parentContexts":[]
+// CHECK-BASE: "relatedSymbols":[]
+// CHECK-BASE: "relationships":[]
+// CHECK-BASE: "text":"Base docs"
+// CHECK-BASE: "kind":{"displayName":"Class","identifier":"objective-c.class"}
+// CHECK-BASE: "title":"Base"
+
+// RUN: c-index-test -single-symbol-sgf-at=%s:12:25 local %s | FileCheck -check-prefix=CHECK-BASE-PROP %s
+// CHECK-BASE-PROP: "parentContexts":[{"kind":"objective-c.class","name":"Base","usr":"c:objc(cs)Base"}]
+// CHECK-BASE-PROP: "relatedSymbols":[{"accessLevel":"public","declarationLanguage":"objective-c"
+// CHECK-BASE-PROP: "isSystem":false
+// CHECK-BASE-PROP: "usr":"c:@S@Foo"}]
+// CHECK-BASE-PROP: "relationships":[{"kind":"memberOf","source":"c:objc(cs)Base(py)baseProperty","target":"c:objc(cs)Base"
+// CHECK-BASE-PROP: "text":"Base property docs"
+// CHECK-BASE-PROP: "kind":{"displayName":"Instance Property","identifier":"objective-c.property"}
+// CHECK-BASE-PROP: "title":"baseProperty"
+
+// RUN: c-index-test -single-symbol-sgf-at=%s:15:9 local %s | FileCheck -check-prefix=CHECK-BASE-METHOD %s
+// CHECK-BASE-METHOD: "parentContexts":[{"kind":"objective-c.class","name":"Base","usr":"c:objc(cs)Base"}]
+// CHECK-BASE-METHOD: "relatedSymbols":[]
+// CHECK-BASE-METHOD: "relationships":[{"kind":"memberOf","source":"c:objc(cs)Base(im)baseMethodWithArg:","target":"c:objc(cs)Base"
+// CHECK-BASE-METHOD: "text":"Base method docs"
+// CHECK-BASE-METHOD: "kind":{"displayName":"Instance Method","identifier":"objective-c.method"}
+// CHECK-BASE-METHOD: "title":"baseMethodWithArg:"
+
+// RUN: c-index-test -single-symbol-sgf-at=%s:19:11 local %s | FileCheck -check-prefix=CHECK-PROTOCOL %s
+// CHECK-PROTOCOL: "parentContexts":[]
+// CHECK-PROTOCOL: "relatedSymbols":[]
+// CHECK-PROTOCOL: "relationships":[]
+// CHECK-PROTOCOL: "text":"Protocol docs"
+// CHECK-PROTOCOL: "kind":{"displayName":"Protocol","identifier":"objective-c.protocol"}
+// CHECK-PROTOCOL: "title":"Protocol"
+
+// RUN: c-index-test -single-symbol-sgf-at=%s:21:27 local %s | FileCheck -check-prefix=CHECK-PROTOCOL-PROP %s
+// CHECK-PROTOCOL-PROP: "parentContexts":[{"kind":"objective-c.protocol","name":"Protocol","usr":"c:objc(pl)Protocol"}]
+// CHECK-PROTOCOL-PROP: "relatedSymbols":[{"accessLevel":"public","declarationLanguage":"objective-c"
+// CHECK-PROTOCOL-PROP: "isSystem":false
+// CHECK-PROTOCOL-PROP: "usr":"c:@S@Foo"}]
+// CHECK-PROTOCOL-PROP: "relationships":[{"kind":"memberOf","source":"c:objc(pl)Protocol(py)protocolProperty","target":"c:objc(pl)Protocol"
+// CHECK-PROTOCOL-PROP: "text":"Protocol property docs"
+// CHECK-PROTOCOL-PROP: "kind":{"displayName":"Instance Property","identifier":"objective-c.property"}
+// CHECK-PROTOCOL-PROP: "title":"protocolProperty"
+
+// RUN: c-index-test -single-symbol-sgf-at=%s:25:15 local %s | FileCheck -check-prefix=CHECK-DERIVED %s
+// CHECK-DERIVED: "parentContexts":[]
+// CHECK-DERIVED: "relatedSymbols":[{"accessLevel":"public","declarationLanguage":"objective-c"
+// CHECK-DERIVED: "isSystem":false
+// CHECK-DERIVED: "usr":"c:objc(cs)Base"}]
+// CHECK-DERIVED: "relationships":[{"kind":"inheritsFrom","source":"c:objc(cs)Derived","target":"c:objc(cs)Base"
+// CHECK-DERIVED: "text":"Derived docs"
+// CHECK-DERIVED: "kind":{"displayName":"Class","identifier":"objective-c.class"}
+// CHECK-DERIVED: "title":"Derived"
+
+// RUN: c-index-test -single-symbol-sgf-at=%s:27:11 local %s | FileCheck -check-prefix=CHECK-DERIVED-METHOD %s
+// CHECK-DERIVED-METHOD: "parentContexts":[{"kind":"objective-c.class","name":"Derived","usr":"c:objc(cs)Derived"}]
+// CHECK-DERIVED-METHOD: "relatedSymbols":[]
+// CHECK-DERIVED-METHOD: "relationships":[{"kind":"memberOf","source":"c:objc(cs)Derived(im)derivedMethodWithValue:","target":"c:objc(cs)Derived"
+// CHECK-DERIVED-METHOD: "text":"Derived method docs"
+// CHECK-DERIVED-METHOD: "kind":{"displayName":"Instance Method","identifier":"objective-c.method"}
+// CHECK-DERIVED-METHOD: "title":"derivedMethodWithValue:"
+
+// RUN: c-index-test -single-symbol-sgf-at=%s:31:11 local %s | FileCheck -check-prefix=CHECK-DERIVED-METHOD-IMPL %s
+// CHECK-DERIVED-METHOD-IMPL: "parentContexts":[{"kind":"objective-c.class","name":"Derived","usr":"c:objc(cs)Derived"}]
+// CHECK-DERIVED-METHOD-IMPL: "relatedSymbols":[]
+// CHECK-DERIVED-METHOD-IMPL: "relationships":[{"kind":"memberOf","source":"c:objc(cs)Derived(im)derivedMethodWithValue:","target":"c:objc(cs)Derived"
+// CHECK-DERIVED-METHOD-IMPL: "text":"Derived method docs"
+// CHECK-DERIVED-METHOD-IMPL: "kind":{"displayName":"Instance Method","identifier":"objective-c.method"}
+// CHECK-DERIVED-METHOD-IMPL: "title":"derivedMethodWithValue:"

clang/tools/c-index-test/c-index-test.c

 /* c-index-test.c */
 
 #include "clang-c/BuildSystem.h"
 #include "clang-c/CXCompilationDatabase.h"
 #include "clang-c/CXErrorCode.h"
+#include "clang-c/CXSourceLocation.h"
 #include "clang-c/CXString.h"
 #include "clang-c/Documentation.h"
 #include "clang-c/Index.h"
 #include "clang/Config/config.h"
 #include <assert.h>
 #include <ctype.h>
 #include <stdio.h>
 #include <stdlib.h>
dispose_tu:
   clang_disposeTranslationUnit(TU);
 free_remapped_files:
   free_remapped_files(unsaved_files, num_unsaved_files);
 dispose_index:
   clang_disposeIndex(Idx);
   return result;
 }
 
+static void inspect_single_symbol_sgf_cursor(CXCursor Cursor) {
+  CXSourceLocation CursorLoc;
+  CXString SGFData;
+  const char *SGF;
+  unsigned line, column;
+  CursorLoc = clang_getCursorLocation(Cursor);
+  clang_getSpellingLocation(CursorLoc, 0, &line, &column, 0);
+  printf("%d:%d: ", line, column);
+
+  SGFData = clang_getSymbolGraphForCursor(Cursor);
+  SGF = clang_getCString(SGFData);
+  if (SGF)
+    printf("%s\n", SGF);
+}
+
 /******************************************************************************/
 /* Command line processing.                                                   */
 /******************************************************************************/
 
 static CXCursorVisitor GetVisitor(const char *s) {
   if (s[0] == '\0')
     return FilteredPrintingVisitor;
   if (strcmp(s, "-usrs") == 0)
fprintf(stderr,
     "       c-index-test -test-print-type-size {<args>}*\n"
     "       c-index-test -test-print-bitwidth {<args>}*\n"
     "       c-index-test -test-print-target-info {<args>}*\n"
     "       c-index-test -test-print-type-declaration {<args>}*\n"
     "       c-index-test -print-usr [<CursorKind> {<args>}]*\n"
     "       c-index-test -print-usr-file <file>\n");
   fprintf(stderr,
           "       c-index-test -single-symbol-sgfs <symbol filter> {<args>*}\n"
+          "       c-index-test -single-symbol-sgf-at=<site> {<args>*}\n"
           "       c-index-test -single-symbol-sgf-for=<usr> {<args>}*\n");
   fprintf(stderr,
     "       c-index-test -write-pch <file> <compiler arguments>\n"
     "       c-index-test -compilation-db [lookup <filename>] database\n");
   fprintf(stderr,
     "       c-index-test -print-build-session-timestamp\n");
   fprintf(stderr,
     "       c-index-test -read-diagnostics <file>\n\n");
else if (argc > 2 && strcmp(argv[1], "-write-pch") == 0)
     return write_pch_file(argv[2], argc - 3, argv + 3);
   else if (argc > 2 && strcmp(argv[1], "-compilation-db") == 0)
     return perform_test_compilation_db(argv[argc-1], argc - 3, argv + 2);
   else if (argc == 2 && strcmp(argv[1], "-print-build-session-timestamp") == 0)
     return perform_print_build_session_timestamp();
   else if (argc > 3 && strcmp(argv[1], "-single-symbol-sgfs") == 0)
     return perform_test_load_source(argc - 3, argv + 3, argv[2],
                                     PrintSingleSymbolSGFs, NULL);
+  else if (argc > 2 && strstr(argv[1], "-single-symbol-sgf-at=") == argv[1])
+    return inspect_cursor_at(
+        argc, argv, "-single-symbol-sgf-at=", inspect_single_symbol_sgf_cursor);
   else if (argc > 2 && strstr(argv[1], "-single-symbol-sgf-for=") == argv[1])
     return perform_test_single_symbol_sgf(argv[1], argc - 2, argv + 2);
 
   print_usage();
   return 1;
 }
 
 /***/