[clang-tidy] check for flagging using declarations in headers

[Naysh]

Summary

This patch adds an "Alias Free Headers" check, based off the guidelines at http://google.github.io/styleguide/cppguide.html#Aliases, to the abseil module.

The purpose of the check is to eliminate using declarations from header files.

[JonasToth]

Please upload with full context.

[steveire]

Would it make sense to make this a generic non-abseil check?

[lebedev.ri]

clang-tidy/abseil/AliasFreeHeadersCheck.cpp

22

Relevant:

$ clang-tidy -dump-config | grep -i extension -A1
  - key:             cert-dcl59-cpp.HeaderFileExtensions
    value:           ',h,hh,hpp,hxx'
--
  - key:             google-build-namespaces.HeaderFileExtensions
    value:           h,hh,hpp,hxx,
  - key:             google-global-names-in-headers.HeaderFileExtensions
    value:           h,hh,hpp,hxx,
--
  - key:             misc-definitions-in-headers.HeaderFileExtensions
    value:           h,hh,hpp,hxx,
  - key:             misc-definitions-in-headers.UseHeaderFileExtension
    value:           '1'

The list of header suffixes should be configurable.

29

https

clang-tidy/abseil/AliasFreeHeadersCheck.h

19

Hm

docs/clang-tidy/checks/list.rst

155

Many unrelated movements.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

73

Please highlight using with ``.

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

6

Please highlight using with ``. Same in other places.

[Eugene.Zelenko]

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

13

Missing link to guidelines,

[aaron.ballman]

Have you run this check over any large code bases to see what its false positive rate looks like? I have to imagine we're going to need some escape hatch for system headers (we shouldn't complain about using declarations outside of the user's control).

clang-tidy/abseil/AliasFreeHeadersCheck.cpp

28

What is a "convenience alias" and why is it dangerous? The diagnostic should explain what's wrong with the user's code.

29

No links in diagnostics, whether http or https. ;-)

Also, why should this be a separate check from the other positional one in D55411?

[Naysh]

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

13

@Eugene.Zelenko: Sorry, I'm not sure what you mean. Could you clarify what guidelines you're referring to?

[Eugene.Zelenko]

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

13

Please see documentation in other using-related checks.

[aaron.ballman]

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

13

I think he's talking about the fact that this is text and not a hyperlink. It should look something more like

The `Abseil Style Guide <http://google.github.io/styleguide/cppguide.html#Aliases>`_ discusses this issue in more detail.

[Naysh]

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

13

Ah okay, that makes sense. Thanks for your help!

clang-tidy/abseil/AbseilTidyModule.cpp

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

 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AliasFreeHeadersCheck>(
+        "abseil-alias-free-headers");
     CheckFactories.registerCheck<DurationDivisionCheck>(
         "abseil-duration-division");
     CheckFactories.registerCheck<DurationFactoryFloatCheck>(

clang-tidy/abseil/AliasFreeHeadersCheck.h

+//===--- AliasFreeHeadersCheck.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_ALIASFREEHEADERSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_ALIASFREEHEADERSCHECK_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-alias-free-headers.html
+class AliasFreeHeadersCheck : public ClangTidyCheck {
+public:
+  AliasFreeHeadersCheck(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_ALIASFREEHEADERSCHECK_H

clang-tidy/abseil/AliasFreeHeadersCheck.cpp

+//===--- AliasFreeHeadersCheck.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 "AliasFreeHeadersCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+void AliasFreeHeadersCheck::registerMatchers(MatchFinder *Finder) {
+  // Match all using declarations in header files.
+  Finder->addMatcher(usingDecl(isExpansionInFileMatching(".*\\.h.*")).bind("x"),
+    this);
+}
+
+void AliasFreeHeadersCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<UsingDecl>("x");
+  diag(MatchedDecl->getLocation(), "convenience aliases in header files are "
+    "dangerous: see http://google.github.io/styleguide/cppguide.html#Aliases");
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

clang-tidy/abseil/CMakeLists.txt

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

docs/ReleaseNotes.rst

 Improvements to clang-tidy
 --------------------------
 
+- New :doc:`abseil-alias-free-headers
+  <clang-tidy/checks/abseil-alias-free-headers>` check.
+
+  Flags using declarations in header files, and suggests that
+  these aliases be removed
+
 - New :doc:`abseil-duration-division
   <clang-tidy/checks/abseil-duration-division>` check.
 

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

+.. title:: clang-tidy - abseil-alias-free-headers
+
+abseil-alias-free-headers
+=========================
+
+Flags using declarations in header files, and suggests that these aliases be removed.
+
+A using declaration placed in a header file forces users of that header file
+accept the specified alias. Because of this, using declarations should almost never
+be used in a header file
+
+The style guide  http://google.github.io/styleguide/cppguide.html#Aliases discusses this
+issue in more detail

docs/clang-tidy/checks/list.rst

 =================
 
 .. toctree::
+   abseil-alias-free-headers
    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-alias-free-headers.hpp

+// RUN: %check_clang_tidy %s abseil-alias-free-headers %t
+
+namespace foo {
+  void f();
+}
+
+using foo::f;
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: convenience aliases in header files are dangerous: see http://google.github.io/styleguide/cppguide.html#Aliases [abseil-alias-free-headers]
+
+void other_function();