[clang-tidy] new check cppcoreguidelines-pro-type-reinterpret-cast

[mgehre]

Summary

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#-type1-dont-use-reinterpret_cast.

[vsk]

The patch lgtm. Has there been a discussion on cfe-dev about adopting these guidelines?

I count well over 500 uses of reinterpret_cast in llvm+clang, so we might not all be on the same page on this.

[xazax.hun]

If you check AvoidCStyleCastsCheck.cpp, it also suggest what kind of cast should you use to replace a C style cast. Probably, in some cases, the developers might be able to change the reinterpret_cast to something more type safe. It would be nice to make a suggestion in those cases.

[aaron.ballman]

As a slightly more broad question: I think we should have a user-customizable way to categorize these checks so that you can enable/disable them with finer-grained control. Some of the existing checkers already cover the Cpp guidelines, and we'll likely be adding plenty more. There's quite likely overlap with Google and LLVM checkers, etc. It would be really nice if we had a way to say: -checks=-*, CppGuidelines or -checks=-*, CERT, etc.

(I'm not suggesting this as part of this patch, but I think it is an idea we should consider exploring because style guidelines abound: the new C++ ones, MISRA, CERT, joint strike fighter, etc. User-customizable categorization would really help for this sort of thing. This would help assuage my issue with the checker being on by default in misc-* -- it could be off in misc-* but on in cppcoreguidelines-*, for instance.)

~Aaron

clang-tidy/misc/NoReinterpretCastCheck.cpp

29 ↗

(On Diff #36155)

I am worried about the amount of chattiness for this diagnostic and the fact that it does not provide the user with any idea as to what to do instead. The C-style cast checker will suggest that users don't use C-style casts, which suggests there's no way to appease this checker. The style guide suggests using variant, but that is not a workable solution for projects that don't have a variant type.

FWIW, I've run into the same thing for CERT rules like:

DCL50-CPP. Do not define a C-style variadic function
MSC50-CPP. Do not use std::rand() for generating pseudorandom numbers (to a lesser extent, because <random> exists now.)

I don't have a particularly good solution to the issue though. I'm not opposed to the checker, but I am opposed to the checker being on by default for misc-* checkers.

[alexfh]

In D13313#257476, @aaron.ballman wrote:

As a slightly more broad question: I think we should have a user-customizable way to categorize these checks so that you can enable/disable them with finer-grained control. Some of the existing checkers already cover the Cpp guidelines, and we'll likely be adding plenty more. There's quite likely overlap with Google and LLVM checkers, etc. It would be really nice if we had a way to say: -checks=-*, CppGuidelines or -checks=-*, CERT, etc.

(I'm not suggesting this as part of this patch, but I think it is an idea we should consider exploring because style guidelines abound: the new C++ ones, MISRA, CERT, joint strike fighter, etc. User-customizable categorization would really help for this sort of thing. This would help assuage my issue with the checker being on by default in misc-* -- it could be off in misc-* but on in cppcoreguidelines-*, for instance.)

~Aaron

One way we could get CppCoreGuidelines checks available for easy enabling as a whole is to create a separate module and register all relevant checks there with names relevant to the CppCoreGuidelines (e.g. register the clang::tidy::google::ExplicitConstructorCheck there as "cppcoreguidelines-rc-explicit"). If a checks needs to be slightly modified in order to be closer to a specific rule, we might add some check options and configure proper defaults in the CppCoreGuidelinesModule::getModuleOptions(). If a larger change in behavior is needed, we could inherit from existing checks as well.

[alexfh]

In D13313#256982, @vsk wrote:

The patch lgtm. Has there been a discussion on cfe-dev about adopting these guidelines?

I count well over 500 uses of reinterpret_cast in llvm+clang, so we might not all be on the same page on this.

I don't think LLVM can adopt a significant part of C++ Core Guidelines. Nevertheless, it makes sense to have these rules implemented as clang-tidy checks for the projects that decide to adopt them.

[alexfh]

A high-level comment: the "misc" module is the place for checks that we didn't find (or create) a better category for. Here it's clear that we need a separate category, so we need a CppCoreGuidelinesModule and the cppcoreguidelines- check prefix. I also suggest using anchor names (converted to lower case) in the document as check names, this one would be cppcoreguidelines-pro-type-reinterpretcast.

test/clang-tidy/misc-no-reinterpret-cast.cpp

7 ↗

(On Diff #36155)

Please add a newline at the end of file to pacify various diff tools.

[aaron.ballman]

In D13313#257506, @alexfh wrote:

In D13313#257476, @aaron.ballman wrote:

As a slightly more broad question: I think we should have a user-customizable way to categorize these checks so that you can enable/disable them with finer-grained control. Some of the existing checkers already cover the Cpp guidelines, and we'll likely be adding plenty more. There's quite likely overlap with Google and LLVM checkers, etc. It would be really nice if we had a way to say: -checks=-*, CppGuidelines or -checks=-*, CERT, etc.

(I'm not suggesting this as part of this patch, but I think it is an idea we should consider exploring because style guidelines abound: the new C++ ones, MISRA, CERT, joint strike fighter, etc. User-customizable categorization would really help for this sort of thing. This would help assuage my issue with the checker being on by default in misc-* -- it could be off in misc-* but on in cppcoreguidelines-*, for instance.)

~Aaron

One way we could get CppCoreGuidelines checks available for easy enabling as a whole is to create a separate module and register all relevant checks there with names relevant to the CppCoreGuidelines (e.g. register the clang::tidy::google::ExplicitConstructorCheck there as "cppcoreguidelines-rc-explicit"). If a checks needs to be slightly modified in order to be closer to a specific rule, we might add some check options and configure proper defaults in the CppCoreGuidelinesModule::getModuleOptions(). If a larger change in behavior is needed, we could inherit from existing checks as well.

That's a good idea that I hadn't considered before. I may give this a shot for the CERT checks as well. Thank you for the suggestion!

~Aaron

[mgehre]

Renamed the check to cppcoreguidelines-pro-type-reinterpret-cast

[mgehre]

Fix add_new_check.py for capitalization of CppCoreGuidelinesTidyModule.cpp

[mgehre]

Read 'Check' suffix on ProTypeReinterpretCastCheck

[aaron.ballman]

LGTM with one minor nit. Thank you for working on this!

~Aaron

clang-tidy/add_new_check.py

150

Thank you for pointing this out, I hadn't realized we had a helper for this, let alone that it does the wrong thing for the new module I was working on. :-P

clang-tidy/cppcoreguidelines/ProTypeReinterpretCastCheck.cpp

31

I don't think we need the parenthetical any longer because the name of the checker will already be displayed to the user with the diagnostic.

[mgehre]

Rebased
Removed "(C++ Core Guidelines, rule Type.1)" from diagnostic

Can someone please commit this?

[mgehre]

Shot to fast, fixed test.

Now it's ready

[aaron.ballman]

Does not apply cleanly to ToT; can you rebase?

~Aaron

[mgehre]

rebased

[mgehre]

I'm not sure what you mean by ToT. I rebased this against master @ http://llvm.org/git/clang-tools-extra.git

[mgehre]

Rebased

[mgehre]

"arc diff" produced a invalid patch file (due to some file being "moved"?)
This patch is manually generated by git format-patch. I checked that it applies cleanly against svn trunk.

[aaron.ballman]

Thank you for the patch! I've committed it in r249399.

~Aaron

clang-tidy/CMakeLists.txt

 add_subdirectory(tool)
 add_subdirectory(cert)
 add_subdirectory(llvm)
+add_subdirectory(cppcoreguidelines)
 add_subdirectory(google)
 add_subdirectory(misc)
 add_subdirectory(modernize)

clang-tidy/Makefile

 LIBRARYNAME := clangTidy
 include $(CLANG_LEVEL)/../../Makefile.config
 
-DIRS = utils cert readability llvm google misc modernize tool
+DIRS = utils cert cppcoreguidelines readability llvm google misc modernize tool
 
 include $(CLANG_LEVEL)/Makefile

clang-tidy/add_new_check.py

 
 # Modifies the module to include the new check.
 def adapt_module(module_path, module, check_name, check_name_camel):
-  filename = os.path.join(module_path, module.capitalize() + 'TidyModule.cpp')
+  modulecpp = filter(lambda p: p.lower() == module.lower() + "tidymodule.cpp", os.listdir(module_path))[0]
+  filename = os.path.join(module_path, modulecpp)
   with open(filename, 'r') as f:
     lines = f.readlines()
 

clang-tidy/cppcoreguidelines/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS support)
+
+add_clang_library(clangTidyCppCoreGuidelinesModule
+  CppCoreGuidelinesTidyModule.cpp
+  ProTypeReinterpretCastCheck.cpp
+
+  LINK_LIBS
+  clangAST
+  clangASTMatchers
+  clangBasic
+  clangLex
+  clangTidy
+  clangTidyUtils
+  clangTooling
+  )

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

+//===--- CppCoreGuidelinesModule.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 "../ClangTidy.h"
+#include "../ClangTidyModule.h"
+#include "../ClangTidyModuleRegistry.h"
+#include "ProTypeReinterpretCastCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+class CppCoreGuidelinesModule : public ClangTidyModule {
+public:
+  void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<ProTypeReinterpretCastCheck>(
+        "cppcoreguidelines-pro-type-reinterpret-cast");
+  }
+};
+
+// Register the LLVMTidyModule using this statically initialized variable.
+static ClangTidyModuleRegistry::Add<CppCoreGuidelinesModule>
+    X("cppcoreguidelines-module", "Adds checks for the C++ Core Guidelines.");
+
+} // namespace cppcoreguidelines
+
+// This anchor is used to force the linker to link in the generated object file
+// and thus register the CppCoreGuidelinesModule.
+volatile int CppCoreGuidelinesModuleAnchorSource = 0;
+
+} // namespace tidy
+} // namespace clang

clang-tidy/cppcoreguidelines/Makefile

+##===- clang-tidy/cppcoreguidelines/Makefile ----------------------------*- Makefile -*-===##
+#
+#                     The LLVM Compiler Infrastructure
+#
+# This file is distributed under the University of Illinois Open Source
+# License. See LICENSE.TXT for details.
+#
+##===----------------------------------------------------------------------===##
+CLANG_LEVEL := ../../../..
+LIBRARYNAME := clangTidyCppCoreGuidelinesModule
+
+include $(CLANG_LEVEL)/Makefile

clang-tidy/cppcoreguidelines/ProTypeReinterpretCastCheck.h

+//===--- ProTypeReinterpretCast.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_PRO_TYPE_REINTERPRETCAST_CHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_REINTERPRETCAST_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+
+/// Flags all occurrences of reinterpret_cast
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-no-reinterpret-cast.html
+class ProTypeReinterpretCastCheck : public ClangTidyCheck {
+public:
+  ProTypeReinterpretCastCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_PRO_TYPE_REINTERPRETCAST_CHECK_H

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 "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+
+void ProTypeReinterpretCastCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(cxxReinterpretCastExpr().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 tidy
+} // namespace clang

clang-tidy/tool/CMakeLists.txt

   clangBasic
   clangTidy
   clangTidyCERTModule
+  clangTidyCppCoreGuidelinesModule
   clangTidyGoogleModule
   clangTidyLLVMModule
   clangTidyMiscModule

clang-tidy/tool/ClangTidyMain.cpp

 static int LLVM_ATTRIBUTE_UNUSED LLVMModuleAnchorDestination =
     LLVMModuleAnchorSource;
 
+// This anchor is used to force the linker to link the CppCoreGuidelinesModule.
+extern volatile int CppCoreGuidelinesModuleAnchorSource;
+static int LLVM_ATTRIBUTE_UNUSED CppCoreGuidelinesModuleAnchorDestination =
+    CppCoreGuidelinesModuleAnchorSource;
+
 // This anchor is used to force the linker to link the GoogleModule.
 extern volatile int GoogleModuleAnchorSource;
 static int LLVM_ATTRIBUTE_UNUSED GoogleModuleAnchorDestination =

clang-tidy/tool/Makefile

 USEDLIBS = clangTidy.a clangTidyLLVMModule.a clangTidyGoogleModule.a \
            clangTidyMiscModule.a clangTidyModernizeModule.a clangTidyReadability.a \
      clangTidyUtils.a clangTidyCERTModule.a clangStaticAnalyzerFrontend.a \
+     clangTidyCppCoreGuidelinesModule.a \
      clangStaticAnalyzerCheckers.a clangStaticAnalyzerCore.a \
      clangFormat.a clangASTMatchers.a clangTooling.a clangToolingCore.a \
      clangFrontend.a clangSerialization.a clangDriver.a clangParse.a \

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

+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#-type1-dont-use-reinterpret_cast.

docs/clang-tidy/checks/list.rst

 
 .. toctree::
    cert-variadic-function-def
+   cppcoreguidelines-pro-type-reinterpret-cast
    google-build-explicit-make-pair
    google-build-namespaces
    google-build-using-namespace

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

+// RUN: %python %S/check_clang_tidy.py %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]