[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

110

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.
+///
+/// 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);
+  void diagWithFixIt(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_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
+
+static StringRef WarningMessage = "static declaration in header";
+
+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");
+}
+
+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;
+  }
+}
+
+void StaticDeclarationInHeaderCheck::diagWithFixIt(const NamedDecl *MatchedDecl,
+                                                   const SourceManager &SM) {
+  diag(MatchedDecl->getLocation(), WarningMessage)
+      << FixItHint::CreateInsertion(MatchedDecl->getSourceRange().getBegin(),
+                                    "inline")
+      << FixItHint::CreateRemoval(getStaticSourceRange(MatchedDecl, SM));
+}
+
+void StaticDeclarationInHeaderCheck::storeOptions(
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "HeaderFileExtensions", RawStringHeaderFileExtensions);
+}
+
+void StaticDeclarationInHeaderCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      functionDecl(isStaticStorageClass(), isInHeaderFile(HeaderFileExtensions),
+                   unless(anyOf(isMemberFunction(), isInline(), isConstexpr(),
+                                isInAnonymousNamespace())))
+          .bind("x"),
+      this);
+  Finder->addMatcher(
+      varDecl(isStaticStorageClass(), isInHeaderFile(HeaderFileExtensions),
+              unless(anyOf(hasType(isConstQualified()), isStaticLocal(),
+                           isStaticDataMember(), isInline(),
+                           isInAnonymousNamespace())))
+          .bind("x"),
+      this);
+}
+
+void StaticDeclarationInHeaderCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<NamedDecl>("x");
+  if (!MatchedDecl)
+    return;
+
+  const SourceManager *SM = Result.SourceManager;
+  if (!SM)
+    return;
+
+  if (llvm::isa<FunctionDecl>(MatchedDecl))
+    diagWithFixIt(MatchedDecl, *SM);
+  else if (llvm::isa<VarDecl>(MatchedDecl)) {
+    if (Result.Context->getLangOpts().CPlusPlus17)
+      diagWithFixIt(MatchedDecl, *SM);
+    else
+      diag(MatchedDecl->getLocation(), WarningMessage);
+  }
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-unsafe-functions
 
   Checks for functions that have safer, more secure replacements available, or
   are considered deprecated due to design flaws.
   This check relies heavily on, but is not exclusive to, the functions from
   the *Annex K. "Bounds-checking interfaces"* of C11.
 
 - New :doc:`cppcoreguidelines-avoid-capture-default-when-capturing-this
   <clang-tidy/checks/cppcoreguidelines/avoid-capture-default-when-capturing-this>` check.
 
   Warns when lambda specify a capture default and capture ``this``.
 
+- 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 check aliases
 ^^^^^^^^^^^^^^^^^
 
 - New alias :doc:`cert-msc24-c
   <clang-tidy/checks/cert/msc24-c>` to :doc:`bugprone-unsafe-functions
   <clang-tidy/checks/bugprone/unsafe-functions>` was added.
 
 - New alias :doc:`cert-msc33-c

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.
+
+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
+  static void f(){}
+  static int x = 123;
+
+The code should instead be changed to replace ``static`` with ``inline``, for
+both function and variable declarations (since C++17):
+
+.. code-block:: c++
+
+  // foo.h
+  inline void f(){}
+  inline int x = 123;  // Since C++17
+
+The check will issue automatic fixes if supported.
+
+Alternatively, the code could be changed to remove ``static`` and move the
+definitions to a separate translation unit.
+
+.. code-block:: c++
+
+  // foo.h
+  void f();
+  int x;
+
+  // foo.cpp
+  void f(){}
+  int x = 123;
+
+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                              -std=c++98,c++03,c++11,c++14 %s misc-static-declaration-in-header %t
+// RUN: %check_clang_tidy -check-suffix=CXX17-OR-LATER -std=c++17-or-later          %s misc-static-declaration-in-header %t
+
+static int v1 = 123;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: static declaration in header [misc-static-declaration-in-header]
+// CHECK-FIXES-CXX17-OR-LATER: {{^}}inline int v1 = 123;{{$}}
+
+static void f1(){}
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: static declaration in header
+// CHECK-FIXES: {{^}}inline void f1(){}{{$}}
+
+namespace foo {
+  static int v2 = 123;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header
+  // CHECK-FIXES-CXX17-OR-LATER: {{^}}  inline int v2 = 123;{{$}}
+
+  static int f2(){}
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: static declaration in header
+  // CHECK-FIXES: {{^}}  inline int f2(){}{{$}}
+}
+
+// OK
+struct Foo {
+  static const int v3 = 123;
+  static void f3(){}
+};
+
+// OK
+void f4() {
+  static int v4 = 123;
+}
+
+// OK, static doesn't make matters worse here due to the anonymous namespace
+namespace {
+  static int v5 = 123;
+  static int f5(){}
+}
+
+// OK, static is only redundant (readability)
+static const int v6 = 123;
+static inline void f6(){}
+
+#if __cplusplus >= 201103L
+static constexpr int v7 = 123;
+static constexpr int f7(){ return 123; }
+#endif