[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

33

Spurious newline.

42

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.

44–45

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

clang-tidy/abseil/AnonymousEnclosedAliasesCheck.h

21

Detecting incorrect -> Detecting the incorrect

22

when there exists one -> when one exists

[Naysh]

clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp

42

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

42

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

 //===------- AbseilTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "AnonymousEnclosedAliasesCheck.h"
 #include "DurationAdditionCheck.h"
 #include "DurationComparisonCheck.h"
 #include "DurationConversionCastCheck.h"
 #include "DurationDivisionCheck.h"
 #include "DurationFactoryFloatCheck.h"
 #include "DurationFactoryScaleCheck.h"
 #include "DurationSubtractionCheck.h"
 #include "DurationUnnecessaryConversionCheck.h"
 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
 #include "TimeSubtractionCheck.h"
 #include "UpgradeDurationConversionsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AnonymousEnclosedAliasesCheck>(
+        "abseil-anonymous-enclosed-aliases");
     CheckFactories.registerCheck<DurationAdditionCheck>(
         "abseil-duration-addition");
     CheckFactories.registerCheck<DurationComparisonCheck>(
         "abseil-duration-comparison");
     CheckFactories.registerCheck<DurationConversionCastCheck>(
         "abseil-duration-conversion-cast");
     CheckFactories.registerCheck<DurationDivisionCheck>(
         "abseil-duration-division");

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 types of nodes:
+  // 1. anonymous namespace declarations,
+  // 2. using declarations that are not inside an anonymous namespace and
+  // not inside any other scope.
+  Finder->addMatcher(
+      namespaceDecl(allOf(namespaceDecl(isAnonymous()),
+                          namespaceDecl(isExpansionInMainFile())))
+          .bind("anonymous_namespace"),
+      this);
+  Finder->addMatcher(
+      usingDecl(
+          allOf(usingDecl(isExpansionInMainFile()),
+                usingDecl(unless(hasAncestor(functionDecl()))),
+                usingDecl(unless(hasAncestor(cxxRecordDecl()))),
+                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(),
+           "using declaration %0 should be in the anonymous namespace. "
+           "Use discretion when moving using declarations as it might "
+           "necessitate moving lines containing relevant aliases.")
+          << 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(),
+         "using declaration %0 should be in the anonymous namespace. "
+         "Use discretion when moving using declarations as it might "
+         "necessitate moving lines containing relevant aliases.")
+        << CurrentUsingDecl;
+  }
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tidy/abseil/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  AnonymousEnclosedAliasesCheck.cpp
   DurationAdditionCheck.cpp
   DurationComparisonCheck.cpp
   DurationConversionCastCheck.cpp
   DurationDivisionCheck.cpp
   DurationFactoryFloatCheck.cpp
   DurationFactoryScaleCheck.cpp
   DurationRewriter.cpp
   DurationSubtractionCheck.cpp

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 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-addition
   <clang-tidy/checks/abseil-duration-addition>` check.
 
   Checks for cases where addition should be performed in the ``absl::Time``
   domain.
 
 - New :doc:`abseil-duration-conversion-cast
   <clang-tidy/checks/abseil-duration-conversion-cast>` 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

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
+   abseil-anonymous-enclosed-aliases
    abseil-duration-addition
    abseil-duration-comparison
    abseil-duration-conversion-cast
    abseil-duration-division
    abseil-duration-factory-float
    abseil-duration-factory-scale
    abseil-duration-subtraction
    abseil-duration-unnecessary-conversion

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

+// RUN: %check_clang_tidy %s abseil-anonymous-enclosed-aliases %t
+namespace bar {
+
+class A {};
+class B {};
+
+} // namespace bar
+
+namespace foo1 {
+
+// CHECK-MESSAGES: :[[@LINE+4]]:12: warning: using declaration 'A' should
+// be in the anonymous namespace. Use discretion when moving using declarations
+// as it might necessitate moving lines containing relevant aliases.
+// [abseil-anonymous-enclosed-aliases]
+using bar::A;
+void f(A a);
+
+namespace {} // anonymous namespace
+
+} // namespace foo1
+
+namespace foo2 {
+
+namespace {
+
+// This is okay
+using ::bar::B;
+
+} // anonymous namespace
+
+void g(B b);
+
+} // namespace foo2
+
+// Check should not be triggered below when the using declaration is at
+// function or class (instead of namespace) scope.
+namespace outer {
+
+int fun_scope() {
+  using ::bar::A;
+  return 0;
+} // function scope
+
+class Base {
+public:
+  void f();
+}; // class scope
+
+class Derived : public Base {
+public:
+  using Base::f;
+}; // class scope
+
+namespace {} // anonymous namespace
+
+} // namespace outer
 No newline at end of file