This is an archive of the discontinued LLVM Phabricator instance.

[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

Event Timeline

hugoeg created this revision.Aug 9 2018, 5:44 PM
hugoeg updated this revision to Diff 160042.Aug 9 2018, 5:52 PM

add missing files

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.

25

Please add new line at end.

Eugene.Zelenko retitled this revision from [Clang-Tidy] Check for abseil to [clang-tidy] Add abseil-no-internal-deps check.Aug 9 2018, 7:05 PM
Eugene.Zelenko set the repository for this revision to rCTE Clang Tools Extra.
Eugene.Zelenko added a subscriber: cfe-commits.
JonasToth added inline comments.
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 added inline comments.Aug 10 2018, 5:54 AM
clang-tidy/abseil/NoInternalDepsCheck.cpp
39

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

39

s/abseil/Abseil/

hugoeg updated this revision to Diff 160112.Aug 10 2018, 8:23 AM
hugoeg marked 10 inline comments as done.

Applied corrections from first round comments

hugoeg added inline comments.Aug 10 2018, 8:25 AM
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 added inline comments.Aug 10 2018, 9:10 AM
clang-tidy/abseil/NoInternalDepsCheck.cpp
25

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

hugoeg updated this revision to Diff 160124.Aug 10 2018, 9:16 AM
hugoeg marked an inline comment as done.

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

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 updated this revision to Diff 160168.Aug 10 2018, 12:16 PM
hugoeg marked 5 inline comments as done.

corrections from comments applied

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 added inline comments.Aug 12 2018, 7:21 AM
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 added inline comments.Aug 13 2018, 1:10 AM
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 marked 8 inline comments as done.Aug 13 2018, 8:26 AM
hugoeg added inline comments.
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 updated this revision to Diff 160363.Aug 13 2018, 8:27 AM

most corrections from comments have been applied

JonasToth added inline comments.Aug 13 2018, 9:11 AM
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 added inline comments.Aug 13 2018, 9:12 AM
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 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
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?

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
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 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
6

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
20

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
20

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
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 added inline comments.Aug 17 2018, 7:41 AM
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 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
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

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
3

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
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 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
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 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
25

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
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 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
9

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
9

Yeah, I misread it as CHECK-MESSAGE.

kbobyrev added inline comments.
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 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

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

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

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