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

Authored by hugoeg on Aug 9 2018, 5:44 PM.

Details

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.

Diff Detail

Repository
rL LLVM
There are a very large number of changes, so older changes are hidden. Show Older Changes
hugoeg updated this revision to Diff 160371.Aug 13 2018, 9:17 AM
hugoeg marked an inline comment as done.
hugoeg added inline comments.Aug 13 2018, 9:32 AM
test/clang-tidy/abseil-no-internal-deps.cpp
2 ↗(On Diff #160168)

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?

hugoeg updated this revision to Diff 160380.Aug 13 2018, 9:59 AM
hugoeg marked 3 inline comments as done.
JonasToth added inline comments.Aug 13 2018, 1:02 PM
test/clang-tidy/abseil-no-internal-deps.cpp
2 ↗(On Diff #160168)

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 updated this revision to Diff 160432.Aug 13 2018, 1:20 PM
hugoeg marked an inline comment as done.

applied corrections from comments

JonasToth added inline comments.Aug 14 2018, 1:10 AM
test/clang-tidy/abseil-no-internal-deps.cpp
5 ↗(On Diff #160432)

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

hugoeg updated this revision to Diff 160562.Aug 14 2018, 5:41 AM
hugoeg marked an inline comment as done.

@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
1 ↗(On Diff #160562)

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
19 ↗(On Diff #160562)

the style doesn't looks correct to me.

hugoeg added inline comments.Aug 16 2018, 7:14 AM
test/clang-tidy/abseil-fake-declarations.h
1 ↗(On Diff #160562)

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
19 ↗(On Diff #160562)

This is the style given from clang-format

hugoeg updated this revision to Diff 161130.Aug 16 2018, 3:25 PM

added IsInAbseilFile Matcher

JonasToth added inline comments.Aug 17 2018, 12:36 AM
clang-tidy/abseil/NoInternalDepsCheck.cpp
21 ↗(On Diff #161130)

You can elide the braces for single stmt ifs

24 ↗(On Diff #161130)

Please dont use auto here as the typ is not clear

26 ↗(On Diff #161130)

ellide braces

29 ↗(On Diff #161130)

no auto plz

32 ↗(On Diff #161130)

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 added inline comments.Aug 17 2018, 7:41 AM
clang-tidy/abseil/NoInternalDepsCheck.cpp
20 ↗(On Diff #161130)

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 updated this revision to Diff 161263.Aug 17 2018, 8:30 AM
hugoeg marked 5 inline comments as done.

applied corrections form comments

hugoeg added inline comments.Aug 17 2018, 8:36 AM
clang-tidy/abseil/NoInternalDepsCheck.cpp
20 ↗(On Diff #161130)

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

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

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
2 ↗(On Diff #160168)

I think the names are ok.

hugoeg marked 11 inline comments as done.Aug 20 2018, 7:19 AM
hugoeg updated this revision to Diff 161485.Aug 20 2018, 8:10 AM

fixed some minor things in the test

hokein added inline comments.Aug 20 2018, 8:29 AM
clang-tidy/abseil/NoInternalDepsCheck.cpp
20 ↗(On Diff #161485)

nit: LLVM code guideline uses Camel for variable names.

26 ↗(On Diff #161485)

typo: utiliy => utility.

51 ↗(On Diff #161485)

nit: remove the empty line.

20 ↗(On Diff #161130)

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);
docs/clang-tidy/checks/abseil-no-internal-deps.rst
8 ↗(On Diff #161485)

This sentence doesn't parse.

17 ↗(On Diff #161485)

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

18 ↗(On Diff #161485)

nit: space between foo and {.

hugoeg updated this revision to Diff 161494.Aug 20 2018, 9:05 AM
hugoeg marked 5 inline comments as done.

corrections applied

hugoeg marked an inline comment as done.Aug 20 2018, 10:05 AM
hugoeg added inline comments.
clang-tidy/abseil/NoInternalDepsCheck.cpp
20 ↗(On Diff #161130)

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 marked 2 inline comments as done.Aug 20 2018, 10:06 AM
hugoeg marked an inline comment as done.Aug 20 2018, 10:14 AM
hugoeg updated this revision to Diff 161550.Aug 20 2018, 1:08 PM

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

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
24 ↗(On Diff #161550)

nit: loc => Loc

hugoeg updated this revision to Diff 161714.Aug 21 2018, 7:24 AM
hugoeg marked an inline comment as done.

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

hugoeg added a comment.EditedAug 21 2018, 7:43 AM

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 added inline comments.Aug 24 2018, 5:15 AM
docs/clang-tidy/checks/abseil-no-internal-deps.rst
17 ↗(On Diff #161714)

I think this line is also triggered the warning?

test/clang-tidy/abseil-no-internal-deps.cpp
8 ↗(On Diff #161714)

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 updated this revision to Diff 162379.Aug 24 2018, 7:41 AM
hugoeg marked 2 inline comments as done.

fixed comments

test/clang-tidy/abseil-no-internal-deps.cpp
8 ↗(On Diff #161714)

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

hugoeg marked an inline comment as done.Aug 24 2018, 8:43 AM

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

test/clang-tidy/abseil-no-internal-deps.cpp
8 ↗(On Diff #161714)

Yeah, I misread it as CHECK-MESSAGE.

kbobyrev added inline comments.
clang-tidy/abseil/NoInternalDepsCheck.cpp
24 ↗(On Diff #162379)

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

32 ↗(On Diff #162379)

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

hugoeg updated this revision to Diff 162864.Aug 28 2018, 7:54 AM
hugoeg marked 2 inline comments as done.

made some major updates after no-namespace landed

hokein added inline comments.Aug 28 2018, 8:15 AM
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 marked an inline comment as done.Aug 28 2018, 8:20 AM
hugoeg added inline comments.
test/clang-tidy/Inputs/absl/external-file.h
1 ↗(On Diff #162864)

how do I make it compilable?

hugoeg marked an inline comment as not done.Aug 28 2018, 8:21 AM

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

hugoeg updated this revision to Diff 162895.Aug 28 2018, 10:13 AM
hugoeg marked 3 inline comments as done.

fixed issues outlines in comments

hugoeg marked an inline comment as done.Aug 28 2018, 10:13 AM
hugoeg updated this revision to Diff 162921.Aug 28 2018, 11:46 AM

renamed check as suggested

hokein accepted this revision.Aug 29 2018, 4:39 AM

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.

This revision is now accepted and ready to land.Aug 29 2018, 4:39 AM
hugoeg updated this revision to Diff 163083.Aug 29 2018, 7:16 AM
hugoeg marked an inline comment as done.

nit comment fixed

nit comment fixed

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

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!

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.

This revision was automatically updated to reflect the committed changes.

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.

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