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

Authored by hugoeg on Thu, Aug 9, 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

hugoeg created this revision.Thu, Aug 9, 5:44 PM
hugoeg updated this revision to Diff 160042.Thu, Aug 9, 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.

22

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.Thu, Aug 9, 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.Fri, Aug 10, 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.Fri, Aug 10, 8:23 AM
hugoeg marked 10 inline comments as done.

Applied corrections from first round comments

hugoeg added inline comments.Fri, Aug 10, 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.Fri, Aug 10, 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.Fri, Aug 10, 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.Fri, Aug 10, 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.Sun, Aug 12, 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.Mon, Aug 13, 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.Mon, Aug 13, 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.Mon, Aug 13, 8:27 AM

most corrections from comments have been applied

JonasToth added inline comments.Mon, Aug 13, 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.Mon, Aug 13, 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.Mon, Aug 13, 9:17 AM
hugoeg marked an inline comment as done.
hugoeg added inline comments.Mon, Aug 13, 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.Mon, Aug 13, 9:59 AM
hugoeg marked 3 inline comments as done.
JonasToth added inline comments.Mon, Aug 13, 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.Mon, Aug 13, 1:20 PM
hugoeg marked an inline comment as done.

applied corrections from comments

JonasToth added inline comments.Tue, Aug 14, 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.Tue, Aug 14, 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.Thu, Aug 16, 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.Thu, Aug 16, 3:25 PM

added IsInAbseilFile Matcher

JonasToth added inline comments.Fri, Aug 17, 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.Fri, Aug 17, 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.Fri, Aug 17, 8:30 AM
hugoeg marked 5 inline comments as done.

applied corrections form comments

hugoeg added inline comments.Fri, Aug 17, 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