[Clang-tidy] Add a check for definitions in the global namespace.

[bkramer]

Summary

This is prone to ODR violations and generally frowned upon in many
codebases (e.g. LLVM). The checker flags definitions, variables and
classes in the global namespace. Common false positives like extern "C"
functions are filtered, symbols with internal linkage or hidden
visibility are not warned on either.

On LLVM most of the instances are helper functions that should be just
made static or globals that belong into a namespace.

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

[bkramer]

Add relnote.

[alexfh]

Do we want to merge this check with google/GlobalNamesInHeadersCheck.cpp that targets a similar set of issues?

[bkramer]

DefinitionsInHeaders is tackling a different problem. IMO DefinitionsInHeaders is something that should be on by default everywhere, while this check for definitions in the global namespace is more of a coding style issue.

GlobalNamesInHeaders is a bit of a misnomer, it looks for using declarations in headers. I don't think it makes sense to merge this check into it either, with the new check we mostly find things exported by accident in .cpp files, it has little to do with headers.

I'm welcome to naming suggestions for this check, btw ;)

[alexfh]

In D23130#505242, @bkramer wrote:

DefinitionsInHeaders is tackling a different problem. IMO DefinitionsInHeaders is something that should be on by default everywhere, while this check for definitions in the global namespace is more of a coding style issue.

Yes, DefinitionsInHeaders is a different beast.

GlobalNamesInHeaders is a bit of a misnomer, it looks for using declarations in headers.

The google-global-names-in-headers check aims to "flag global namespace pollution in header files" including declarations of classes, functions and other entities. However, "right now it only triggers on using declarations and directives", so it's basically under-implemented. The only problem, I guess, is keeping false positives under control. The check you're proposing is somewhat close. It isn't specifically limited to headers (the google-global-names-in-headers check can run on all files if properly configured, and we can rename it to remove the -in-headers part). And the second difference is that it is limited to definitions, but I don't yet understand, why it is important, since declarations in the global namespace (except for using declarations, typedefs, etc.) will have a corresponding definition in the global namespace. I guess, these checks, if somewhat different, may be similar enough to share some implementation.

[Prazek]

clang-tidy/misc/GlobalNamespaceCheck.cpp

46

I think it would be better to check it in matcher.
I see that there is isExternC, but it only works for FunctionDecl, but I think that adding isExternC for VarDecl would be good

67

What about macros? I think you should check isMacroId on location here (don't do fixit when it is in macro, but print warning)

Also please add tests for it.

[bkramer]

In D23130#505290, @alexfh wrote:

And the second difference is that it is limited to definitions, but I don't yet understand, why it is important, since declarations in the global namespace (except for using declarations, typedefs, etc.) will have a corresponding definition in the global namespace. I guess, these checks, if somewhat different, may be similar enough to share some implementation.

My reasoning was to avoid duplicated warnings (on both declaration and definition) and false positives on headers importing C symbols. I think that the latter would trigger an explosion in false positives :(

clang-tidy/misc/GlobalNamespaceCheck.cpp

46

I'll fix this once we figured out where to put this check.

67

The FixIt isn't actually implemented. Adding FixIts for this check is something I'm planning to do, but it's not part of this patch and doesn't make sense to implement before we know that this check is actually working properly.

[aaron.ballman]

clang-tidy/misc/GlobalNamespaceCheck.cpp

72

ODR violations instead of ODR conflicts?

docs/clang-tidy/checks/misc-global-namespace.rst

7

violations instead of conflicts. Also, expanding the documentation a bit more would be useful -- not everyone understands what an ODR violation is, why it would be bad, etc. It can even be as simple as adding in an external link for more info.

test/clang-tidy/misc-global-namespace.cpp

13

Add a case for extern "C" int i;?

Also, isn't this a definition with external linkage in C++ (or is it only in C)? extern int i = 30; Assuming it's a definition, should this also trigger the diagnostic?

[bkramer]

• Merge google-global-names-in-headers and misc-global-namespace into a new check google-global-names.

[alexfh]

Could you run the check on LLVM and post a summary of results?

clang-tidy/google/GlobalNamesCheck.cpp

90 ↗

(On Diff #66792)

Is this already filtered-out by the matcher?

114 ↗

(On Diff #66792)

The warning explains, what's wrong, but doesn't tell why. We don't need the level of details of the documentation here, but maybe expand the message a bit to cover some of the possible effects (e.g. "this can cause ODR violations" or something like this)?

docs/ReleaseNotes.rst

62

Please mention the old check name as well.

docs/clang-tidy/checks/google-global-names.rst

4 ↗

(On Diff #66792)

Cut extra =s.

[bkramer]

Address review comments.

[bkramer]

In D23130#505769, @alexfh wrote:

Could you run the check on LLVM and post a summary of results?

I have a full list at https://reviews.llvm.org/P7085. It's huge.

[aaron.ballman]

For cases where the external linkage is desired, how would you go about silencing this diagnostic?

test/clang-tidy/google-global-names.cpp

13–14 ↗

(On Diff #66800)

This strikes me as being intentional enough to warrant not diagnosing because of the extern keyword.

[bkramer]

test/clang-tidy/google-global-names.cpp

13–14 ↗

(On Diff #66800)

The only case I see where this pattern is valuable is interfacing with C code. Not sure yet if we want to allow that or enforce extern "C" instead. Ideas?

an extern global in the global namespace still feels like something we should warn on :|

[aaron.ballman]

test/clang-tidy/google-global-names.cpp

13–14 ↗

(On Diff #66800)

Yet externs in the global namespace do happen for valid reasons (such as not breaking ABIs by putting the extern definition into a namespace or changing the language linkage) -- I'm trying to think of ways we can allow the user to silence this diagnostic in those cases. I feel like in cases where the user writes "extern", they're explicitly specifying their intent and that doesn't seem like a case to warn them about, in some regards. It would give us two ways to silence the diagnostic (well, three, but two are morally close enough):

1. Put it into a namespace
2. Slap extern on it if it is global for C++ compatibility (such as ABIs)
3. Slap extern "C" on it if it global for C compatibility

I suppose we could require extern "C++" instead of extern, but I don't think that's a particularly common use of the language linkage specifier?

[alexfh]

Benjamin, what's the plan here?

[bkramer]

Update to head

[bkramer]

In D23130#588681, @alexfh wrote:

Benjamin, what's the plan here?

I still think this check is useful, particularly for LLVM. I also don't think any of the existing review comments still apply or have ever applied in the first place, so I rebased this onto head, it's up for review again.

[aaron.ballman]

clang-tidy/google/GlobalNamesCheck.cpp

77 ↗

(On Diff #77216)

Is this formatting that clang-format generates?

test/clang-tidy/google-global-names.cpp

18 ↗

(On Diff #77216)

Can you also add a test:

extern "C++" void h2() {}

I believe this will diagnose, but whether it should diagnose or not, I'm less certain of.

13–14 ↗

(On Diff #66800)

I still think that a user explicitly writing 'extern' is expecting external linkage and all that goes along with it.

[alexfh]

and generally frowned upon in many codebases (e.g. LLVM)

Should it still be a part of google/? The old check was enforcing a part of the Google C++ style guide, but the new one seems to be somewhat broader. Am I mistaken?

[bkramer]

Add extern "C++" test case.

[bkramer]

In D23130#589643, @alexfh wrote:

and generally frowned upon in many codebases (e.g. LLVM)

Should it still be a part of google/? The old check was enforcing a part of the Google C++ style guide, but the new one seems to be somewhat broader. Am I mistaken?

The google style guide has a rule that code should usually be placed in a namespace. The wording there is fuzzy, but I believe this check still belongs under google/

clang-tidy/google/GlobalNamesCheck.cpp

77 ↗

(On Diff #77216)

Yes. I clang-formatted the entire file.

test/clang-tidy/google-global-names.cpp

18 ↗

(On Diff #77216)

Test added, doesn't diagnose.

13–14 ↗

(On Diff #66800)

I disagree. If this is a special variable to be accessed via dlopen it should be extern "C". If not it should be in a namespace.

[malcolm.parsons]

clang-tidy/google/GlobalNamesCheck.cpp

96 ↗

(On Diff #77410)

Should isMSVCRTEntryPoint() be checked too?

[aaron.ballman]

test/clang-tidy/google-global-names.cpp

13–14 ↗

(On Diff #66800)

I'm thinking more that it's not a special variable to be accessed via dlopen, but instead is a special implementation detail that doesn't need to be exposed via a header file. For instance, see ClangTidyMain.cpp. Such uses have no way to silence this check, which may be fine since it's in the Google module. I merely wanted to point it out because it does happen for valid reasons, and having a way to silence a diagnostic aside from NOLINT is nice.

[bkramer]

test/clang-tidy/google-global-names.cpp

13–14 ↗

(On Diff #66800)

That's an example of something that should be in a namespace. Currently it's polluting the global namespace which will break anyone who happens to use the same name for a symbol anywhere else. I think that flagging those cases is useful.

[aaron.ballman]

clang-tidy/google/GlobalNamesCheck.cpp

62 ↗

(On Diff #77410)

s/unless/Unless

84 ↗

(On Diff #77410)

I wonder if this check would have better performance by making this a local AST matcher instead? UsingDecl and UsingDirectiveDecl do not have linkage, so it may require adjusting the matcher somewhat.

96 ↗

(On Diff #77410)

Yes, it should.

clang-tidy/google/GoogleTidyModule.cpp

68 ↗

(On Diff #77410)

Given that this was shipped under the old name, I think we need to figure out our policy for how to handle this. It also comes up in D26511, so I would like us to be consistent with what we do.

test/clang-tidy/google-global-names.cpp

13–14 ↗

(On Diff #66800)

Okay, that's fair. I was thinking that since it was in the global namespace, it was there for a reason. After looking a bit more closely, I think you're correct, this would be a true-positive. You've convinced me and I retract my concerns. :-)

[bkramer]

• Moved external linkage check to matcher
• added msvcrt entry point check
• fixed comment typos.

[bkramer]

clang-tidy/google/GlobalNamesCheck.cpp

90 ↗

(On Diff #66792)

Nope.

clang-tidy/google/GoogleTidyModule.cpp

68 ↗

(On Diff #77410)

There was no special handling in D26511, so I left this as-is.

[malcolm.parsons]

clang-tidy/google/GoogleTidyModule.cpp

68 ↗

(On Diff #77410)

D26511 made the existing check documentation redirect to the new documentation.

[alexfh]

It looks like all concerns were resolved.

LG, if you still want to proceed with this patch.

[bkramer]

I'd like to, but I don't know when I find time to rebase this thing after more than a year of waiting for review.

[alexfh]

In D23130#1037256, @bkramer wrote:

I'd like to, but I don't know when I find time to rebase this thing after more than a year of waiting for review.

Sorry, it was just lost ¯\_(ツ)_/¯. I may be relying on pings from the other side too much.

[shafik]

Is this still relevant? It looks like some of these changes made it in.

clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
+  GlobalNamespaceCheck.cpp
   MisplacedConstCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp
   DefinitionsInHeadersCheck.cpp
   FoldInitTypeCheck.cpp
   ForwardDeclarationNamespaceCheck.cpp
   InaccurateEraseCheck.cpp

clang-tidy/misc/GlobalNamespaceCheck.h

+//===--- GlobalNamespaceCheck.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_MISC_GLOBAL_NAMESPACE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_GLOBAL_NAMESPACE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Finds definitions in the global namespace. Those definitions are prone to
+/// ODR conflicts.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-global-namespace.html
+class GlobalNamespaceCheck : public ClangTidyCheck {
+public:
+  GlobalNamespaceCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_GLOBAL_NAMESPACE_H

clang-tidy/misc/GlobalNamespaceCheck.cpp

+//===--- GlobalNamespaceCheck.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 "GlobalNamespaceCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void GlobalNamespaceCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(
+      namedDecl(hasDeclContext(translationUnitDecl()),
+                anyOf(functionDecl(isDefinition(), unless(isDeleted()),
+                                   unless(isExternC())),
+                      varDecl(isDefinition()), cxxRecordDecl(isDefinition())))
+          .bind("decl"),
+      this);
+}
+
+void GlobalNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<NamedDecl>("decl");
+
+  // Ignore decls with internal linkage. Hidden visibility can still lead to ODR
+  // violations but cannot cross DSO boundaries, so it's often used to separate
+  // namespaces, ignore those too.
+  LinkageInfo LV = MatchedDecl->getLinkageAndVisibility();
+  if (!isExternallyVisible(LV.getLinkage()) ||
+      LV.getVisibility() == HiddenVisibility)
+    return;
+
+  if (const auto *VDecl = dyn_cast<VarDecl>(MatchedDecl)) {
+    // extern "C" globals need to be in the global namespace.
+    if (VDecl->isExternC())
+      return;
+  }
+
+  if (const auto *RDecl = dyn_cast<CXXRecordDecl>(MatchedDecl)) {
+    // Ignore C-like records. Those are often used in C wrappers and don't
+    // contain anything with linkage.
+    if (RDecl->isCLike())
+      return;
+  }
+
+  if (const auto *FDecl = dyn_cast<FunctionDecl>(MatchedDecl)) {
+    // main() should be in the global namespace.
+    if (FDecl->isMain())
+      return;
+
+    // Ignore overloaded operators.
+    // FIXME: Should this only ignore global operator new/delete?
+    if (FDecl->isOverloadedOperator())
+      return;
+  }
+
+  // FIXME: If we want to be really fancy we could move declaration-less
+  // definitions into a global namespace or make them static with a FixIt.
+  diag(MatchedDecl->getLocation(), "definition for %0 in the global namespace; "
+                                   "move it into a namespace or give it "
+                                   "internal linkage to avoid ODR conflicts")
+      << MatchedDecl;
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tidy/misc/MiscTidyModule.cpp

 //===--- MiscTidyModule.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 "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
-#include "MisplacedConstCheck.h"
-#include "UnconventionalAssignOperatorCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "DanglingHandleCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
+#include "GlobalNamespaceCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
+#include "MisplacedConstCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "MultipleStatementMacroCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "PointerAndIntegralOperationCheck.h"
 #include "RedundantExpressionCheck.h"
 #include "SizeofContainerCheck.h"
 #include "SizeofExpressionCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringConstructorCheck.h"
 #include "StringIntegerAssignmentCheck.h"
 #include "StringLiteralWithEmbeddedNulCheck.h"
 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowByValueCatchByReferenceCheck.h"
+#include "UnconventionalAssignOperatorCheck.h"
 #include "UndelegatedConstructor.h"
 #include "UniqueptrResetReleaseCheck.h"
 #include "UnusedAliasDeclsCheck.h"
 #include "UnusedParametersCheck.h"
 #include "UnusedRAIICheck.h"
 #include "UnusedUsingDeclsCheck.h"
 #include "VirtualNearMissCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 class MiscModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
+    CheckFactories.registerCheck<GlobalNamespaceCheck>("misc-global-namespace");
     CheckFactories.registerCheck<MisplacedConstCheck>(
         "misc-misplaced-const");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(
         "misc-bool-pointer-implicit-conversion");
     CheckFactories.registerCheck<DanglingHandleCheck>(
         "misc-dangling-handle");

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - New `cppcoreguidelines-slicing
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-slicing.html>`_ check
 
   Flags slicing of member variables or vtable.
 
 - New `cppcoreguidelines-special-member-functions
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html>`_ check
 
   Flags classes where some, but not all, special member functions are user-defined.
 
+- New `misc-global-namespace
+  <http://clang.llvm.org/extra/clang-tidy/checks/misc-global-namespace.html>`_ check
+
+  Flags definitions in the global namespace.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/list.rst

.. toctree::
    llvm-twine-local
    misc-argument-comment
    misc-assert-side-effect
    misc-bool-pointer-implicit-conversion
    misc-dangling-handle
    misc-definitions-in-headers
    misc-fold-init-type
    misc-forward-declaration-namespace
+   misc-global-namespace
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
    misc-misplaced-const
    misc-misplaced-widening-cast
    misc-move-const-arg

docs/clang-tidy/checks/misc-global-namespace.rst

+.. title:: clang-tidy - misc-global-namespace
+
+misc-global-namespace
+=====================
+
+Finds definitions in the global namespace. Those definitions are prone to ODR
+conflicts.

test/clang-tidy/misc-global-namespace.cpp

+// RUN: %check_clang_tidy %s misc-global-namespace %t
+
+void f() {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: definition for 'f' in the global namespace
+class F {};
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: definition for 'F' in the global namespace
+int i;
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: definition for 'i' in the global namespace
+
+// No warnings below.
+void g();
+
+extern "C" void h() {}
+
+extern "C" {
+void j() {}
+}
+
+struct Clike {
+  int i;
+};
+
+void *operator new(__SIZE_TYPE__, int) { return 0; }
+void operator delete(void *, int) {}
+
+__attribute__((visibility("hidden"))) void k() {}
+static void l() {}
+namespace {
+void m() {}
+}
+namespace x {
+void n() {}
+}
+
+int main() {}