[clang-tidy] check for using declaration qualification

[Naysh]

Summary

This patch adds a "Qualified Aliases" check, based off the guidelines in https://abseil.io/tips/119, to the abseil module.

The check flags any using declarations which are not fully qualified.

[Eugene.Zelenko]

clang-tidy/abseil/AnonymousEnclosedAliasesCheck.cpp

31 ↗

(On Diff #176900)

Please remove unnecessary empty line.

33 ↗

(On Diff #176900)

Please remove unnecessary empty line.

34 ↗

(On Diff #176900)

const auto *

clang-tidy/abseil/QualifiedAliasesCheck.cpp

14

Please remove unnecessary empty line.

clang-tidy/abseil/SafelyScopedCheck.cpp

27 ↗

(On Diff #176900)

Please run Clang-format over patch.

docs/ReleaseNotes.rst

73

Please enclose using in ``. Same in other places, including documentation.

docs/clang-tidy/checks/abseil-alias-free-headers.rst

10 ↗

(On Diff #176900)

Are there guidelines?

docs/clang-tidy/checks/abseil-qualified-aliases.rst

25

Please remove unnecessary empty line.

docs/clang-tidy/checks/abseil-safely-scoped.rst

28 ↗

(On Diff #176900)

Please remove unnecessary empty line.

test/clang-tidy/abseil-qualified-aliases.cpp

28

Please remove unnecessary empty line.

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

27 ↗

(On Diff #176900)

Please remove unnecessary empty line.

[Eugene.Zelenko]

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

20 ↗

(On Diff #176900)

Are there guidelines?

docs/clang-tidy/checks/abseil-qualified-aliases.rst

23

Please make text consistent across all these checks.

[alexfh]

Could you send each check in a separate patch, please? This makes the review much more straightforward and I'm sure it will be faster too.

[alexfh]

clang-tidy/abseil/AbseilTidyModule.cpp

25

Please upload the patch with full context. See https://llvm.org/docs/Phabricator.html

[Naysh]

To respond to alexfh's request to split this submission into four separate patches (corresponding to each of the introduced checks), I've updated the patch to only include the "Qualified Aliases" check.

I'll add the other checks as separate patches.

[aaron.ballman]

clang-tidy/abseil/QualifiedAliasesCheck.cpp

29

Drop the top-level const qualifier (here and elsewhere).

34

No need for the assignment.

39

This is missing a check for whether the referenced name is within the current namespace.

43–44

Do not put HTML links into the diagnostic -- the wording should stand on its own. I would probably phrase it "using declaration should use a fully qualified name" or something along those lines.

clang-tidy/abseil/QualifiedAliasesCheck.h

19

This should be fixed.

test/clang-tidy/abseil-qualified-aliases.cpp

22–23

I don't think this should warn, per the tip.

clang-tidy/abseil/AbseilTidyModule.cpp

 #include "FasterStrsplitDelimiterCheck.h"
 #include "NoInternalDependenciesCheck.h"
 #include "NoNamespaceCheck.h"
+#include "QualifiedAliasesCheck.h"
 #include "RedundantStrcatCallsCheck.h"
 #include "StringFindStartswithCheck.h"
 #include "StrCatAppendCheck.h"

     CheckFactories.registerCheck<NoInternalDependenciesCheck>(
         "abseil-no-internal-dependencies");
     CheckFactories.registerCheck<NoNamespaceCheck>("abseil-no-namespace");
+    CheckFactories.registerCheck<QualifiedAliasesCheck>(
+        "abseil-qualified-aliases");
     CheckFactories.registerCheck<RedundantStrcatCallsCheck>(
         "abseil-redundant-strcat-calls");
     CheckFactories.registerCheck<StrCatAppendCheck>(

clang-tidy/abseil/CMakeLists.txt

   FasterStrsplitDelimiterCheck.cpp
   NoInternalDependenciesCheck.cpp
   NoNamespaceCheck.cpp
+  QualifiedAliasesCheck.cpp
   RedundantStrcatCallsCheck.cpp
   StrCatAppendCheck.cpp
   StringFindStartswithCheck.cpp

clang-tidy/abseil/QualifiedAliasesCheck.h

+//===--- QualifiedAliasesCheck.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_QUALIFIEDALIASESCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_QUALIFIEDALIASESCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-qualified-aliases.html
+class QualifiedAliasesCheck : public ClangTidyCheck {
+public:
+  QualifiedAliasesCheck(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_QUALIFIEDALIASESCHECK_H

clang-tidy/abseil/QualifiedAliasesCheck.cpp

+//===--- QualifiedAliasesCheck.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 "QualifiedAliasesCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+void QualifiedAliasesCheck::registerMatchers(MatchFinder *Finder) {
+  // Matches all using declarations.
+  Finder->addMatcher(usingDecl().bind("x"), this);
+}
+
+void QualifiedAliasesCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<UsingDecl>("x");
+
+  // Finds the nested-name-specifier location.
+  const NestedNameSpecifierLoc QualifiedLoc = MatchedDecl->getQualifierLoc();
+  const SourceLocation FrontLoc = QualifiedLoc.getBeginLoc();
+
+  // Checks if the using declaration is fully qualified.
+  const SourceManager *SM = Result.SourceManager;
+  CharSourceRange FrontRange = CharSourceRange();
+  FrontRange.setBegin(FrontLoc);
+  FrontRange.setEnd(FrontLoc.getLocWithOffset(2));
+  llvm::StringRef Beg = Lexer::getSourceText(FrontRange, *SM, LangOptions());
+
+  // If the using declaration is fully qualified, don't produce a warning.
+  if (Beg.startswith("::"))
+    return;
+
+  diag(FrontLoc, "using declaration is not fully qualified: see "
+  "https://abseil.io/tips/119");
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

   Ensures code does not open ``namespace absl`` as that violates Abseil's
   compatibility guidelines.
 
+- New :doc:`abseil-qualified-aliases
+  <clang-tidy/checks/abseil-qualified-aliases>` check.
+
+  Detects using declarations that are not fully qualified, and suggests
+  that the developer fully qualify those declarations.
+
 - New :doc:`abseil-redundant-strcat-calls
   <clang-tidy/checks/abseil-redundant-strcat-calls>` check.
 

docs/clang-tidy/checks/abseil-qualified-aliases.rst

+.. title:: clang-tidy - abseil-qualified-aliases
+
+abseil-qualified-aliases
+========================
+
+Detects using declarations that are not fully qualified, and suggests
+that the developer fully qualify those declarations.
+
+Example:
+.. code-block:: c++
+
+  namespace foo {
+    void f();
+    void correct();
+  }
+
+  namespace bar {
+    using foo::f; // The check produces a warning here.
+    using ::foo::correct; // The check sees no issue here.
+  }
+
+See https://abseil.io/tips/119 for a more in depth justification of this
+check.

docs/clang-tidy/checks/list.rst

    abseil-faster-strsplit-delimiter
    abseil-no-internal-dependencies
    abseil-no-namespace
+   abseil-qualified-aliases
    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-qualified-aliases.cpp

+// RUN: %check_clang_tidy %s abseil-qualified-aliases %t
+
+namespace foo {
+  void f();
+  void correct();
+}
+
+namespace bar {
+  using foo::f;
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: using declaration is not fully qualified: see https://abseil.io/tips/119 [abseil-qualified-aliases]
+  using ::foo::correct;
+}
+
+namespace outermost {
+  namespace middle {
+    namespace innermost {
+
+      enum Color {Red, Blue, Yellow};
+
+    } // namespace innermost
+
+    using innermost::Color;
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: using declaration is not fully qualified: see https://abseil.io/tips/119 [abseil-qualified-aliases]
+
+  } // namespace middle
+} // namespace example