This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Adding RestrictIncludes check to Fuchsia module
ClosedPublic

Authored by juliehockett on Feb 26 2018, 12:42 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

juliehockett created this revision.Feb 26 2018, 12:42 PM
Eugene.Zelenko added inline comments.
clang-tidy/fuchsia/CMakeLists.txt
8 ↗(On Diff #135950)

Will be good idea to be have consistent name and documentation terminology: include versus header.

clang-tidy/fuchsia/RestrictIncludesCheck.cpp
48 ↗(On Diff #135950)

Please use using and run Clang-tidy modernize.

70 ↗(On Diff #135950)

Please don't use auto here, since type is not obvious.

80 ↗(On Diff #135950)

Please include <cstring>.

81 ↗(On Diff #135950)

Please don't use auto here, since type is not obvious.

101 ↗(On Diff #135950)

I don't think that you should prefix llvm with ::

clang-tidy/fuchsia/RestrictIncludesCheck.h
25 ↗(On Diff #135950)

Please run Clang-format over code.

30 ↗(On Diff #135950)

Please separate constructor with empty line from rest of methods.

hokein added inline comments.Feb 27 2018, 3:05 AM
docs/clang-tidy/checks/fuchsia-restrict-includes.rst
27 ↗(On Diff #135950)

The check seems do nothing with the default option.

Do we have a corresponding guideline of fuchsia?

aaron.ballman added inline comments.Feb 27 2018, 7:52 AM
clang-tidy/fuchsia/RestrictIncludesCheck.cpp
37–39 ↗(On Diff #135950)

Please don't suffix with an underscore -- you can actually drop the underscore here (we allow constructor arguments to hide the names of class members).

48–49 ↗(On Diff #135950)

Are you sure that vector and map are the best data types to use, rather than LLVM ADTs?

Also you can drop the elaborated type specifier for FileID.

69 ↗(On Diff #135950)

const auto &

74 ↗(On Diff #135950)

This comment doesn't really add much value.

75 ↗(On Diff #135950)

Diagnostics are not complete sentences, so you should remove the punctuation and capitalization here.

clang-tidy/fuchsia/RestrictIncludesCheck.h
32 ↗(On Diff #135950)

Function can be const and return a const ref.

test/clang-tidy/fuchsia-restrict-includes.cpp
13 ↗(On Diff #135950)

I think this could use some edge case tests for awful things people can do:

#define foo <stdio.h>

#include foo

# /* hahaha */ include /* oops */ foo
juliehockett marked 16 inline comments as done.

Fixing comments and updating tests.

aaron.ballman added inline comments.Feb 27 2018, 11:29 AM
clang-tidy/fuchsia/RestrictIncludesCheck.cpp
42–44 ↗(On Diff #136096)

We don't really use Doxygen comments for local class stuff, so you can convert these into regular comments.

75 ↗(On Diff #136096)

I believe this check will be finding transitive includes, correct? If so, maybe this should be two diagnostics: one to say "'%0' is prohibited from being included" and a note for the transitive case to say "file included transitively from here"?

Regardless of the behavior you'd like for it, we should have a test case:

// a.h
// Assumed that a.h is prohibited
void f();

// b.h
#include "a.h"

// c.c
#include "b.h"

int main() {
  f();
}
83 ↗(On Diff #136096)

Are you okay with breaking user's code from this removal? If you remove the header file, but not the parts of the code that require that header file to be included, this FixIt will break code.

87 ↗(On Diff #136096)

Do you need to clear this? I don't think EndOfMainFile() can be called twice, can it?

92–95 ↗(On Diff #136096)

This can be replaced with: return llvm::is_contained(Check.getRestrictedIncludes(), IncludeFile); which suggests the private function isn't needed at all.

clang-tidy/fuchsia/RestrictIncludesCheck.h
20 ↗(On Diff #136096)

s/headers/includes

28 ↗(On Diff #136096)

There is a mild problem here (present in the previous form, using commas, as well): the separator is technically a legal character for a file name. You may want to provide a comment in the documentation that explicitly states a file with the delimiter in the name is not supported. e.g., a file named foo.h;bar.h is not something you can prohibit with this check. I don't think it's worth picking a different delimiter over given how unlikely this file name is in practice.

29 ↗(On Diff #136096)

The default value here seems very strange as it's not an include file. I assume this intended to use vector?

docs/clang-tidy/checks/fuchsia-restrict-includes.rst
28 ↗(On Diff #136096)

The list is not comma-separated, it's semicolon-separated.

juliehockett marked 9 inline comments as done.
juliehockett edited the summary of this revision. (Show Details)

Sorry for the delay in updating this -- check now restricts includes to a whitelist, rather than a blacklist, and only applies to system headers, to more strictly control which are allowed.

Also updated warning text and addressed comments.

clang-tidy/fuchsia/RestrictIncludesCheck.cpp
75 ↗(On Diff #136096)

It will flag, but not fix, if a disallowed file is transitively included *and* the relevant flag is passed (e.g. -system-headers for system, or -header-filter=.* for other. I changed the warning text for header warnings to clarify that.

83 ↗(On Diff #136096)

Yes, but I did add a note to the documentation to call that out.

docs/clang-tidy/checks/fuchsia-restrict-includes.rst
27 ↗(On Diff #135950)

The idea is to maintain a list of disallowed headers in the local .clang-tidy file, so it will be dependent on that. If no headers are restricted, the check won't do anything. I'll add documentation to that effect.

aaron.ballman added inline comments.May 8 2018, 7:43 AM
clang-tidy/fuchsia/FuchsiaTidyModule.cpp
41 ↗(On Diff #145547)

I think this should be named fuchsia-restrict-system-includes.

clang-tidy/fuchsia/RestrictIncludesCheck.cpp
60 ↗(On Diff #145547)

I'm not certain that IsAngled is sufficient. For instance, say the user prohibits use of vector, nothing prevents the user from writing #include "vector" to access the system header (assuming there is no user header with the same name). e.g., https://godbolt.org/g/Scfv3U

Perhaps we could use SourceManager::isInSystemHeader() or extract some logic from it to test whether the actual included file is in the system header search path?

docs/ReleaseNotes.rst
116 ↗(On Diff #145547)

Be explicit that this only impacts system headers.

docs/clang-tidy/checks/fuchsia-restrict-includes.rst
6–7 ↗(On Diff #145547)

Should also be explicit about system headers.

test/clang-tidy/Inputs/fuchsia-restrict-includes/system/r.h
2 ↗(On Diff #145547)

Please add in the EOF.

test/clang-tidy/Inputs/fuchsia-restrict-includes/system/transitive.h
2 ↗(On Diff #145547)

Please add in the EOF.

test/clang-tidy/fuchsia-restrict-includes.cpp
31 ↗(On Diff #145547)

Please add in the EOF.

juliehockett marked 10 inline comments as done.

Made the check for system headers more comprehensive & fixed newline issues

Eugene.Zelenko added inline comments.May 8 2018, 2:30 PM
docs/ReleaseNotes.rst
116 ↗(On Diff #145788)

Is it necessary to highlight that warnings will not be emitted in case of disallowed headers are not found? Same in documentation.

aaron.ballman added inline comments.May 8 2018, 3:22 PM
clang-tidy/fuchsia/RestrictSystemIncludesCheck.cpp
67 ↗(On Diff #145788)

Rather than use this StringMap, can you change InclusionDirective() to filter out includes you don't care about? Something along the lines of:

FileID FID = SM.translateFile(File);
assert(FID != FileID() && "Source Manager does not know about this header file");

bool Invalid;
const SLocEntry &Entry = SM.getSLocEntry(FID, &Invalid);
if (!Invalid && SrcMgr::isSystem(Entry.getFile().getFileCharacteristic())) {
  // It's a system file.
}

This way you don't have to store all the path information and test against paths in EndOfMainFile(), unless I'm missing something. Note, this is untested code and perhaps that assert will fire (maybe SourceManager is unaware of this file by the time InclusionDirective() is called).

Alternatively, you could alter InclusionDirective() to also pass in the CharacteristicKind calculated by Preprocessor::HandleIncludeDirective() as that seems like generally useful information for the callback to have on hand.

clang-tidy/fuchsia/RestrictSystemIncludesCheck.h
1–2 ↗(On Diff #145788)

This should only be on a single line -- you can remove some - characters.

docs/clang-tidy/checks/fuchsia-restrict-system-includes.rst
32 ↗(On Diff #145788)

This default seems a bit odd to me, but perhaps it's fine. What's novel is that the check is a no-op by default, so how do Fuchsia developers get the correct list? Or is there no canonical list and developers are expected to populate their own manually?

juliehockett marked 3 inline comments as done.

Updating the inclusiondirective to filter out non-system files

docs/ReleaseNotes.rst
116 ↗(On Diff #145788)

I'm not sure I understand what you're saying...are you saying if the documentation should not include anything about what happens in the case of no headers?

docs/clang-tidy/checks/fuchsia-restrict-system-includes.rst
32 ↗(On Diff #145788)

The idea is that it's a case-by-case basis -- this may change at some point in the future if we decide there's a standard whitelist of system includes across the board, but at the moment the thought is to allow everything in some places, and use this check to limit them in others. It'll need to be populated on a case-by-case basis, since different directories will have different requirements.

aaron.ballman added inline comments.May 9 2018, 1:58 PM
docs/clang-tidy/checks/fuchsia-restrict-system-includes.rst
32 ↗(On Diff #145788)

So there's never a case where you need to prohibit use of all system includes? Right now, an empty string means "allow all includes" while a non-empty string means "allow only these includes", so there's no way to prohibit all includes except by listing them manually. I'm wondering whether you want to use a glob for allowed file names, which gives you extra flexibility. e.g., * allows all includes (default value), foo.h allows only <foo.h>, experimental/* allows all headers in the experimental directory, cstd* allows <cstdint> while prohibiting <stdint.h>, and the empty string will disallow all system headers.

Perhaps you don't need that level of flexibility, but for the use cases I'm imagining it seems more user friendly to be able to write cstd* rather than manually listing out all the C++ headers explicitly.

This revision was not accepted when it landed; it landed in state Needs Review.May 9 2018, 3:29 PM
This revision was automatically updated to reflect the committed changes.
juliehockett reopened this revision.May 9 2018, 3:32 PM

Sorry, branches got crossed. Reverted and reopened.

Updating parameter list to be a glob-style list.

aaron.ballman accepted this revision.May 11 2018, 10:55 AM

Thank you for adding the glob feature, I think that will be very powerful in practice. Aside from a minor nit, this LGTM!

clang-tidy/fuchsia/RestrictSystemIncludesCheck.h
35 ↗(On Diff #146349)

Function can be marked const.

This revision is now accepted and ready to land.May 11 2018, 10:55 AM
juliehockett added inline comments.May 11 2018, 11:42 AM
clang-tidy/fuchsia/RestrictSystemIncludesCheck.h
35 ↗(On Diff #146349)

GlobList::contains isn't const, so it can't...

aaron.ballman added inline comments.May 11 2018, 12:01 PM
clang-tidy/fuchsia/RestrictSystemIncludesCheck.h
35 ↗(On Diff #146349)

Bah, I didn't see that. You can ignore, sorry for the noise.

This revision was automatically updated to reflect the committed changes.