[clang-tidy] Add misc-static-declaration-in-header check

[carlosgalvezp]

Summary

Static declarations in header files is considered bad practice,
since header files are meant for sharing code, and static
leads to internal linkage. This can cause code bloat and lead
to subtle issues, for example ODR violations. Essentially, this
is as problematic as having anonymous namespaces in headers,
which we already have checks for.

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.cpp

47

Look like this setting should be global for Clang-tidy, because it already used in other checks. It also accompanied by other option.

clang-tools-extra/docs/ReleaseNotes.rst

123

Please add first sentence from documentation.

clang-tools-extra/docs/clang-tidy/checks/misc/static-declaration-in-header.rst

40

Please replace quotes with single back-ticks.

[carlosgalvezp]

clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.cpp

47

Fully agree, I can put up a separate patch for that. I suppose we need to wait 2 releases to be able to remove the existing duplicated options from the checks?

@njames93 Do you agree?

[carlosgalvezp]

Add description in release notes.

clang-tools-extra/docs/clang-tidy/checks/misc/static-declaration-in-header.rst

40

There's 3 other such instances of this pattern:

• 1 of them has: `";...."` (double back tick followed by quotes).
• 2 of them have it like I wrote it here.

None of them follow the format you propose, so I'd be reluctant to introduce a third convention :) Would it be OK to keep the most prominent format for consistency, and fix the formatting for all in a separate patch?

[carlosgalvezp]

clang-tools-extra/docs/clang-tidy/checks/misc/static-declaration-in-header.rst

40

Sorry, there's 2, not 3 (the third one is this check :) )

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/misc/static-declaration-in-header.rst

40

Single back-ticks are used for option values. Will be good idea to fix other places too.

[carlosgalvezp]

Replace quotes with single backticks.

[carlosgalvezp]

Warn only about cases where there really is a problem.
There are other cases which are just a readability issue,
where a separate check for "redundant static" would fit
better. Remove also the automatic fix-it since it leads
to potentially broken code.

[carlosgalvezp]

Remove remaining function declaration in the check class.

[carlosgalvezp]

Narrow warnings further, and provide fix-its.

[carlosgalvezp]

I'm happy with the check now, feel free to review! I have run it on the LLVM repo and it seems to do what it's supposed to (including fix-its).

[carlosgalvezp]

Friendly ping.

[carlosgalvezp]

Friendly ping. I realized @aaron.ballman you probably have very good insights into this use case, since (AFAICT) you wrote a guideline about anonymous namespaces in headers, which is very closely related to this:
https://wiki.sei.cmu.edu/confluence/display/cplusplus/DCL59-CPP.+Do+not+define+an+unnamed+namespace+in+a+header+file

Would appreciate your feedback!

[carlosgalvezp]

Rebase

clang-tools-extra/clang-tidy/misc/CMakeLists.txt

add_clang_library(clangTidyMiscModule
   MisleadingIdentifier.cpp
   MisplacedConstCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoRecursionCheck.cpp
   NonCopyableObjects.cpp
   NonPrivateMemberVariablesInClassesCheck.cpp
   RedundantExpressionCheck.cpp
   StaticAssertCheck.cpp
+  StaticDeclarationInHeaderCheck.cpp
   ThrowByValueCatchByReferenceCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   UniqueptrResetReleaseCheck.cpp
   UnusedAliasDeclsCheck.cpp
   UnusedParametersCheck.cpp
   UnusedUsingDeclsCheck.cpp
   UseAnonymousNamespaceCheck.cpp
 

clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

 #include "MisleadingIdentifier.h"
 #include "MisplacedConstCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoRecursionCheck.h"
 #include "NonCopyableObjects.h"
 #include "NonPrivateMemberVariablesInClassesCheck.h"
 #include "RedundantExpressionCheck.h"
 #include "StaticAssertCheck.h"
+#include "StaticDeclarationInHeaderCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedUsingDeclsCheck.h"
 #include "UseAnonymousNamespaceCheck.h"
 
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<NoRecursionCheck>("misc-no-recursion");
     CheckFactories.registerCheck<NonCopyableObjectsCheck>(
         "misc-non-copyable-objects");
     CheckFactories.registerCheck<NonPrivateMemberVariablesInClassesCheck>(
         "misc-non-private-member-variables-in-classes");
     CheckFactories.registerCheck<RedundantExpressionCheck>(
         "misc-redundant-expression");
     CheckFactories.registerCheck<StaticAssertCheck>("misc-static-assert");
+    CheckFactories.registerCheck<StaticDeclarationInHeaderCheck>(
+        "misc-static-declaration-in-header");
     CheckFactories.registerCheck<ThrowByValueCatchByReferenceCheck>(
         "misc-throw-by-value-catch-by-reference");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<UniqueptrResetReleaseCheck>(
         "misc-uniqueptr-reset-release");
     CheckFactories.registerCheck<UnusedAliasDeclsCheck>(
         "misc-unused-alias-decls");

clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.h

+//===--- StaticDeclarationInHeaderCheck.h - clang-tidy ----------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STATICDECLARATIONINHEADERCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STATICDECLARATIONINHEADERCHECK_H
+
+#include "../ClangTidyCheck.h"
+#include "../utils/FileExtensionsUtils.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Warns about ``static`` variable and function declarations at file or
+/// namespace scope in header files. Provides fix-it hint to remove ``static``.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc/static-declaration-in-header.html
+class StaticDeclarationInHeaderCheck : public ClangTidyCheck {
+public:
+  StaticDeclarationInHeaderCheck(StringRef Name, ClangTidyContext *Context);
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+  const StringRef RawStringHeaderFileExtensions;
+  utils::FileExtensionsSet HeaderFileExtensions;
+  SourceRange getStaticSourceRange(NamedDecl const *MatchedDecl,
+                                   const SourceManager &SM);
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STATICDECLARATIONINHEADERCHECK_H

clang-tools-extra/clang-tidy/misc/StaticDeclarationInHeaderCheck.cpp

+//===--- StaticDeclarationInHeaderCheck.cpp - clang-tidy ------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "StaticDeclarationInHeaderCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+namespace {
+
+AST_POLYMORPHIC_MATCHER(isStatic, AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+                                                                  VarDecl)) {
+  return Node.getStorageClass() == SC_Static;
+}
+
+AST_MATCHER(FunctionDecl, isMemberFunction) {
+  return llvm::isa<CXXMethodDecl>(&Node);
+}
+
+AST_MATCHER(VarDecl, isStaticDataMember) { return Node.isStaticDataMember(); }
+
+AST_POLYMORPHIC_MATCHER_P(isInHeaderFile,
+                          AST_POLYMORPHIC_SUPPORTED_TYPES(FunctionDecl,
+                                                          VarDecl),
+                          utils::FileExtensionsSet, HeaderFileExtensions) {
+  return utils::isExpansionLocInHeaderFile(
+      Node.getBeginLoc(), Finder->getASTContext().getSourceManager(),
+      HeaderFileExtensions);
+}
+
+} // namespace
+
+StaticDeclarationInHeaderCheck::StaticDeclarationInHeaderCheck(
+    StringRef Name, ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      RawStringHeaderFileExtensions(Options.getLocalOrGlobal(
+          "HeaderFileExtensions", utils::defaultHeaderFileExtensions())) {
+  if (!utils::parseFileExtensions(RawStringHeaderFileExtensions,
+                                  HeaderFileExtensions,
+                                  utils::defaultFileExtensionDelimiters())) {
+    this->configurationDiag("Invalid header file extension: '%0'")
+        << RawStringHeaderFileExtensions;
+  }
+}
+
+SourceRange StaticDeclarationInHeaderCheck::getStaticSourceRange(
+    NamedDecl const *MatchedDecl, const SourceManager &SM) {
+  Token Tok;
+  SourceLocation Loc = MatchedDecl->getSourceRange().getBegin();
+  while (Loc < MatchedDecl->getSourceRange().getEnd() &&
+         !Lexer::getRawToken(Loc, Tok, SM, getLangOpts(), true)) {
+    SourceRange TokenRange(Tok.getLocation(), Tok.getEndLoc());
+    StringRef SourceText = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(TokenRange), SM, getLangOpts());
+    if (SourceText == "static") {
+      return TokenRange;
+    }
+    Loc = Tok.getEndLoc();
+  }
+  llvm_unreachable("Could not find 'static' token");
+}
+
+void StaticDeclarationInHeaderCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
+}
+
+void StaticDeclarationInHeaderCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(functionDecl(isStatic(),
+                                  isInHeaderFile(HeaderFileExtensions),
+                                  unless(isMemberFunction()))
+                         .bind("x"),
+                     this);
+  Finder->addMatcher(
+      varDecl(isStatic(), isInHeaderFile(HeaderFileExtensions),
+              unless(anyOf(isStaticLocal(), isStaticDataMember())))
+          .bind("x"),
+      this);
+}
+
+void StaticDeclarationInHeaderCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  if (const auto *MatchedDecl = Result.Nodes.getNodeAs<NamedDecl>("x")) {
+    diag(MatchedDecl->getLocation(),
+         "static declaration in header; remove 'static'")
+        << FixItHint::CreateRemoval(
+               getStaticSourceRange(MatchedDecl, *Result.SourceManager));
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`cppcoreguidelines-avoid-const-or-ref-data-members
 
   Warns when a struct or class uses const or reference (lvalue or rvalue) data members.
 
 - New :doc:`cppcoreguidelines-avoid-do-while
   <clang-tidy/checks/cppcoreguidelines/avoid-do-while>` check.
 
   Warns when using ``do-while`` loops.
 
+- New :doc:`misc-static-declaration-in-header
+  <clang-tidy/checks/misc/static-declaration-in-header>` check.
+
+  Warns about ``static`` variable and function declarations at file or
+  namespace scope in header files.
+
 - New :doc:`misc-use-anonymous-namespace
   <clang-tidy/checks/misc/use-anonymous-namespace>` check.
 
   Warns when using ``static`` function or variables at global scope, and suggests
   moving them into an anonymous namespace.
 
 - New :doc:`bugprone-standalone-empty <clang-tidy/checks/bugprone/standalone-empty>` check.
 

clang-tools-extra/docs/clang-tidy/checks/list.rst

.. csv-table::
    `misc-misleading-identifier <misc/misleading-identifier.html>`_,
    `misc-misplaced-const <misc/misplaced-const.html>`_,
    `misc-new-delete-overloads <misc/new-delete-overloads.html>`_,
    `misc-no-recursion <misc/no-recursion.html>`_,
    `misc-non-copyable-objects <misc/non-copyable-objects.html>`_,
    `misc-non-private-member-variables-in-classes <misc/non-private-member-variables-in-classes.html>`_,
    `misc-redundant-expression <misc/redundant-expression.html>`_, "Yes"
    `misc-static-assert <misc/static-assert.html>`_, "Yes"
+   `misc-static-declaration-in-header <misc/static-declaration-in-header.html>`_, "Yes"
    `misc-throw-by-value-catch-by-reference <misc/throw-by-value-catch-by-reference.html>`_,
    `misc-unconventional-assign-operator <misc/unconventional-assign-operator.html>`_,
    `misc-uniqueptr-reset-release <misc/uniqueptr-reset-release.html>`_, "Yes"
    `misc-unused-alias-decls <misc/unused-alias-decls.html>`_, "Yes"
    `misc-unused-parameters <misc/unused-parameters.html>`_, "Yes"
    `misc-unused-using-decls <misc/unused-using-decls.html>`_, "Yes"
    `misc-use-anonymous-namespace <misc/use-anonymous-namespace.html>`_,
    `modernize-avoid-bind <modernize/avoid-bind.html>`_, "Yes"

clang-tools-extra/docs/clang-tidy/checks/misc/static-declaration-in-header.rst

+.. title:: clang-tidy - misc-static-declaration-in-header
+
+misc-static-declaration-in-header
+=================================
+
+Warns about ``static`` variable and function declarations at file or namespace
+scope in header files. Provides fix-it hint to remove ``static``.
+
+Header files are used to share code among different translation units. ``static``,
+on the other hand, can be used to express that a given declaration has internal
+linkage. Using ``static`` in header files leads to each translation unit creating
+their own internal copy of the function or variable at hand, which is problematic.
+This can cause unexpected results, bloat the resulting executable or even trigger
+one-definition-rule (ODR) violations.
+
+This problem is similar to having anonymous namespaces in header files, already
+covered by :doc:`google-build-namespaces<clang-tidy/checks/google/build-namespaces>`.
+
+Example of problematic code:
+
+.. code-block:: c++
+
+  // foo.h
+
+  // Bad
+  static int x = 123;
+  static void f() {}
+
+  // Good
+  int x = 123;
+  void f() {}
+
+Options
+-------
+
+.. option:: HeaderFileExtensions
+
+   A semicolon-separated list of filename extensions of header files (the filename
+   extensions should not include "." prefix). Default is ";h;hh;hpp;hxx".
+   For extension-less header files, using an empty string or leaving an
+   empty string between ";" if there are other filename extensions.

clang-tools-extra/test/clang-tidy/checkers/misc/static-declaration-in-header.hpp

+// RUN: %check_clang_tidy %s misc-static-declaration-in-header %t
+
+static int v1 = 123;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: static declaration in header; remove 'static' [misc-static-declaration-in-header]
+// CHECK-FIXES: {{^}}int v1 = 123;
+
+static const int c1 = 123;
+// CHECK-MESSAGES: :[[@LINE-1]]:18: warning: static declaration in header; remove 'static'
+// CHECK-FIXES: {{^}}const int c1 = 123;
+
+static constexpr int c2 = 123;
+// CHECK-MESSAGES: :[[@LINE-1]]:22: warning: static declaration in header; remove 'static'
+// CHECK-FIXES: {{^}}constexpr int c2 = 123;
+
+static void f1(){}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: static declaration in header; remove 'static'
+// CHECK-FIXES: {{^}}void f1(){}
+
+namespace foo {
+  static int v2 = 123;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header; remove 'static'
+  // CHECK-FIXES: {{^}}  int v2 = 123;
+
+  static int f2(){}
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header; remove 'static'
+  // CHECK-FIXES: {{^}}  int f2(){}
+}
+
+namespace {
+  static int v3 = 123;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header; remove 'static'
+  // CHECK-FIXES: {{^}}  int v3 = 123;
+
+  static int f3(){}
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header; remove 'static'
+  // CHECK-FIXES: {{^}}  int f3(){}
+}
+
+// OK
+struct Foo {
+  static const int v4 = 123;
+  static void f4(){}
+};
+
+// OK
+void f5() {
+  static int v5;
+}