Add a call super attribute plugin example

[psionic12]

Summary

If a virtual method is marked as call_super, the
override method must call it, simpler feature like @CallSuper
in Android Java.

[psionic12]

add final attribute checking

[psionic12]

clang/test/Frontend/plugin-call-super.cpp

8

If I remove the keyword "virtual" here, the isVirtual() will return false.

To my knowledge "final" means this method cannot be overridden, but it is still a virtual method, right?

[aaron.ballman]

clang/docs/ClangPlugins.rst

117 ↗

(On Diff #303772)

The number of underlines here looks off -- can you verify it's correct?

119 ↗

(On Diff #303772)

Should add backticks around call_super since it's syntax. Also, sub-classes should be subclasses.

"call it in overridden the method" -> "call it in the overridden method"

119 ↗

(On Diff #303772)

There should be more explanation here about what concepts the example demonstrates. For example, one interesting bit to me is that it shows how you can add a custom attribute that's useful even if it does not generate an AST node for the attribute.

clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp

26

Comments should be grammatical including starting with a capital letter and trailing punctuation. (Should be corrected elsewhere in the patch as well.)

33

Is the functionality for getting names out of a NamedDecl insufficient?

44

ctor should probably be explicit

44

Can drop clang:: prefixed everywhere since you have a using declaration at the top of the file.

64

Should put single quotes around call_super as it's a syntactic element.

I'm not certain that %0 and %1 are necessary because the overridden methods will have the same names. I think you can drop the %1 and rely on the note to point out where the overridden method lives.

67

Single quotes around call_super here as well; we usually phrase this sort of thing as "overridden method marked 'call_super' here

74

Single quotes around call_super and we should use consistent terminology for "attached" vs "marked".

77

Won't this visit every method declaration multiple times? Once for the declaration itself (as we encounter it) and once for each time it's listed as an overridden method?

81

We typically elide the braces for single-line substatements.

91

FWIW, you can pass Overridden and MethodDecl in directly rather than trying to scrape the name out yourself -- the diagnostics engine knows how to properly turn a NamedDecl into a string for diagnostics.

136

For the C++11 spelling, it should be in the clang attribute namespace rather than the standard attribute namespace.

151

Single quotes around the attribute name.

191

Can you add a newline to the end of the file?

clang/test/Frontend/plugin-call-super.cpp

8

Only virtual functions can be marked final, so yes, it's still a virtual function. isVirtual() tells you whether the user wrote virtual themselves, but I'm not certain why isVirtual() would return false there -- the function overrides a method, so I would have expected this to return true: https://github.com/llvm/llvm-project/blob/master/clang/include/clang/AST/DeclCXX.h#L2023

[psionic12]

clang/docs/ClangPlugins.rst

119 ↗

(On Diff #303772)

"how you can add a custom attribute that's useful even if it does not generate an AST node for the attribute", do you mean I should add an Annotation Attribute object even I don't use it? So that when someone dumps the AST, the call_super attribute will show?

Or do you mean to explain the inner implementation of how could the RecursiveASTVisitor knows the declaration is marked as call_super, even if I didn't add any extra attribute nodes to this declaration node?

clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp

33

fullName() here is used to get a string which format is "class_name::method_name", I think NamedDecl::getName or NamedDecl::getNameAsString only gets the method name, without the class name.

Or is there an easy way to accomplish this?

64

About the '%0' and '%1' problem, same as the comment about "fullName()" function, since the overridden methods have the same name, I want use "class_name::method_name" to distinguish between these methods to make users clear.

It would very nice if you could help to come up with a decent warning message which is better than this, I tried some but none of them give a compiler-warning-style feeling...

91

Same problem, a decent warning message is welcome, I don't like this scrape-name-thing either.

[psionic12]

clang/docs/ClangPlugins.rst

119 ↗

(On Diff #303772)

I got you point, please ignore the comment above.

Since this is an example, I should mention more about this example itself rather than how to use this plugin, right?

[psionic12]

Add more details in ClangPlugins.rst.
Re-design overridden usage checking.
Fix some code style issues.

[psionic12]

clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp

134

AddAfterMainAction will get run after IR codes were generated, I found this when writing test cases, so it would be better to use AddBeforeMainAction

[aaron.ballman]

I have more review to do on this but have to run for a while and wanted to get you this feedback sooner rather than later.

clang/docs/ClangPlugins.rst

119 ↗

(On Diff #304129)

Sorry, I was unclear, it should be *double* backticks around syntax elements like call_super, not single backticks.

122–123 ↗

(On Diff #304129)

Slight tweaking for grammar:

This example shows that attribute plugins combined with PluginASTAction in Clang can do some of the same things which Java Annotations do.

(with double backticks around PluginASTAction)

125 ↗

(On Diff #304129)

Slight tweaking for grammar:

Unlike the other attribute plugin examples, this one does not attach an attribute AST node to the declaration AST node. Instead, it keeps a separate list of attributed declarations, which may be faster than using Decl::getAttr<T>() in some cases. The disadvantage of this approach is that the attribute is not part of the AST, which means that dumping the AST will lose the attribute information, pretty printing the AST won't write the attribute back out to source, and AST matchers will not be able to match against the attribute on the declaration.

(with double backticks around Decl::getAttr<T>())

117 ↗

(On Diff #303772)

This still appears to be incorrect and will cause build errors for the documentation.

119 ↗

(On Diff #303772)

Since this is an example, I should mention more about this example itself rather than how to use this plugin, right?

Exactly!

clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp

64

It would very nice if you could help to come up with a decent warning message which is better than this, I tried some but none of them give a compiler-warning-style feeling...

How about:

virtual function %q0 is marked as 'call_super' but this overriding method does not call the base version

and

"function marked 'call_super' here"

91

You can use %q0 to specify that you want a qualified name printed, I believe, which should do what you want with the function name.

[psionic12]

fix grammer errors in warning messages and documents

[psionic12]

clang/docs/ClangPlugins.rst

117 ↗

(On Diff #303772)

Do you mean that there's a command to build the documentation and currently my patch will cause a failure on it?

I thought this ClangPlugins.rst is only an documentation with markdown, but seems that it's not what I thought?

Currently I will make sure there's no build error on the plugin itself and the regression test case, and make sure the
regression test will pass. Seems that's not enough, right?

[psionic12]

I just enabled LLVM_ENABLE_DOXYGEN LLVM_BUILD_DOCS, and use make doxygen-clang, it seems no error with my newest patch, is this

This still appears to be incorrect and will cause build errors for the documentation.

use the same way?

[aaron.ballman]

clang/docs/ClangPlugins.rst

117 ↗

(On Diff #303772)

Do you mean that there's a command to build the documentation and currently my patch will cause a failure on it?

Yes, we have a bot that builds docs: http://lab.llvm.org:8011/#/builders/92

I thought this ClangPlugins.rst is only an documentation with markdown, but seems that it's not what I thought?

It is a documentation file with markdown, but the markdown bots will complain if a markdown file cannot be built (they treat markdown warnings as errors).

Currently I will make sure there's no build error on the plugin itself and the regression test case, and make sure the regression test will pass. Seems that's not enough, right?

Most of the time that's enough because the markdown usage is pretty trivial and can be inspected by sight for obvious issues (so people don't typically have to set up their build environments to generate documentation and test it with every patch).

In this case, the issue is with the underlines under the title. The number of underlines needs to match the number of characters in the title, but in this case there are 20 = characters but 23 title characters.

clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp

10–11

How about: This Clang plugin checks that overrides of the marked virtual function directly call the marked function.

81

supper is used -> the superclass method is called

85

are not get called, warning -> are not called, warn

153

Instead of methods, how about virtual functions? Then the logic can be: if (!TheMethod || !TheMethod->isVirtual()) and you can remove the diagnostic below.

167–169

attr is added) save the address of the Decl in sets -> .attr is added). Save the address of the Decl in a set

clang/test/Frontend/plugin-call-super.cpp

2–3

The way we would typically test something like this is to pass -fsyntax-only and -verify to verify the diagnostics rather than using FileCheck, because the plugin doesn't have any impact on code generation (just diagnostics). This makes the test faster to check (and is a bit easier than using FileCheck).

[psionic12]

Fix the grammar
Simplify the code logic
Simplify the test case

[psionic12]

clang/docs/ClangPlugins.rst

117 ↗

(On Diff #303772)

It seems that only a committee member can trigger this bot, any way I can test on my own environment? So that I can make sure the doc will compile successfully before uploading patches?

As I mentioned before, using make doxygen-clang will succeed even the = characters are not match with title characters, so it seems that the bot doesn't use this way.

clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp

70

Call lateDiagAppertainsToDecl in every VisitCXXMethodDecl functions is not very efficiency, since marked methods are already saved.

105

Move the lateDiagAppertainsToDecl() here and only check marked functions.

[aaron.ballman]

clang/docs/ClangPlugins.rst

117 ↗

(On Diff #303772)

You can test in your own environment (if a build bot can do it, so can anyone else). I don't typically build docs locally so my instructions are untested, but I think you need to set the following cmake variables:

LLVM_BUILD_DOCS=YES ' enables building docs
LLVM_ENABLE_SPHINX=YES ' enable building sphinx docs (.rst files)

You also have to have Sphinx installed (you can do this using pip install -U Sphinx) and on the PATH before you configure cmake for Clang.

Doxygen is a different documentation format than Sphinx. Doxygen handles building the docs from comments in the source files. Sphinx is what turns these .rst files into HTML. If you enable Sphinx builds from cmake there should be a target added that lets you build those docs.

clang/test/Frontend/plugin-call-super.cpp

6

Hmm, this seems wrong to me -- we do expect diagnostics from this file.

19–20

These warnings and notes (and the warning a few lines up) are ones I would have expected to catch using // expected-warning {{virtual function 'Base2::Test' is marked as 'call_super' but this overriding method does not call the base version}} style checks instead of needing to use FileCheck.

Do plugin-based diagnostics not get caught by -verify? I expect this test file to fail as currently written because of the expected-no-diagnostics, but I've not done a whole lot of testing of plugins before.

[psionic12]

use VerifyDiagnosticConsumer (-verify) instead of FileCheck for syntax only feature test.
remove illustration in ClangPlugins.rst (which is not very appropriate)

[psionic12]

clang/docs/ClangPlugins.rst

116 ↗

(On Diff #305405)

After a whole day's research of Sphinx, I figured out that ClangPlugins.rst is the "proto-type" of https://clang.llvm.org/docs/ClangPlugins.html, which is the document on how to use Clang plugin features.

This leads that my change in ClangPlugins.rst are not very appropriate, sort of off-topic.

Sorry I don't notice the rest of the file, and misunderstood it as a documents for examples.

I decide to move the illustration to the source code.

[psionic12]

clang/test/Frontend/plugin-call-super.cpp

19–20

-verify works well with plugins, I just tested, thanks for pointing out this elegant test way for syntax only features.

[aaron.ballman]

clang/docs/ClangPlugins.rst

116 ↗

(On Diff #305405)

You're correct that the .rst file is the prototype for the public HTML documentation, sorry for not making that more clear sooner. I think it's fine to have the documentation either in the header file (as you've switched to) or within this file (since it is showing a slightly different way to define a plugin-based attribute).

clang/test/Frontend/plugin-call-super.cpp

6

Rather than duplicating the test logic like this, you can pass a custom prefix to -verify that gets used when checking the expected results. e.g.,

// RUN: %clang ... -verify=callsuper ...
// RUN: %clang ... -verify=nocallsuper -DBAD_CALLSUPER ...
// callsuper-no-diagnostics

...
void Derive::Test() { // nocallsuper-expected-warning {{virtual function ... }}
  Base1::Test();
#ifndef BAD_CALLSUPER
  Base2::Test();
#endif
}

[psionic12]

Simplify the test case

[aaron.ballman]

LGTM, thank you for the new plugin example! Do you need me to commit on your behalf? If so, are you okay with Yafei Liu <[e-mail]> for attribution?

[psionic12]

@aaron.ballman That would be nice if your could help, and Yafei Liu <[e-mail]> is okay.

[aaron.ballman]

In D91047#2407091, @psionic12 wrote:

@aaron.ballman That would be nice if your could help, and Yafei Liu <[e-mail]> is okay.

Thank you for the new example, I've commit on your behalf in 2ce6352e46344d97fed6a3ca6de7228345956191

clang/examples/CMakeLists.txt

 if(NOT CLANG_BUILD_EXAMPLES)
   set_property(DIRECTORY PROPERTY EXCLUDE_FROM_ALL ON)
   set(EXCLUDE_FROM_ALL ON)
 endif()
 
 add_subdirectory(clang-interpreter)
 add_subdirectory(PrintFunctionNames)
 add_subdirectory(AnnotateFunctions)
 add_subdirectory(Attribute)
+add_subdirectory(CallSuperAttribute)

clang/examples/CallSuperAttribute/CMakeLists.txt

+add_llvm_library(CallSuperAttr MODULE CallSuperAttrInfo.cpp PLUGIN_TOOL clang)
+
+if(LLVM_ENABLE_PLUGINS AND (WIN32 OR CYGWIN))
+  set(LLVM_LINK_COMPONENTS
+    Support
+  )
+  clang_target_link_libraries(CallSuperAttr PRIVATE
+    clangAST
+    clangBasic
+    clangFrontend
+    clangLex
+    )
+endif()

clang/examples/CallSuperAttribute/CallSuperAttrInfo.cpp

+//===- AnnotateFunctions.cpp ----------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Attribute plugin to mark a virtual method as ``call_super``, subclasses must
+// call it in the overridden method.
+//
+// This example shows that attribute plugins combined with ``PluginASTAction``
+// in Clang can do some of the same things which Java Annotations do.
+//
+// Unlike the other attribute plugin examples, this one does not attach an
+// attribute AST node to the declaration AST node. Instead, it keeps a separate
+// list of attributed declarations, which may be faster than using
+// ``Decl::getAttr<T>()`` in some cases. The disadvantage of this approach is
+// that the attribute is not part of the AST, which means that dumping the AST
+// will lose the attribute information, pretty printing the AST won't write the
+// attribute back out to source, and AST matchers will not be able to match
+// against the attribute on the declaration.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Frontend/FrontendPluginRegistry.h"
+#include "clang/Sema/ParsedAttr.h"
+#include "clang/Sema/Sema.h"
+#include "clang/Sema/SemaDiagnostic.h"
+#include "llvm/ADT/SmallPtrSet.h"
+using namespace clang;
+
+namespace {
+// Cached methods which are marked as 'call_super'.
+llvm::SmallPtrSet<const CXXMethodDecl *, 16> MarkedMethods;
+bool isMarkedAsCallSuper(const CXXMethodDecl *D) {
+  // Uses this way to avoid add an annotation attr to the AST.
+  return MarkedMethods.contains(D);
+}
+
+class MethodUsageVisitor : public RecursiveASTVisitor<MethodUsageVisitor> {
+public:
+  bool IsOverriddenUsed = false;
+  explicit MethodUsageVisitor(
+      llvm::SmallPtrSet<const CXXMethodDecl *, 16> &MustCalledMethods)
+      : MustCalledMethods(MustCalledMethods) {}
+  bool VisitCallExpr(CallExpr *CallExpr) {
+    const CXXMethodDecl *Callee = nullptr;
+    for (const auto &MustCalled : MustCalledMethods) {
+      if (CallExpr->getCalleeDecl() == MustCalled) {
+        // Super is called.
+        // Notice that we cannot do delete or insert in the iteration
+        // when using SmallPtrSet.
+        Callee = MustCalled;
+      }
+    }
+    if (Callee)
+      MustCalledMethods.erase(Callee);
+
+    return true;
+  }
+
+private:
+  llvm::SmallPtrSet<const CXXMethodDecl *, 16> &MustCalledMethods;
+};
+
+class CallSuperVisitor : public RecursiveASTVisitor<CallSuperVisitor> {
+public:
+  CallSuperVisitor(DiagnosticsEngine &Diags) : Diags(Diags) {
+    WarningSuperNotCalled = Diags.getCustomDiagID(
+        DiagnosticsEngine::Warning,
+        "virtual function %q0 is marked as 'call_super' but this overriding "
+        "method does not call the base version");
+    NotePreviousCallSuperDeclaration = Diags.getCustomDiagID(
+        DiagnosticsEngine::Note, "function marked 'call_super' here");
+  }
+  bool VisitCXXMethodDecl(CXXMethodDecl *MethodDecl) {
+    if (MethodDecl->isThisDeclarationADefinition() && MethodDecl->hasBody()) {
+      // First find out which overridden methods are marked as 'call_super'
+      llvm::SmallPtrSet<const CXXMethodDecl *, 16> OverriddenMarkedMethods;
+      for (const auto *Overridden : MethodDecl->overridden_methods()) {
+        if (isMarkedAsCallSuper(Overridden)) {
+          OverriddenMarkedMethods.insert(Overridden);
+        }
+      }
+
+      // Now find if the superclass method is called in `MethodDecl`.
+      MethodUsageVisitor Visitor(OverriddenMarkedMethods);
+      Visitor.TraverseDecl(MethodDecl);
+      // After traversing, all methods left in `OverriddenMarkedMethods`
+      // are not called, warn about these.
+      for (const auto &LeftOverriddens : OverriddenMarkedMethods) {
+        Diags.Report(MethodDecl->getLocation(), WarningSuperNotCalled)
+            << LeftOverriddens << MethodDecl;
+        Diags.Report(LeftOverriddens->getLocation(),
+                     NotePreviousCallSuperDeclaration);
+      }
+    }
+    return true;
+  }
+
+private:
+  DiagnosticsEngine &Diags;
+  unsigned WarningSuperNotCalled;
+  unsigned NotePreviousCallSuperDeclaration;
+};
+
+class CallSuperConsumer : public ASTConsumer {
+public:
+  void HandleTranslationUnit(ASTContext &Context) override {
+    auto &Diags = Context.getDiagnostics();
+    for (const auto *Method : MarkedMethods) {
+      lateDiagAppertainsToDecl(Diags, Method);
+    }
+
+    CallSuperVisitor Visitor(Context.getDiagnostics());
+    Visitor.TraverseDecl(Context.getTranslationUnitDecl());
+  }
+
+private:
+  // This function does checks which cannot be done in `diagAppertainsToDecl()`,
+  // typical example is checking Attributes (such as `FinalAttr`), on the time
+  // when `diagAppertainsToDecl()` is called, `FinalAttr` is not added into
+  // the AST yet.
+  void lateDiagAppertainsToDecl(DiagnosticsEngine &Diags,
+                                const CXXMethodDecl *MethodDecl) {
+    if (MethodDecl->hasAttr<FinalAttr>()) {
+      unsigned ID = Diags.getCustomDiagID(
+          DiagnosticsEngine::Warning,
+          "'call_super' attribute marked on a final method");
+      Diags.Report(MethodDecl->getLocation(), ID);
+    }
+  }
+};
+
+class CallSuperAction : public PluginASTAction {
+public:
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
+                                                 llvm::StringRef) override {
+    return std::make_unique<CallSuperConsumer>();
+  }
+
+  bool ParseArgs(const CompilerInstance &CI,
+                 const std::vector<std::string> &args) override {
+    return true;
+  }
+
+  PluginASTAction::ActionType getActionType() override {
+    return AddBeforeMainAction;
+  }
+};
+
+struct CallSuperAttrInfo : public ParsedAttrInfo {
+  CallSuperAttrInfo() {
+    OptArgs = 0;
+    static constexpr Spelling S[] = {
+        {ParsedAttr::AS_GNU, "call_super"},
+        {ParsedAttr::AS_CXX11, "clang::call_super"}};
+    Spellings = S;
+  }
+
+  bool diagAppertainsToDecl(Sema &S, const ParsedAttr &Attr,
+                            const Decl *D) const override {
+    const auto *TheMethod = dyn_cast_or_null<CXXMethodDecl>(D);
+    if (!TheMethod || !TheMethod->isVirtual()) {
+      S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type_str)
+          << Attr << "virtual functions";
+      return false;
+    }
+    MarkedMethods.insert(TheMethod);
+    return true;
+  }
+  AttrHandling handleDeclAttribute(Sema &S, Decl *D,
+                                   const ParsedAttr &Attr) const override {
+    // No need to add an attr object (usually an `AnnotateAttr` is added).
+    // Save the address of the Decl in a set, it maybe faster than compare to
+    // strings.
+    return AttributeNotApplied;
+  }
+};
+
+} // namespace
+static FrontendPluginRegistry::Add<CallSuperAction>
+    X("call_super_plugin", "clang plugin, checks every overridden virtual "
+                           "function whether called this function or not.");
+static ParsedAttrInfoRegistry::Add<CallSuperAttrInfo>
+    Y("call_super_attr", "Attr plugin to define 'call_super' attribute");

clang/test/CMakeLists.txt

list(APPEND CLANG_TEST_DEPS
     c-arcmt-test
   )
 endif ()
 
 if (CLANG_BUILD_EXAMPLES)
   list(APPEND CLANG_TEST_DEPS
     Attribute
     AnnotateFunctions
+    CallSuperAttr
     clang-interpreter
     PrintFunctionNames
     )
 endif ()
 
 set(CLANG_TEST_PARAMS
   USE_Z3_SOLVER=0
   )

clang/test/Frontend/plugin-call-super.cpp

+// RUN: %clang -fplugin=%llvmshlibdir/CallSuperAttr%pluginext -fsyntax-only -Xclang -verify=callsuper %s
+// RUN: %clang -fplugin=%llvmshlibdir/CallSuperAttr%pluginext -DBAD_CALLSUPER -fsyntax-only -Xclang -verify=badcallsuper %s
+// REQUIRES: plugins, examples
+
+// callsuper-no-diagnostics
+struct Base1 {
+  [[clang::call_super]] virtual void Test() {}
+};
+struct Base2 {
+  [[clang::call_super]] virtual void Test() {}
+};
+struct Derive : public Base1, public Base2 {
+#ifndef BAD_CALLSUPER
+  void Test() override;
+#else
+  [[clang::call_super]] virtual void Test() override final;
+  // badcallsuper-warning@16 {{'call_super' attribute marked on a final method}}
+#endif
+};
+void Derive::Test() {
+  Base1::Test();
+#ifndef BAD_CALLSUPER
+  Base2::Test();
+#else
+  // badcallsuper-warning@20 {{virtual function 'Base2::Test' is marked as 'call_super' but this overriding method does not call the base version}}
+  // badcallsuper-note@10 {{function marked 'call_super' here}}
+#endif
+}
+struct Derive2 : public Base1, public Base2 {
+  void Test() override {
+    Base1::Test();
+    Base2::Test();
+  }
+};