[clang-tidy] check for flagging using declarations not in the inner most namespace

[Ywicheng]

Summary

This patch adds one check corresponding to the Scope of the Alias section in https://abseil.io/tips/119.

In particular, it is better to put aliases in the innermost namespace when there is any such scope.

[JonasToth]

Please upload the patch with full context.

[aaron.ballman]

clang-tidy/abseil/SafelyScopedCheck.cpp

23

Why is this not also checking that the using declaration is not within a header file?

25–28

Did clang-format produce this? If not, you should run the patch through clang-format.

33

I suspect this diagnostic is going to be too aggressive and once you add more tests will find it needs some additional constraints.

33–34

Do not add links to diagnostic messages. They should stand on their own, and not be grammatical with punctuation. I would suggest: using declaration not declared in the innermost namespace

test/clang-tidy/abseil-safely-scoped.cpp

2

You're missing a lot of tests, such as using declarations at function scope, within a class scope, etc.

[aaron.ballman]

clang-tidy/abseil/SafelyScopedCheck.cpp

38

You should remove the full stop at the end of the diagnostic.

test/clang-tidy/abseil-safely-scoped.cpp

16–17

This should be moved closer to where the actual warning will show up.

However, the diagnostic doesn't help me to understand what is wrong with the code or how to fix it. I think it might be telling me that the using declaration is supposed to be inside of the anonymous namespace, but moving the using declaration there will break the declaration of f().

[aaron.ballman]

clang-tidy/abseil/SafelyScopedCheck.cpp

23

I'm still wondering about this. The Abseil tip specifically says Never declare namespace aliases or convenience using-declarations at namespace scope in header files, only in .cc files., which is why I had figured I would see some enforcement from this check.

38

The diagnostic still doesn't meet our (somewhat strange) requirements; diagnostics should not be full sentences with capitalization and punctuation. However, it also doesn't really tell the user what is wrong with their code, just how to fix it to make the diagnostic go away.

I don't have a particularly great suggestion for a replacement, however. I'm still trying to wrap my mind around the suggested changes to code, because it seems like this advice isn't general purpose. Consider:

namespace Foo { // I control this namespace.
  namespace details {
    int func(int I);
  } // namespace details

  using Frobble::Bobble;

  void f(SomeNameFromFrobbleBobble FB) {
    FB.whatever(details::func(12));
  }
} // namespace Foo

The suggestion to move the using declaration into the innermost namespace is actually a poor one -- we don't want the interface of f() to be void f(details::SomeNameFromFrobbleBobble FB), which is what this check seems to suggest we do. I don't see how we can determine whether the using declaration should or should not be moved, so I can't really tell what the diagnostic text should be.

clang-tidy/abseil/AbseilTidyModule.cpp

 #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 "SafelyScopedCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"
 #include "TimeSubtractionCheck.h"
 #include "UpgradeDurationConversionsCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace abseil {
CheckFactories.registerCheck<DurationUnnecessaryConversionCheck>(
         "abseil-duration-unnecessary-conversion");
     CheckFactories.registerCheck<FasterStrsplitDelimiterCheck>(
         "abseil-faster-strsplit-delimiter");
     CheckFactories.registerCheck<NoInternalDependenciesCheck>(
         "abseil-no-internal-dependencies");
     CheckFactories.registerCheck<NoNamespaceCheck>("abseil-no-namespace");
     CheckFactories.registerCheck<RedundantStrcatCallsCheck>(
         "abseil-redundant-strcat-calls");
+    CheckFactories.registerCheck<SafelyScopedCheck>(
+        "abseil-safely-scoped");
     CheckFactories.registerCheck<StrCatAppendCheck>(
         "abseil-str-cat-append");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
     CheckFactories.registerCheck<TimeSubtractionCheck>(
         "abseil-time-subtraction");
     CheckFactories.registerCheck<UpgradeDurationConversionsCheck>(
         "abseil-upgrade-duration-conversions");

clang-tidy/abseil/CMakeLists.txt

add_clang_library(clangTidyAbseilModule
   DurationFactoryScaleCheck.cpp
   DurationRewriter.cpp
   DurationSubtractionCheck.cpp
   DurationUnnecessaryConversionCheck.cpp
   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp
   RedundantStrcatCallsCheck.cpp
+  SafelyScopedCheck.cpp
   StrCatAppendCheck.cpp
   StringFindStartswithCheck.cpp
   TimeSubtractionCheck.cpp
   UpgradeDurationConversionsCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tidy/abseil/SafelyScopedCheck.h

+//===--- SafelyScopedCheck.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_SAFELYSCOPEDCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_SAFELYSCOPEDCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Detecting using declarations not in a namespace declaration or not in
+/// the innermost layer of namespace declarations.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-safely-scoped.html
+class SafelyScopedCheck : public ClangTidyCheck {
+public:
+  SafelyScopedCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_SAFELYSCOPEDCHECK_H

clang-tidy/abseil/SafelyScopedCheck.cpp

+//===--- SafelyScopedCheck.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 "SafelyScopedCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+void SafelyScopedCheck::registerMatchers(MatchFinder *Finder) {
+  // The target using declaration is either:
+  // 1. not in any scope, or
+  // 2. in some namespace declaration but not in the innermost layer
+  Finder->addMatcher(
+      usingDecl(
+          eachOf(allOf(usingDecl(unless(hasAncestor(functionDecl()))),
+                       usingDecl(unless(hasAncestor(cxxRecordDecl()))),
+                       usingDecl(unless(hasAncestor(namespaceDecl())))),
+                 usingDecl(hasParent(namespaceDecl(forEach(namespaceDecl()))))))
+          .bind("use"),
+      this);
+}
+
+void SafelyScopedCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<UsingDecl>("use");
+  diag(MatchedDecl->getLocation(),
+       "using declaration %0 is not declared in the innermost namespace. "
+       "Use discretion when moving using declarations as it might "
+       "necessitate moving lines containing relevant aliases.")
+      << MatchedDecl;
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`abseil-safely-scoped
+  <clang-tidy/checks/abseil-safely-scoped>` check.
+
+  Finds instances of using declarations not in the innermost layer
+  of a series of namespaces.
+
 - 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-safely-scoped.rst

+.. title:: clang-tidy - abseil-safely-scoped
+
+abseil-safely-scoped
+====================
+
+Flags using declarations that are not contained in an innermost
+namespace, and suggests these declarations be moved elsewhere.
+
+Example:
+
+.. code-block:: c++
+
+  using something; // should be inside the innermost namespace bar below
+
+  namespace foo {
+  namespace bar {
+  
+  } // bar
+
+  using something_else; // shoulw be inside the innermost namespace bar above
+
+  } // foo
+
+Placing convenience aliases in upper level namespaces can lead to ambiguity in 
+which name the compiler should use. 
+
+See https://abseil.io/tips/119 for more explanation. 
+

docs/clang-tidy/checks/list.rst

.. toctree::
    abseil-duration-factory-float
    abseil-duration-factory-scale
    abseil-duration-subtraction
    abseil-duration-unnecessary-conversion
    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
    abseil-redundant-strcat-calls
+   abseil-safely-scoped
    abseil-str-cat-append
    abseil-string-find-startswith
    abseil-time-subtraction
    abseil-upgrade-duration-conversions
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat
    android-cloexec-dup

test/clang-tidy/abseil-safely-scoped.cpp

+// RUN: %check_clang_tidy %s abseil-safely-scoped %t
+namespace bar {
+
+class A {};
+class B {};
+
+} // namespace bar
+
+namespace foo1 {
+
+// CHECK-MESSAGES: :[[@LINE+4]]:12: warning: using declaration 'A' is not
+// declared in the innermost namespace. Use discretion when moving using
+// declarations as it might necessitate moving lines containing relevant
+// aliases. [abseil-safely-scoped]
+using bar::A;
+void f(A a);
+
+namespace {} // anonymous namespace
+
+} // namespace foo1
+
+namespace foo2 {
+
+namespace {
+
+using ::bar::B;
+
+} // anonymous namespace
+
+void g(B b);
+
+} // namespace foo2
+
+// Check should not be triggered below when we are at
+// function (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 inner {} // namespace inner
+
+} // namespace outer