[clang-tidy] check for using declarations not in an anonymous namespace when there exists one

[Ywicheng]

Summary

This patch adds one check corresponding to the Unnamed Namespace section in https://abseil.io/tips/119.

In particular, if there exists an anonymous, it is better to put all aliases there.

[Ywicheng]

Updated abseil-anonymous-enclosed-alises.rst and ReleaseNotes.rst

[JonasToth]

Please upload with full context.

[aaron.ballman]

clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp

32

Spurious newline.

41

I don't think this logic works in practice because there's no way to determine that the anonymous namespace is even a candidate for putting the using declaration into it. Consider a common scenario where there's an anonymous namespace declared in a header file (like an STL header outside of the user's control), and a using declaration inside of an implementation file. Just because the STL declared an anonymous namespace doesn't mean that the user could have put their using declaration in it.

43–44

Diagnostics should not be complete sentences or contain hyperlinks. (Same below.)

clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h

20

Detecting incorrect -> Detecting the incorrect

21

when there exists one -> when one exists

[Naysh]

clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp

41

If we altered the check to only apply to anonymous namespaces and using declarations at namespace scope (that is, we only suggest aliases be moved to anonymous namespaces when the unnamed namespace and alias are themselves inside some other namespace), would this issue be resolved?

[aaron.ballman]

clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp

41

If they're inside the same namespace, then I think that could work, but if the namespaces are different I don't know that you can be sure the alias can be moved into the anonymous namespace.

clang-tidy/abseil/AbseilTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "AnonymousEnclosedAliasesCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"

 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AnonymousEnclosedAliasesCheck>(
+        "abseil-anonymous-enclosed-aliases");
     CheckFactories.registerCheck<DurationDivisionCheck>(
         "abseil-duration-division");
     CheckFactories.registerCheck<DurationFactoryFloatCheck>(

clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h

+//===--- AnonymousEnclosedAliasesCheck.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_ABSEIL_ANONYMOUSENCLOSEDALIASESCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_ANONYMOUSENCLOSEDALIASESCHECK_H
+
+#include "../ClangTidy.h"
+#include <vector>
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Detecting incorrect practice of putting using declarations outside an
+/// anonymous namespace when there exists one.
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/
+/// abseil-anonymous-enclosed-aliases.html
+class AnonymousEnclosedAliasesCheck : public ClangTidyCheck {
+public:
+  AnonymousEnclosedAliasesCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+private:
+  const NamespaceDecl* AnonymousNamespaceDecl;
+  std::vector<const UsingDecl*> MatchedUsingDecls;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_ANONYMOUSENCLOSEDALIASESCHECK_H

clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp

+//===--- AnonymousEnclosedAliasesCheck.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 "AnonymousEnclosedAliasesCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+void AnonymousEnclosedAliasesCheck::registerMatchers(MatchFinder *Finder) {
+  // We try to match two nodes:
+  // 1. anonymous namespace declarations,
+  // 2. using declarations that are not inside an anonymous declaration
+  Finder->addMatcher(
+    namespaceDecl(isAnonymous()).bind("anonymous_namespace"), this);
+  Finder->addMatcher(
+    usingDecl(unless(hasAncestor(
+      namespaceDecl(isAnonymous())))).bind("using_decl"), this);
+}
+
+void AnonymousEnclosedAliasesCheck::check(const MatchFinder::MatchResult &Result) {
+
+  const auto *MatchedUsingDecl =
+    Result.Nodes.getNodeAs<UsingDecl>("using_decl");
+  // If a potential using declaration is matched,
+  if (MatchedUsingDecl) {
+    // and if an anonymous namespace declaration has already been found,
+    // the matched using declaration is a target, and we print out
+    // the diagnostics for it. Otherwise, we add the using declaration
+    // to the vector containing all candidate using declarations.
+    if (AnonymousNamespaceDecl) {
+      diag(MatchedUsingDecl->getLocation(),
+        "UsingDecl %0 should be in the anonymous namespace. See: "
+        "https://abseil.io/tips/119")
+        << MatchedUsingDecl;
+    } else {
+      MatchedUsingDecls.push_back(MatchedUsingDecl);
+    }
+    return;
+  }
+  // Otherwise, an anonymous namespace declaration is matched. In this case,
+  // all the previously matched namespace declarations in the vector
+  // CurrentUsingDecl are our targets, and we print out the
+  // diagnostics for all of them.
+  AnonymousNamespaceDecl =
+    Result.Nodes.getNodeAs<NamespaceDecl>("anonymous_namespace");
+  for (const auto* CurrentUsingDecl: MatchedUsingDecls) {
+    diag(CurrentUsingDecl->getLocation(),
+      "UsingDecl %0 should be in the anonymous namespace. See: "
+      "https://abseil.io/tips/119")
+      << CurrentUsingDecl;
+  }
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tidy/abseil/CMakeLists.txt

 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  AnonymousEnclosedAliasesCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   FasterStrsplitDelimiterCheck.cpp

docs/ReleaseNotes.rst

 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`abseil-anonymous-enclosed-aliases
+  <clang-tidy/checks/abseil-anonymous-enclosed-aliases>` check.
+
+  Finds instances of using declarations not in an anonymous namespace
+  when there exists one.
+
 - New :doc:`abseil-duration-division
   <clang-tidy/checks/abseil-duration-division>` check.
 

docs/clang-tidy/checks/abseil-anonymous-enclosed-aliases.rst

+.. title:: clang-tidy - abseil-anonymous-enclosed-aliases
+
+abseil-anonymous-enclosed-aliases
+=================================
+
+Finds using declarations outside of anonymous namespaces, and
+suggests those declarations be moved to that namespace.
+
+Example:
+.. code-block:: c++
+
+  namespace foo {
+  
+  using something; // should be inside the anonymous namespace below
+
+  namespace {
+
+  } // anonymous namespace
+
+  } // foo

docs/clang-tidy/checks/list.rst

 =================
 
 .. toctree::
+   abseil-anonymous-enclosed-aliases
    abseil-duration-division
    abseil-duration-factory-float
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls
-   abseil-string-find-startswith
    abseil-str-cat-append
+   abseil-string-find-startswith
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat

    hicpp-special-member-functions (redirects to cppcoreguidelines-special-member-functions) <hicpp-special-member-functions>
    hicpp-static-assert (redirects to misc-static-assert) <hicpp-static-assert>
    hicpp-undelegated-constructor (redirects to bugprone-undelegated-constructor) <hicpp-undelegated-constructor>
+   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) <hicpp-uppercase-literal-suffix>
    hicpp-use-auto (redirects to modernize-use-auto) <hicpp-use-auto>
    hicpp-use-emplace (redirects to modernize-use-emplace) <hicpp-use-emplace>
    hicpp-use-equals-default (redirects to modernize-use-equals-default) <hicpp-use-equals-default>

    hicpp-use-nullptr (redirects to modernize-use-nullptr) <hicpp-use-nullptr>
    hicpp-use-override (redirects to modernize-use-override) <hicpp-use-override>
    hicpp-vararg (redirects to cppcoreguidelines-pro-type-vararg) <hicpp-vararg>
-   hicpp-uppercase-literal-suffix (redirects to readability-uppercase-literal-suffix) <hicpp-uppercase-literal-suffix>
    llvm-header-guard
    llvm-include-order
    llvm-namespace-comment

test/clang-tidy/abseil-anonymous-enclosed-aliases.cpp

+// RUN: %check_clang_tidy %s abseil-anonymous-enclosed-aliases %t
+namespace bar {
+
+class something {
+
+};
+}
+
+namespace foo {
+
+using bar::something;
+
+namespace {
+
+}  // anonymous namespace
+}  // namespace foo
+// CHECK-MESSAGES: :[[@LINE-6]]:12: warning: UsingDecl 'something' should be in the anonymous namespace. See: https://abseil.io/tips/119 [abseil-anonymous-enclosed-aliases]
+
+
+namespace foo {
+
+namespace {
+
+using ::bar::something;
+
+}  // anonymous namespace
+}  // namespace foo