Adding a check to restrict system includes to a whitelist. Given a list of includes that are explicitly allowed, the check issues a fixit to remove any system include not on that list from the source file.
Details
- Reviewers
aaron.ballman hokein ilya-biryukov - Commits
- rG02aba94e768c: [clang-tidy] Adding RestrictSystemIncludes check to Fuchsia module
rG06af01bbc23d: [clang-tidy] Adding RestrictSystemIncludes check to Fuchsia module
rCTE332125: [clang-tidy] Adding RestrictSystemIncludes check to Fuchsia module
rL332125: [clang-tidy] Adding RestrictSystemIncludes check to Fuchsia module
rL331930: [clang-tidy] Adding RestrictSystemIncludes check to Fuchsia module
rCTE331930: [clang-tidy] Adding RestrictSystemIncludes check to Fuchsia module
Diff Detail
- Repository
- rL LLVM
Event Timeline
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. |
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? |
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 |
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. |
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. |
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. |
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. |
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? |
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. |
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. |
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. |
clang-tidy/fuchsia/RestrictSystemIncludesCheck.h | ||
---|---|---|
35 ↗ | (On Diff #146349) | GlobList::contains isn't const, so it can't... |
clang-tidy/fuchsia/RestrictSystemIncludesCheck.h | ||
---|---|---|
35 ↗ | (On Diff #146349) | Bah, I didn't see that. You can ignore, sorry for the noise. |