[clang-tidy] Use [[clang::suppress]] with cppcoreguidelines-pro-type-reinterpret-cast

[mgehre]

Summary

This is a proof-of-concept how the [[clang::suppress(tags,...)]] attribute can
work with suppressing clang-tidy warnings.

The list of tags that can be used to suppress a specific warnings comes
from [1]. It is the group that the cppcoreguidelines check belongs to
("type"), the name of the check there ("type.1") and the clang-tidy
name ("cppcoreguidelines-pro-type-reinterpret-cast").

I envision to put this into all cppcoreguidelines checks. Alternatively,
this could go into clang-tidy core, to have this automatically for all
checks.

I'm looking forward to your comments!

Depends on: D24886

[1] https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#inforce-enforcement

[mgehre]

Minor fix

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeReinterpretCastCheck.cpp

25

Hmm, it seems like this is boilerplate we are going to want for every rule, and that it's pretty easy to not get this right (for instance, if you change the way the check is spelled, you have to remember to update this as well). Could this actually be handled more transparently, by gathering this information when the check is registered and exposing it to the check?

The checks would still need to use unless(isSuppressed(Rules)) in some form, but I am thinking that it would be more convenient if we could do: Finder->addMatcher(cxxReinterpretCastExpr(unlessSuppressed(*this)).bind("cast"), this);

clang-tidy/cppcoreguidelines/Suppression.h

59

Why is the check for hasAttrs required?

Also, this use of hasAncestor() makes this expensive to use, and that expense may be hidden from the caller. Is there a way to structure this so that we don't need to walk the entire ancestor tree?

[mgehre]

clang-tidy/cppcoreguidelines/ProTypeReinterpretCastCheck.cpp

25

I see multiple ways on how to integrate that into clang-tidy:

1. Add something like you proposed to each matcher of each check
2. Change (or add overload of)

DiagnosticBuilder diag(SourceLocation Loc, StringRef Description,
                        DiagnosticIDs::Level Level = DiagnosticIDs::Warning);

to add a AST node as parameter and make the SourceLocation optional. Most checks anyhow
call this like diag(E->getLoc(), "...."), and then they would do diag(E, "...").
Diag then would check from the that AST node upwards to see if it finds a matching [[suppress]] attribute

3. Change the RecursiveASTVistor that drives the AST Matches to prune certain matchers from the list of to-be-evaluated matchers

for all AST subtrees that are below a certain [[suppress]] attribute. (This is based on how I image that the AST matchers work)

clang-tidy/cppcoreguidelines/Suppression.h

59

hasAttr() is needed here, because inside of hasSuppressAttr(), we call getAttrs() which would assert if hasAttr() is false.
I cannot push hasAttr() into hasSuppressAttr(), because hasSuppressAttr() is a template getting either Decl or AttributedStmt,
and AttributedStmt does not have an hasAttr() method.

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeReinterpretCastCheck.cpp

25

Ideally, I would like to see this attribute used to suppress Clang diagnostics as well, however. So while I think Option 2 is better suited to that future direction, it's still not ideal because all instances of diag() need to be updated to take advantage. Options 1 and 3 are basically limited to clang-tidy use.

I wonder if there's a way to modify the diagnostic builder to transparently handle this without requiring modifying all of the call sites?

clang-tidy/cppcoreguidelines/Suppression.h

59

I believe that getSpecificAttr should be resilient to no attributes being present (since it also has to handle the case there are no attributes of the specific type, so no attributes at all is simply a degenerate case), and so the check for hasAttrs() should be redundant.

[malcolm.parsons]

clang-tidy/cppcoreguidelines/ProTypeReinterpretCastCheck.cpp

25

clang-tidy reports how many warnings were suppressed by NOLINT comments.
I'd expect the number of warnings suppressed by [[clang::suppress]] to be included in the count.
Handling suppressions in the matchers or visitors would prevent this.

[aaron.ballman]

clang-tidy/cppcoreguidelines/ProTypeReinterpretCastCheck.cpp

25

As would handling the suppression transparently within the diagnostic engine itself.

[mgehre]

clang-tidy/cppcoreguidelines/ProTypeReinterpretCastCheck.cpp

25

If there is a way to turn a SourceLocation into a ASTNode, diag() could handle this transparently (do you know how?). I would still add an overload of diag that takes an AST node as a performance optimization, because I imagine that going from SourceLocation to ASTNode would be a costly operation.
I can prototype that approach, if you like.

clang-tidy/cppcoreguidelines/Suppression.h

59

Decl::getAttrs() will assert if called on a Decl where hasAttrs() is false, see
http://clang.llvm.org/doxygen/DeclBase_8cpp_source.html#l00741

[aaron.ballman]

clang-tidy/cppcoreguidelines/Suppression.h

59

Ugh, that is a problem, but not one you should have to handle as part of this patch. I'll put it on my list of things to look into, thank you for being patient with me! :-)

[bam]

I'm sorry, could we bring it to the finish line?
It's a pity the standard C++ Core Guidelines rule suppression syntax is still not supported, as Clang-tidy is one of the main tools supporting the Guidelines..

Happy new year to all!

[mgehre]

In D24888#4025382, @bam wrote:

I'm sorry, could we bring it to the finish line?
It's a pity the standard C++ Core Guidelines rule suppression syntax is still not supported, as Clang-tidy is one of the main tools supporting the Guidelines..

Happy new year to all!

Hi @bam, this is currently not on my priority list. Feel free to take over the PR!

[bam]

I'm sorry but I'm not familiar with the codebase, so I can't help, unfortunately.

clang-tidy/cppcoreguidelines/ProTypeReinterpretCastCheck.cpp

 //===--- ProTypeReinterpretCastCheck.cpp - clang-tidy----------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ProTypeReinterpretCastCheck.h"
+#include "Suppression.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 void ProTypeReinterpretCastCheck::registerMatchers(MatchFinder *Finder) {
   if (!getLangOpts().CPlusPlus)
     return;
 
-  Finder->addMatcher(cxxReinterpretCastExpr().bind("cast"), this);
+  std::vector<StringRef> Rules{"type", "type.1", "cppcoreguidelines-pro-type-reinterpret-cast"};
+  Finder->addMatcher(cxxReinterpretCastExpr(unless(isSuppressed(Rules))).bind("cast"), this);
 }
 
 void ProTypeReinterpretCastCheck::check(
     const MatchFinder::MatchResult &Result) {
   const auto *MatchedCast =
       Result.Nodes.getNodeAs<CXXReinterpretCastExpr>("cast");
   diag(MatchedCast->getOperatorLoc(), "do not use reinterpret_cast");
 }
 
 } // namespace cppcoreguidelines
 } // namespace tidy
 } // namespace clang

clang-tidy/cppcoreguidelines/Suppression.h

+//===--- Suppression.h - clang-tidy------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SUPPRESSION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SUPPRESSION_H
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+const ast_matchers::internal::VariadicDynCastAllOfMatcher<Stmt, AttributedStmt>
+    attributedStmt;
+
+AST_MATCHER(Decl, hasAttrs) { return Node.hasAttrs(); }
+
+// If called on a Decl, assumes that hasAttrs() is true.
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+    hasSuppressAttr, AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, AttributedStmt),
+    std::vector<StringRef>, Rules, 0) {
+  llvm::outs() << "hasSuppressAttr called\n";
+  if (const auto *Suppress =
+          getSpecificAttr<const SuppressAttr>(Node.getAttrs())) {
+    for (const auto &Rule : Rules) {
+      auto It = std::find_if(Suppress->rules_begin(), Suppress->rules_end(),
+                             [Rule](const std::string &R) {
+                               llvm::outs() << "Rule " << R << "\n";
+                               return R == Rule;
+                             });
+      if (It != Suppress->rules_end())
+        return true;
+    }
+  }
+  return false;
+}
+
+// If called on a Decl, assumes that hasAttrs() is true.
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+    hasSuppressAttr, AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, AttributedStmt),
+    StringRef, Rule, 1) {
+  std::vector<StringRef> Rules(1, Rule);
+  return ast_matchers::internal::Matcher<NodeType>(hasSuppressAttr(Rules))
+      .matches(Node, Finder, Builder);
+}
+
+/// Checks if any ancestor has a [[clang::suppress(rule, ...)]] attribute with
+/// one of the given Rules.
+AST_POLYMORPHIC_MATCHER_P(isSuppressed,
+                          AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt),
+                          std::vector<StringRef>, Rules) {
+  using namespace clang::ast_matchers;
+  return ast_matchers::internal::Matcher<NodeType>(
+             anyOf(hasAncestor(attributedStmt(hasSuppressAttr(Rules))),
+                   hasAncestor(decl(hasAttrs(), hasSuppressAttr(Rules)))))
+      .matches(Node, Finder, Builder);
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_SUPPRESSION_H

docs/clang-tidy/checks/cppcoreguidelines-pro-type-reinterpret-cast.rst

 .. title:: clang-tidy - cppcoreguidelines-pro-type-reinterpret-cast
 
 cppcoreguidelines-pro-type-reinterpret-cast
 ===========================================
 
 This check flags all uses of ``reinterpret_cast`` in C++ code.
 
 Use of these casts can violate type safety and cause the program to access a
 variable that is actually of type ``X`` to be accessed as if it were of an
 unrelated type ``Z``.
 
 This rule is part of the "Type safety" profile of the C++ Core Guidelines, see
 https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Pro-type-reinterpretcast.
+
+Warnings of this check can be suppressed by using the C++11 attribute
+``[[clang::suppress("tag")]]`` with a tag of either "type", "type.1" or
+"cppcoreguidelines-pro-type-reinterpret-cast".

test/clang-tidy/cppcoreguidelines-pro-type-reinterpret-cast.cpp

 // RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-reinterpret-cast %t
 
 int i = 0;
 void *j;
 void f() { j = reinterpret_cast<void *>(i); }
 // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: do not use reinterpret_cast [cppcoreguidelines-pro-type-reinterpret-cast]
+
+[[clang::suppress("cppcoreguidelines-pro-type-reinterpret-cast")]]
+void suppress_f() { j = reinterpret_cast<void *>(i); }
+
+void suppress_f2() {
+    [[clang::suppress("type")]] j = reinterpret_cast<void *>(i);
+    [[clang::suppress("type.1")]] {
+        i = 1;
+        j = reinterpret_cast<void *>(i);
+    }
+}