[clang-tidy] Add abseil-no-internal-deps check

[hugoeg]

Summary

Finds instances where the user depends on internal details and warns them against doing so.
Should not be run on internal Abseil files or Abseil source code.

[hugoeg]

add missing files

[Eugene.Zelenko]

Why not to use dependencies instead of deps in check's name?

docs/ReleaseNotes.rst

55

Please remove this line.

58

Please add first statement from documentation.

docs/clang-tidy/checks/abseil-no-internal-deps.rst

7

May be just Warns instead of This check gives users a warning?

Also misspelled dpened.

7

Abseil.

17

Please remove empty line.

22

Please add new line at end.

[JonasToth]

clang-tidy/abseil/NoInternalDepsCheck.cpp

25

Actually that one is generally useful. Accessing the foo::internal from outside of foo is always a problem. Maybe this matcher can become configurable or just match on any internal access from outside the enclosing namespace.

clang-tidy/abseil/NoInternalDepsCheck.h

22

Please make this a full sentence, like This check should not be run on internal ... (if that is grammatically correct)

[alexfh]

clang-tidy/abseil/NoInternalDepsCheck.cpp

39

s/. These can be found at/; see/

39

s/abseil/Abseil/

[hugoeg]

Applied corrections from first round comments

[hugoeg]

clang-tidy/abseil/NoInternalDepsCheck.cpp

25

That's a good idea. While we agree, right now our efforts are focused on releasing abseil specific functions. Perhaps we can refactor this check at a later time.

[JonasToth]

clang-tidy/abseil/NoInternalDepsCheck.cpp

25

Ok. Could you please add a TODO or FIXME that it is not forgotten?

[JonasToth]

Thank you for your first contribution to LLVM btw :)

clang-tidy/abseil/NoInternalDepsCheck.cpp

25

Missing //

36

Please follow the LLVM naming convention (s/initr_dep/InternalDependency/ or similar)

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly
for reference

[Eugene.Zelenko]

docs/ReleaseNotes.rst

58

I think will be good idea to omit user, and just refer to code which depend on internal details. Please synchronize with documentations.

test/clang-tidy/abseil-no-internal-deps.cpp

16

Please separate with empty line.

52

Please remove two empty lines

[hugoeg]

corrections from comments applied

[JonasToth]

No concerns except the small nits from my side.

clang-tidy/abseil/NoInternalDepsCheck.cpp

25

Nit: This comment is very long, pls break the line

docs/clang-tidy/checks/abseil-no-internal-deps.rst

7

s/violtaes/violates/

9

s/folowing/following/

[aaron.ballman]

clang-tidy/abseil/NoInternalDepsCheck.cpp

25

Also, we don't add developer names to comments.

40–42

Diagnostic text is not supposed to be grammatically correct with capitalization and punctuation. Please do not put links into diagnostics. It's more appropriate for this to be in the documentation. I think this might be improved wording: do not reference the 'internal' namespace; those implementation details are reserved to Abeil or something along those lines.

clang-tidy/abseil/NoInternalDepsCheck.h

20–22

Can you re-flow these comments?

docs/clang-tidy/checks/abseil-no-internal-deps.rst

7

Please wrap lines to 80 cols.

Nothing in this check looks at filenames and paths, but this suggests the check will find those. Is that intended work that's missed in this patch, or are the docs incorrect?

[hokein]

docs/clang-tidy/checks/abseil-no-internal-deps.rst

7

Please provide a link for the abseil guideline.

test/clang-tidy/abseil-no-internal-deps.cpp

3

nit: please make sure the code follow LLVM code style, even for test code :)

12

Since we have multiple abseil checks that might use these fake abseil declarations, I'd suggest pull out these to a common header, and include it in this test file.

[hugoeg]

test/clang-tidy/abseil-no-internal-deps.cpp

3

what is this in reference too?
Will the test still work if I wrap the CHECK MESSAGE lines?

12

do I just put the header file in test/clang-tidy ?

[hugoeg]

most corrections from comments have been applied

[JonasToth]

test/clang-tidy/abseil-no-internal-deps.cpp

3

CHECK-MESSAGE can be on one line, even if its longer (that is common in the clang-tidy tests).

But dont use many empty lines and respect naming conventions and run clang-format over the code (except there is a valid reason that the formatting would infer with the tested logic).

12

yes, there is one other similar case hicpp-signed-bitwise-standard-types.h

[JonasToth]

clang-tidy/abseil/NoInternalDepsCheck.h

22

double blank

docs/clang-tidy/checks/abseil-no-internal-deps.rst

18

That codeblock needs to be indented, see other checks for reference

[hugoeg]

test/clang-tidy/abseil-no-internal-deps.cpp

3

Do my function names have to be verbs, they're not doing anything.

I could rename InternalFunction to something like InternallyProcessString annd StringFunction to process String
Would this be preferred?

[JonasToth]

test/clang-tidy/abseil-no-internal-deps.cpp

3

It helps if you somehow show the "topic of the function". But I wrote some tests, that did not strictly follow and they passed review too ;)

Foo is just too generic, sth like DirectAccess, FriendUsage, OpeningNamespace or so is already telling and I guess good enough :)

[hugoeg]

applied corrections from comments

[JonasToth]

test/clang-tidy/abseil-no-internal-deps.cpp

5

Please run clang-format over the test code as well. The braces here in FriendUsage miss a space.

[hokein]

@hugoeg, it looks like you are waiting for review, but this patch doesn't include the IsExpansionInAbseilHeader matcher. What's your plan of it?

test/clang-tidy/abseil-fake-declarations.h

2

I'd expect this header file is used as as a real absl library file:

• create an absl directory in test/clang-tidy/Inputs/, and this directory is the abseil source root directory
• use the real absl function/class names instead of some fake names in the header, e.g. this file could be test/clang-tidy/Inputs/absl/strings/str_cat.h.

This would make the lit test align with the real world case, and it could be helpful if you implement the IsExpansionInAbseilHeader matcher in this check.

test/clang-tidy/abseil-no-internal-deps.cpp

20

the style doesn't looks correct to me.

[hugoeg]

test/clang-tidy/abseil-fake-declarations.h

2

My mentors on the absl team told me to refrain from using real absl function/class names as I did originally. They would prefer I use fake names. I'd be happy to update the filename something real like str_cat.h though

I have been implementing IsExpansionInHeader for the past couple days and I hope to have it included on this patch by the end of the week.

test/clang-tidy/abseil-no-internal-deps.cpp

20

This is the style given from clang-format

[hugoeg]

added IsInAbseilFile Matcher

[JonasToth]

clang-tidy/abseil/NoInternalDepsCheck.cpp

22

You can elide the braces for single stmt ifs

25

Please dont use auto here as the typ is not clear

27

ellide braces

30

no auto plz

33

Are the parens necessary? I think match does return a bool, then you can just return it without the braces. If not, please make it a boolean with an expression like match() > 0 or so.

[hokein]

clang-tidy/abseil/NoInternalDepsCheck.cpp

21

I think we can make it as an ASTMatcher instead of a function like:

AST_POLYMORPHIC_MATCHER_P(isInAbseilFile,
                          AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, TypeLoc)) {
   // your code here.
}

[hugoeg]

applied corrections form comments

[hugoeg]

clang-tidy/abseil/NoInternalDepsCheck.cpp

21

Unfortunately, I do not think we can. That was the way I originally tried to implement it. It worked on no-namespace-check, but not in this one. This is because as an AST_POLYMORPHIC_MATCHER_P, we are trying to match an usage of a Decl, not the Decl itself and since we are matching a TypeLoc in no-internal-deps-check, not really the usage, it doesn't work.

As a result, I modified my original implementation, which you will see in no-namespace-check and turned it into IsInAbseilFile(SourceManager&, SourceLocation), which is just takes a source location and returns whether the location we matched is in an Abseil file

[hugoeg]

Anymore changes I should make or issues I should be aware of?

[JonasToth]

In D50542#1205880, @hugoeg wrote:

Anymore changes I should make or issues I should be aware of?

From my side not!
You still have unresolved comments that cause the revision to not be Ready To Review for the main reviewers, maybe that causes them to overlook it.

test/clang-tidy/abseil-no-internal-deps.cpp

3

I think the names are ok.

[hugoeg]

fixed some minor things in the test

[hokein]

clang-tidy/abseil/NoInternalDepsCheck.cpp

21

Could you explain a bit more why it won't work? I don't understand why it doesn't work.

Basically you define the matcher like:

AST_POLYMORPHIC_MATCHER(
    isInAbseilFile, 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/(base|container|debugging|memory|meta|numeric|strings|"
                 "synchronization|types|utiliy)");
  return RE.match(Filename);
}

And use it for example in your check

Finder->addMatcher(nestedNameSpecifierLoc(
                         loc(specifiesNamespace(namespaceDecl(
                                 matchesName("internal"),
                                 hasParent(namespaceDecl(hasName("absl")))))),
                             unless(isInAbseilFile()))
                         .bind("InternalDep"),
                     this);

21

nit: LLVM code guideline uses Camel for variable names.

27

typo: utiliy => utility.

52

nit: remove the empty line.

docs/clang-tidy/checks/abseil-no-internal-deps.rst

9

This sentence doesn't parse.

18

nit: Please add a trailing comment on the line where it'd trigger the warning.

19

nit: space between foo and {.

[hugoeg]

corrections applied

[hugoeg]

clang-tidy/abseil/NoInternalDepsCheck.cpp

21

You're right, this implementation seems to work, I seemed to have a simple logic error in my original implementation, the new diff will include this version

[hugoeg]

refined test to include some more cases as a result of the new matcher

[hokein]

Your patch seems have some comments unaddressed but you marked done.

About the abseil matcher stuff, you and @deannagarcia have an overlap, maybe just wait for https://reviews.llvm.org/D50580 to submit, and reuse the matcher in this patch?

clang-tidy/abseil/NoInternalDepsCheck.cpp

25

nit: loc => Loc

[hugoeg]

All comments have been address and documentation has been added to matcher

[hugoeg]

Ok , so I've clean some stuff up, I'm currently working with @deannagarcia to get https://reviews.llvm.org/D50580 submitted with the matcher I implemented for us first, then will adjust this as necessary. Documentation has been added.

[hokein]

docs/clang-tidy/checks/abseil-no-internal-deps.rst

18

I think this line is also triggered the warning?

test/clang-tidy/abseil-no-internal-deps.cpp

9

Does the test get passed on the first run RUN: %check_clang_tidy %s abseil-no-internal-deps %t, -- -- -I %S/Inputs of the test? It will suppress clang-tidy warnings from the header, and the warning here should not appear.

[hugoeg]

fixed comments

test/clang-tidy/abseil-no-internal-deps.cpp

9

Yes, the test passes cleanly on both runs, I just re ran it a couple times to make sure.

[hokein]

Please rebase your patch, since the absl matcher patch is submitted. Thanks.

test/clang-tidy/abseil-no-internal-deps.cpp

9

Yeah, I misread it as CHECK-MESSAGE.

[kbobyrev]

clang-tidy/abseil/NoInternalDepsCheck.cpp

25

I think @hokein's comment wasn't addressed despite being checked: loc should become Loc to comply with the LLVM naming rules.

33

Nit: maybe replace with Filename.startswith("absl/") then use std::any_of on Filename.drop_front("absl/".size())?

[hugoeg]

made some major updates after no-namespace landed

[hokein]

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

1 ↗

(On Diff #162864)

The file can not self-compile, and we should make it compilable.

And put the newly-added code at the end of the file, so that other test file can keep unchanged.

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

34 ↗

(On Diff #162864)

nit: remove the empty line.

[hugoeg]

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

1 ↗

(On Diff #162864)

how do I make it compilable?

[Eugene.Zelenko]

I still thik will be good idea to rename check (deps -> dependencies).

[hugoeg]

fixed issues outlines in comments

[hugoeg]

renamed check as suggested

[hokein]

LGTM with a nit, thanks for your work.

clang-tidy/abseil/NoInternalDependenciesCheck.h

20 ↗

(On Diff #162921)

This comment is out of date. The check can handle it, I think.

[hugoeg]

nit comment fixed

[JonasToth]

In D50542#1217511, @hugoeg wrote:

nit comment fixed

DO you have commit rights or shall i commit for you?

[hugoeg]

In D50542#1217515, @JonasToth wrote:

In D50542#1217511, @hugoeg wrote:

nit comment fixed

DO you have commit rights or shall i commit for you?

I do not have commit rights so I would appreciate it if you would commit me. Thanks!

[JonasToth]

Committed r340928. Thanks very much!

Will you (and your team) upstream even more abseil checks? I think it might be reasonable to ask for commit rights.

[hugoeg]

In D50542#1217517, @JonasToth wrote:

Committed r340928. Thanks very much!

Will you (and your team) upstream even more abseil checks? I think it might be reasonable to ask for commit rights.

We will likely not be up streaming more in the near future. Possibly one or two. I am also an intern really close to being done, so it will probably not be worth applying for commit rights. Thank you though for all the help.

[JonasToth]

Ok. Then we can commit for you guys, no problem :)

Am 29.08.2018 um 16:34 schrieb Hugo Gonzalez via Phabricator:

hugoeg added a comment.

In https://reviews.llvm.org/D50542#1217517, @JonasToth wrote:

Committed r340928. Thanks very much!

Will you (and your team) upstream even more abseil checks? I think it might be reasonable to ask for commit rights.

We will likely not be up streaming more in the near future. Possibly one or two. I am also an intern really close to being done, so it will probably not be worth applying for commit rights. Thank you though for all the help.

Repository:

rL LLVM

https://reviews.llvm.org/D50542

clang-tidy/abseil/AbseilTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "NoInternalDepsCheck.h"
 #include "StringFindStartswithCheck.h"
 
 namespace clang {

 class AbseilModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<NoInternalDepsCheck>(
+        "abseil-no-internal-deps");
     CheckFactories.registerCheck<StringFindStartswithCheck>(
         "abseil-string-find-startswith");
   }

clang-tidy/abseil/CMakeLists.txt

 
 add_clang_library(clangTidyAbseilModule
   AbseilTidyModule.cpp
+  NoInternalDepsCheck.cpp
   StringFindStartswithCheck.cpp
 
   LINK_LIBS

clang-tidy/abseil/NoInternalDepsCheck.h

+//===--- NoInternalDepsCheck.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_NOINTERNALDEPSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_ABSEIL_NOINTERNALDEPSCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+/// Finds instances where the user depends on internal details and warns them
+/// against doing so. This check should not be run on internal Abseil files or
+/// Abseil source code.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/abseil-no-internal-deps.html
+class NoInternalDepsCheck : public ClangTidyCheck {
+public:
+  NoInternalDepsCheck(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_NOINTERNALDEPSCHECK_H

clang-tidy/abseil/NoInternalDepsCheck.cpp

+//===--- NoInternalDepsCheck.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 "NoInternalDepsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace abseil {
+
+void NoInternalDepsCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  // TODO: refactor matcher to be configurable or just match on any internal
+  // access from outside the enclosing namespace.
+  
+  Finder->addMatcher(
+      nestedNameSpecifierLoc(loc(specifiesNamespace(namespaceDecl(
+                                 matchesName("internal"),
+                                 hasParent(namespaceDecl(hasName("absl")))))))
+          .bind("InternalDep"),
+      this);
+}
+
+void NoInternalDepsCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *InternalDependency =
+      Result.Nodes.getNodeAs<NestedNameSpecifierLoc>("InternalDep");
+
+  diag(InternalDependency->getBeginLoc(),
+       "do not reference any 'internal' namespaces; those implementation "
+       "details are reserved to Abseil");
+}
+
+} // namespace abseil
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
-The improvements are...
+- New :doc:`abseil-no-internal-deps
+  <clang-tidy/checks/abseil-no-internal-deps>` check.
+  
+  Gives a warning if code using Abseil depends on internal details.
 
 Improvements to clang-tidy
 --------------------------

docs/clang-tidy/checks/abseil-no-internal-deps.rst

+subl.. title:: clang-tidy - abseil-no-internal-deps
+
+abseil-no-internal-deps
+=======================
+
+Gives a warning if code using Abseil depends on internal details. If something
+is in a namespace that includes the word “internal”, code is not allowed to 
+depend upon it beaucse it’s an implementation detail. They cannot friend it, 
+include it, you mention it or refer to it in any way. Doing so violates 
+Abseil's compatibility guidelines and may result in breakage. See 
+https://abseil.io/about/compatibility for more information.
+
+The following cases will result in warnings:
+
+.. code-block:: c++
+
+  absl::strings_internal::foo();
+  class foo{
+    friend struct absl::container_internal::faa;
+  };
+  absl::memory_internal::MakeUniqueResult();

docs/clang-tidy/checks/list.rst

 
 .. toctree::
    abseil-string-find-startswith
+   abseil-no-internal-deps
    android-cloexec-accept
    android-cloexec-accept4
    android-cloexec-creat

test/clang-tidy/abseil-fake-declarations.h

+namespace std {
+struct string {
+  string(const char *);
+  ~string();
+};
+} // namepsace std
+
+namespace absl {
+std::string StringsFunction (std::string s1){
+  return s1;
+}
+class SomeContainer{
+
+};
+namespace strings_internal{
+  void InternalFunction(){}
+  template <class P>
+  P InternalTemplateFunction (P a) {}
+} // namespace strings_internal
+
+namespace container_internal{
+  struct InternalStruct{};
+} // namespace container_internal
+} // namespace absl

test/clang-tidy/abseil-no-internal-deps.cpp

+// RUN: %check_clang_tidy %s abseil-no-internal-deps %t
+
+#include "abseil-fake-declarations.h"
+
+void DirectAcess(){
+  absl::strings_internal::InternalFunction();
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+
+  absl::strings_internal::InternalTemplateFunction <std::string>("a");
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+}
+
+class FriendUsage{
+  friend struct absl::container_internal::InternalStruct;
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+};
+
+namespace absl{
+  void OpeningNamespace(){
+    strings_internal::InternalFunction();
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not reference any 'internal' namespaces; those implementation details are reserved to Abseil
+  }
+} // namespace absl
+
+// should not trigger warnings
+void CorrectUsage(){
+  std::string Str = absl::StringsFunction("a");
+  absl::SomeContainer b;
+}
+
+namespace absl {
+  SomeContainer b;
+  std::string Str = absl::StringsFunction("a");
+} // namespace absl