[clang-tidy] Abseil: no namespace check

[deannagarcia]

Summary

This check ensures that users of Abseil do not open namespace absl in their code, as that violates our compatibility guidelines.

AbseilMatcher.h written by Hugo Gonzalez.

[Eugene.Zelenko]

Check documentation is missing.

clang-tidy/abseil/NoNamespaceCheck.cpp

22

Please place return in separate line.

clang-tidy/abseil/NoNamespaceCheck.h

26

Please run Clang-format.

docs/ReleaseNotes.rst

60–64

Please remove placeholder.

69

Please reformulate to refer to code, not user. Please enclose absl in ``.

70

abseil ->Abseil.

[JonasToth]

Could it happen that some template specializations or so need to land in absl?

clang-tidy/abseil/NoNamespaceCheck.cpp

29

Please follow LLVM naming convention (Capitalize, and Decl should crash with a type)

[aaron.ballman]

clang-tidy/abseil/NoNamespaceCheck.cpp

24

I think this needs a not(isExpansionInSystemHeader()) in there, as this is going to trigger on code that includes a header file using an absl namespace. If I'm incorrect and users typically include abseil as something other than system includes, you'll have to find a different way to solve this.

33

in the user code -> in user code

Does Abseil prohibit the user from specializing templates in the absl namespace with user-defined types?

[lebedev.ri]

We are aware that this test will cause warnings on users code through their dependencies on abseil.
However, from what we know it seems like these warnings are normally suppressed.
If anyone has a good idea on how to avoid this/has insight on whether this will be a problem for the average user, please let me know!

Suppressed how?
I think this needs tests.

[hokein]

The check is missing its document, please add one in docs/clang-tidy/checks/.

clang-tidy/abseil/NoNamespaceCheck.cpp

24

Yeah, we have discussed this issue internally. abseil-user code usually includes abseil header like #include "whatever/abseil/base/optimization.h", so IsExpansionInSystemHeader doesn't work for most cases.

We need a way to filter out all headers being a part of abseil library. I think we can create a matcher InExpansionInAbseilHeader -- basically using IsExpansionInFileMatching with a regex expression that matches typical abseil include path. What do you think?

And we'll have more abseil checks (e.g. abseil-no-internal-deps) that use InExpansionInAbseilHeader, we should put it to a common header.

[aaron.ballman]

clang-tidy/abseil/NoNamespaceCheck.cpp

24

I think that is a sensible approach.

[deannagarcia]

clang-tidy/abseil/NoNamespaceCheck.cpp

24

We will begin working on this, I think it will first be implemented in abseil-no-internal-deps but once it looks good I will add it to this patch.

[hugoeg]

clang-tidy/abseil/NoNamespaceCheck.cpp

24

I'm going to go ahead a implement this in ASTMatchers.h and include it on the patch for abseil-no-internal-deps

[JonasToth]

clang-tidy/abseil/NoNamespaceCheck.cpp

24

In principle it is ok, but I don't think ASTMatchers.h is the correct place, because it is only of use to clang-tidy.

There is a util directory in clang-tidy for this kind of stuff. Defining you own matchers works the same as in ASTMatchers, you can grep through clang-tidy checks (e.g. AST_MATCHER) to have some examples of private matchers.

[hokein]

clang-tidy/abseil/NoNamespaceCheck.cpp

24

ASTMatchers.h is not a reasonable place to put asseil-specific matchers. We have clang-tidy/utils/Matchers.h for putting clang-tidy specific matchers. I'm not sure whether it is reasonable to put abseil-specific matchers there. Maybe we create a AbseilMatcher.h file in clang-tidy/abseil directory?

[JonasToth]

That sounds good as well, just not in clang and best in clang-tidy/utils

ASTMatchers.h is not a reasonable place to put asseil-specific matchers. We have clang-tidy/utils/Matchers.h for putting clang-tidy specific matchers. I'm not sure whether it is reasonable to put abseil-specific matchers there. Maybe we create a AbseilMatcher.h file in clang-tidy/abseil directory?

https://reviews.llvm.org/D50580

[deannagarcia]

This revision includes a matcher so that the warning does not trigger on internal Abseil files.

clang-tidy/abseil/NoNamespaceCheck.cpp

24

I put it in clang-tidy/abseil for now but I can definitely move it to clang-tidy/utils/ if you would prefer that

[hokein]

clang-tidy/abseil/AbseilMatcher.h

17

nit: We need proper documentation for this matcher, since it is exposed to users.

18

• using negative for AST matcher seems uncommon in ASTMatchers, for negative we have unless, so I'd suggest implementing isInAbseilHeader.
• it seems that this matcher is very similar to the matcher in https://reviews.llvm.org/D50542. I think isInAbseilFile should also cover your case here, and also ignore the warning if you run the check on abseil core files.

33

The regex seems incomplete, we are missing algorithm, time subdirectory.

34

typo: utility

35

nit: remove the outer ().

[lebedev.ri]

clang-tidy/abseil/AbseilMatcher.h

33

So what happens if open the namespace in a file that is located in my personal absl/base directory?
It will be false-negatively not reported?

[hokein]

clang-tidy/abseil/AbseilMatcher.h

33

Yes, I'd say this is a known limitation.

[lebedev.ri]

clang-tidy/abseil/AbseilMatcher.h

33

Similarly, the check will break (start producing false-positives) as soon as a new directory is added to abseil proper.
I don't have any reliable idea on how to do this better, but the current solution seems rather suboptimal..

[deannagarcia]

clang-tidy/abseil/AbseilMatcher.h

33

We are aware that there will be a false-negative if code opens namespace in the absl directories, however we think that is pretty rare and that users shouldn't be doing that anyway. No matter how we do this there will be a way for users to circumvent the check, and we will allow those users to do it because it will be their code that breaks in the end.

33

The false-positive with new directories is something we thought about, but new directories aren't added to absl very often so we thought it wouldn't be too hard to add them to this regex when they are.

[hugoeg]

clang-tidy/abseil/AbseilMatcher.h

33

Members of the Abseil team can also update this check as new directories are release

[hokein]

Thanks for the updates. Looks mostly good, a few nits.

clang-tidy/abseil/AbseilMatcher.h

32

nit: this matcher name now should be isInAbseilFile.

clang-tidy/abseil/NoNamespaceCheck.cpp

26

Does the absl namespace is always the top level namespace? If so, I think it would be safer to use qualified name "::absl" here.

clang-tidy/abseil/NoNamespaceCheck.h

19

nit: test => check.

20

nit: our = abseil?

test/clang-tidy/abseil-no-namespace.cpp

22

nit: put it immediately after the above name absl {.

[Eugene.Zelenko]

Please rebase from trunk.

docs/ReleaseNotes.rst

63

Just Ensures. Please use `` for language constructs.

docs/clang-tidy/checks/abseil-no-namespace.rst

6

Please synchronize first sentence with Release Notes.

7

Abseil

10

Code instead of user, please.

20

Please remove space after https:

[Eugene.Zelenko]

docs/ReleaseNotes.rst

60

Please sort new checks list alphabetically.

66

Please remove merge conflict residual.

[hokein]

Looks good.

test/clang-tidy/abseil-no-namespace.cpp

11

Does the test get passed on the first command %check_clang_tidy %s abseil-no-namespace %t -- -- -I %S/Inputs? The first command will suppress all warning in headers, I think it will fail?

[deannagarcia]

Rebased the patch

[deannagarcia]

test/clang-tidy/abseil-no-namespace.cpp

11

The test passes, and I'm pretty sure it's because this is a CHECK not a CHECK-MESSAGES

[hokein]

I have committed the patch for you, rL340800.

test/clang-tidy/abseil-no-namespace.cpp

11

Ah, I thought it was CHECK-MESSAGES, note that CHECK and CHECK-MESSAGE don't match multiple lines, the message should be one-line. I have fixed for you.

clang-tidy/abseil/AbseilMatcher.h

+//===- AbseilMatcher.h - clang-tidy ---------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+namespace clang {
+namespace ast_matchers {
+
+/// Matches AST nodes that were found within Abseil files.
+///
+/// Example matches Y but not X
+///     (matcher = cxxRecordDecl(isInAbseilFile())
+/// \code
+///   #include "absl/strings/internal-file.h"
+///   class X {};
+/// \endcode
+/// absl/strings/internal-file.h:
+/// \code
+///   class Y {};
+/// \endcode
+///
+/// Usable as: Matcher<Decl>, Matcher<Stmt>, Matcher<TypeLoc>,
+/// Matcher<NestedNameSpecifierLoc>
+
+AST_POLYMORPHIC_MATCHER(isExpansionInAbseilHeader,
+                        AST_POLYMORPHIC_SUPPORTED_TYPES(
+                            Decl, Stmt, TypeLoc, NestedNameSpecifierLoc)) {
+  auto &SourceManager = Finder->getASTContext().getSourceManager();
+  SourceLocation Loc = Node.getBeginLoc();
+  if (Loc.isInvalid())
+    return false;
+  const FileEntry *FileEntry =
+      SourceManager.getFileEntryForID(SourceManager.getFileID(Loc));
+  if (!FileEntry)
+    return false;
+  StringRef Filename = FileEntry->getName();
+  llvm::Regex RE(
+      "absl/(algorithm|base|container|debugging|memory|meta|numeric|strings|"
+      "synchronization|time|types|utility)");
+  return RE.match(Filename);
+}
+
+} // namespace ast_matchers
+} // namespace clang

clang-tidy/abseil/AbseilTidyModule.cpp

 //===------- AbseilTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "NoNamespaceCheck.h"
 #include "StringFindStartswithCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace abseil {
 
 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<NoNamespaceCheck>("abseil-no-namespace");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
   }
 };
 
 // Register the AbseilModule using this statically initialized variable.
 static ClangTidyModuleRegistry::Add<AbseilModule> X("abseil-module",
                                                     "Add Abseil checks.");

clang-tidy/abseil/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  NoNamespaceCheck.cpp
   StringFindStartswithCheck.cpp
 
   LINK_LIBS
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   )

clang-tidy/abseil/NoNamespaceCheck.h

+//===--- NoNamespaceCheck.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_NONAMESPACECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NONAMESPACECHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// This test ensures users don't open namespace absl, as that violates
+/// our compatibility guidelines.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-no-namespace.html
+class NoNamespaceCheck : public ClangTidyCheck {
+public:
+  NoNamespaceCheck(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_NONAMESPACECHECK_H

clang-tidy/abseil/NoNamespaceCheck.cpp

+//===--- NoNamespaceCheck.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 "NoNamespaceCheck.h"
+#include "AbseilMatcher.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+void NoNamespaceCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(
+      namespaceDecl(hasName("absl"), unless(isExpansionInAbseilHeader()))
+          .bind("abslNamespace"),
+      this);
+}
+
+void NoNamespaceCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *abslNamespaceDecl =
+      Result.Nodes.getNodeAs<NamespaceDecl>("abslNamespace");
+
+  diag(abslNamespaceDecl->getLocation(),
+       "namespace 'absl' is reserved for implementation of the Abseil library "
+       "and should not be opened in user code");
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
-The improvements are...
+- New :doc:`abseil-no-namespace
+  <clang-tidy/checks/abseil-no-namespace>` check.
+
+  Checks to ensure code does not open `namespace absl` as that
+  violates Abseil's compatibility guidelines.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/abseil-no-namespace.rst

+.. title:: clang-tidy - abseil-no-namespace
+
+abseil-no-namespace
+===================
+
+This check gives users a warning if they attempt to open ``namespace absl``.
+Users should not open ``namespace absl`` as that conflicts with abseil's
+compatibility guidelines and may result in breakage.
+
+Any user that uses:
+
+.. code-block:: c++
+
+ namespace absl {
+  ...
+ }
+
+will be prompted with a warning.
+
+See `the full Abseil compatibility guidelines <https: //
+abseil.io/about/compatibility>`_ for more information.

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
+   abseil-no-namespace
    abseil-string-find-startswith
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat
    android-cloexec-dup
    android-cloexec-epoll-create
    android-cloexec-epoll-create1
    android-cloexec-fopen

test/clang-tidy/Inputs/absl/external-file.h

+namespace absl {}

test/clang-tidy/Inputs/absl/strings/internal-file.h

+namespace absl {}

test/clang-tidy/abseil-no-namespace.cpp

+// RUN: %check_clang_tidy %s abseil-no-namespace %t -- -- -I %S/Inputs
+// RUN: clang-tidy -checks='-*, abseil-no-namespace' -header-filter='.*' %s -- -I %S/Inputs 2>&1 | FileCheck %s
+
+// Warning will not be triggered on internal Abseil code that is included.
+#include "absl/strings/internal-file.h"
+// CHECK-NOT: warning:
+
+// Warning will be triggered on code that is not internal that is included.
+#include "absl/external-file.h"
+// CHECK: absl/external-file.h:1:11: warning: namespace 'absl' is reserved
+// for implementation of the Abseil library and should not be opened in
+// user code [abseil-no-namespace]
+
+namespace absl {}
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: namespace 'absl' is reserved for
+// implementation of the Abseil library and should not be opened in the user
+// code [abseil-no-namespace]
+
+namespace absl {
+int i = 5;
+}
+// CHECK-MESSAGES: :[[@LINE-3]]:11: warning: namespace 'absl' 
+
+// Things that shouldn't trigger the check
+int i = 5;
+namespace std {}