Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[clang-tidy] Implement an include-cleaner check.
ClosedPublic

Authored by VitaNuo on Apr 20 2023, 4:43 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
hokein added inline comments.May 31 2023, 2:00 AM
clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp
2

nit: missing the license file comment.

hokein added inline comments.May 31 2023, 2:29 AM
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
171–176

tooling::HeaderIncludes already handles the include style well (and more). In general, we recommend to use tooling::headerIncludes -- it was built for generic toolings that need to perform #include manipulations, it has been heavily used in clangd/clang-format/clang-include-cleaner etc.

IncludeInserter::createIncludeInsertion is an old implementation in clang-tidy, and it was built before tooling::HeaderIncludes is thing. In the long-term, I think we should probably consider deprecating it and replacing it with tooling::HeaderIncludes.

VitaNuo updated this revision to Diff 527001.May 31 2023, 5:01 AM
VitaNuo marked 25 inline comments as done.

Address review comments.

Thanks for the comments!

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
82

we should actually use FileLoc of the decl location here (i.e. map it back to spelling location) as the decl might be introduced by a macro expansion, but if the spelling of "primary" location belongs to the main file we should still analyze it (e.g. decls introduced via TEST macros)

also SourceManager::isInMainFile will follow #line directives, which can create confusion (a declaration from some other file will create a diagnostic in the main file), so let's use isWrittenInMainFile instead?

87

const will not work, since the container I need to populate has to have llvm::SmallVector<Decl *> type due to the include-cleaner library interfaces.

89

we should actually use FileLoc of the decl location here (i.e. map it back to spelling location) as the decl might be introduced by a macro expansion, but if the spelling of "primary" location belongs to the main file we should still analyze it (e.g. decls introduced via TEST macros)

we actually want spelling location, not fileloc
sorry for the confusion
basically, spelling location will always map to where a name is spelled in a > physical file, even if it's part of macro body
whereas getFileLoc, will map tokens from macro body to their expansion locations (i.e. place in a physical file where the macro is invoked)

These are earlier comments from Kadir on this topic. AFAIU we want the spelling location for TEST_F(WalkUsedTest, MultipleProviders) {... } because:

  • for Decls introduced by the TEST_F expansion, we would like to analyze them only if they are spelled in the main file.
  • for arguments, their transitive spelling location is where they're written. So if TEST_F(WalkUsedTest, MultipleProviders) {... } is written in the main file, the argument locations will be counted.
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
43

Thanks for the tip, I can use the OptionsUtils infrastructure indeed.

clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp
20

they don't, adding a declaration to the custom "vector.h" and then using it in the main file makes vector disappear from the list of unused includes.
On top of that, even if the test did somehow depend on the actual system headers, this wouldn't change anything in the test result, since the vector header would remain unused.

Eugene.Zelenko added inline comments.May 31 2023, 7:06 AM
clang-tools-extra/docs/ReleaseNotes.rst
197

Please keep alphabetical order (by check name) in this section.

200

One sentence is enough.

The only thing is the source location, see my comments for details, otherwise looks good in general.

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
89

I think I'm not convinced, see my comments below.

for Decls introduced by the TEST_F expansion, we would like to analyze them only if they are spelled in the main file.

I think it is true for some cases. For example, a function decl has different parts (declarator, and function body), if the declarator is introduced by a macro which defined in a header, and the function body is spelled in the main-file, we still want to analyze the function body, see a simple example below:

// foo.h
#define DECLARE(X) void X()

// main.cpp
#include "foo.h"

DECLARE(myfunc) {
   int a;
   ...
}

Moreover, we have use ExpansionLoc pattern in other places when we collect the main-file top-level decl (include-cleaner, clangd), and I don't see a special reason to change the pattern in the clang-tidy check.

117

it is sad that we duplicate the include-cleaner analyse implementation (the only difference is that here we record the references for missing-includes), I think we should find a way to share the code in the future.

No action required in this patch, can you add a FIXME?

195

I think we should use the spelling location -- Inc.SymRefLocation can be a macro location which can points to another file, we intend to show diagnostics for main-file only, and walkUsed has some internal logic which guarantees that the spelling loc of returned refs is main-file.

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
22

can you cleanup the includes here? looks like there are some unused-includes now, at least Syntax/Tokens.h from the first glance.

clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst
5

nit: remove extra trailing ==

clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp
2

nit: IncludeCleanerTest.cpp

VitaNuo updated this revision to Diff 527317.Jun 1 2023, 1:08 AM

Update release notes.

VitaNuo marked 2 inline comments as done.Jun 1 2023, 1:08 AM
VitaNuo updated this revision to Diff 527329.Jun 1 2023, 2:30 AM
VitaNuo marked 6 inline comments as done.

Address review comments.

Thanks for the comments!

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
89

Is the expansion location of myfunc in the main file? If that's the case, we need the expansion location indeed.
Otherwise, we need getFileLoc to map file locations from macro arguments to their spelling (in the main file above) and locations from macro bodies to the expansion location.

hokein accepted this revision.Jun 1 2023, 4:03 AM

Thanks, just a few nits. I think it is in a good shape, let's ship it!

Please make sure the premerge-test is happy before landing the patch, the windows premerge test is failing now (https://buildkite.com/llvm-project/premerge-checks/builds/155660#0188764d-dc9f-4e8f-b489-c7b8f8b0c52a)

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
89

Is the expansion location of myfunc in the main file? If that's the case, we need the expansion location indeed.

Right. The expansion location here is a file location which points to the main-file.

Would be nice if you add the above test into the unittest.

190–194

nit: we can simplify it like

if (auto Replacement = HeaderIncludes.insert(...))
   diag(...);
clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp
28

wrap all things into an anonymous namespace to avoid potential ODR violations.

71

Instead of using 4 += statements, I think it is a bit clearer to use llvm::formatv("bar.h;{1};{2};...", appendPathSystemIndependent(...), ....);

This revision is now accepted and ready to land.Jun 1 2023, 4:03 AM
VitaNuo updated this revision to Diff 527359.Jun 1 2023, 4:24 AM

Try to fix windows test.

VitaNuo updated this revision to Diff 527369.Jun 1 2023, 4:54 AM
VitaNuo marked an inline comment as done.

Remove system-independent handling to see behavior on Windows.

VitaNuo updated this revision to Diff 527376.Jun 1 2023, 5:42 AM
VitaNuo marked an inline comment as done.

Remove the regular expression.

VitaNuo updated this revision to Diff 527384.Jun 1 2023, 6:05 AM
VitaNuo marked an inline comment as done.

Remove regex and path handling.

VitaNuo updated this revision to Diff 527385.Jun 1 2023, 6:05 AM

Remove regex and path handling.

VitaNuo updated this revision to Diff 527392.Jun 1 2023, 6:24 AM
VitaNuo marked an inline comment as done.

Add debug statements for windows debugging.

VitaNuo updated this revision to Diff 527402.Jun 1 2023, 6:47 AM

Remove debug statements.

VitaNuo updated this revision to Diff 527457.Jun 1 2023, 9:28 AM

Re-introduce special path handling.

VitaNuo updated this revision to Diff 527473.Jun 1 2023, 10:02 AM

Try to fix windows build.

VitaNuo updated this revision to Diff 527509.Jun 1 2023, 10:59 AM

Try ignoring verbatim spelling.

VitaNuo updated this revision to Diff 527543.Jun 1 2023, 11:44 AM

Fix last test.

VitaNuo updated this revision to Diff 527554.Jun 1 2023, 12:07 PM

Experiment with paths.

VitaNuo updated this revision to Diff 527812.Jun 2 2023, 5:15 AM

Escape the slashes in regex.

VitaNuo updated this revision to Diff 527822.Jun 2 2023, 5:37 AM

Remove path handling.

VitaNuo updated this revision to Diff 527839.Jun 2 2023, 6:36 AM

Re-introduce path handling.

This revision was automatically updated to reflect the committed changes.
mgorny added a subscriber: mgorny.Jun 3 2023, 3:06 AM

I'm getting completely incomprehensible build errors on Gentoo from this, as of aa7eace8431ba213c5431638b894b0e1b4b481c7:

samu: job failed: : && /usr/lib/ccache/bin/x86_64-pc-linux-gnu-g++ -march=znver2 --param=l1-cache-size=32 --param=l1-cache-line-size=64 -O2 -pipe -frecord-gcc-switches -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -Wimplicit-fallthrough -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -pedantic -Wno-long-long -Wl,-O1 -Wl,--as-needed -Wl,--hash-style=gnu   -Wl,--export-dynamic -rdynamic  -Wl,-rpath-link,/var/tmp/portage/sys-devel/clang-17.0.0_pre20230603/work/x/y/clang-abi_x86_64.amd64/./lib64 tools/extra/clang-tidy/tool/CMakeFiles/clang-tidy.dir/ClangTidyToolMain.cpp.o -o bin/clang-tidy -L/usr/lib/llvm/17/lib64 -Wl,-rpath,"\$ORIGIN/../lib64:/usr/lib/llvm/17/lib64:/var/tmp/portage/sys-devel/clang-17.0.0_pre20230603/work/x/y/clang-abi_x86_64.amd64/lib64:"  lib64/libclangTidy.a  lib64/libclangTidyMain.a  lib64/libclangTidyAndroidModule.a  lib64/libclangTidyAbseilModule.a  lib64/libclangTidyAlteraModule.a  lib64/libclangTidyBoostModule.a  lib64/libclangTidyBugproneModule.a  lib64/libclangTidyCERTModule.a  lib64/libclangTidyConcurrencyModule.a  lib64/libclangTidyCppCoreGuidelinesModule.a  lib64/libclangTidyDarwinModule.a  lib64/libclangTidyFuchsiaModule.a  lib64/libclangTidyGoogleModule.a  lib64/libclangTidyHICPPModule.a  lib64/libclangTidyLinuxKernelModule.a  lib64/libclangTidyLLVMModule.a  lib64/libclangTidyLLVMLibcModule.a  lib64/libclangTidyMiscModule.a  lib64/libclangTidyModernizeModule.a  lib64/libclangTidyObjCModule.a  lib64/libclangTidyOpenMPModule.a  lib64/libclangTidyPerformanceModule.a  lib64/libclangTidyPortabilityModule.a  lib64/libclangTidyReadabilityModule.a  lib64/libclangTidyZirconModule.a  lib64/libclangTidyMPIModule.a  lib64/libclangTidyBugproneModule.a  lib64/libclangTidyCppCoreGuidelinesModule.a  lib64/libclangTidyGoogleModule.a  lib64/libclangTidyMiscModule.a  lib64/libclangAnalysis.a  lib64/libclangASTMatchers.a  lib64/libclangAST.a  lib64/libclangLex.a  lib64/libclangBasic.a  lib64/libclangTidyModernizeModule.a  lib64/libclangTidyReadabilityModule.a  lib64/libclangTidyUtils.a  lib64/libclangTidy.a  lib64/libclang-cpp.so.17gitaa7eace8  /usr/lib/llvm/17/lib64/libLLVM-17gitaa7eace8.so && :
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: lib64/libclangTidyMiscModule.a(IncludeCleanerCheck.cpp.o): in function `clang::tidy::misc::IncludeCleanerCheck::registerPPCallbacks(clang::SourceManager const&, clang::Preprocessor*, clang::Preprocessor*) [clone .localalias]':
IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck19registerPPCallbacksERKNS_13SourceManagerEPNS_12PreprocessorES7_+0x24): undefined reference to `clang::include_cleaner::RecordedPP::record(clang::Preprocessor const&)'
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck19registerPPCallbacksERKNS_13SourceManagerEPNS_12PreprocessorES7_+0x94): undefined reference to `clang::include_cleaner::PragmaIncludes::record(clang::Preprocessor&)'
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: lib64/libclangTidyMiscModule.a(IncludeCleanerCheck.cpp.o): in function `clang::tidy::misc::IncludeCleanerCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) [clone .localalias]':
IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck5checkERKNS_12ast_matchers11MatchFinder11MatchResultE+0x508): undefined reference to `clang::include_cleaner::walkUsed(llvm::ArrayRef<clang::Decl*>, llvm::ArrayRef<clang::include_cleaner::SymbolReference>, clang::include_cleaner::PragmaIncludes const*, clang::SourceManager const&, llvm::function_ref<void (clang::include_cleaner::SymbolReference const&, llvm::ArrayRef<clang::include_cleaner::Header>)>)'
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck5checkERKNS_12ast_matchers11MatchFinder11MatchResultE+0x604): undefined reference to `clang::include_cleaner::PragmaIncludes::getPublic(clang::FileEntry const*) const'
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck5checkERKNS_12ast_matchers11MatchFinder11MatchResultE+0x9e8): undefined reference to `clang::include_cleaner::Include::quote[abi:cxx11]() const'
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck5checkERKNS_12ast_matchers11MatchFinder11MatchResultE+0xd15): undefined reference to `clang::include_cleaner::spellHeader[abi:cxx11](clang::include_cleaner::Header const&, clang::HeaderSearch const&, clang::FileEntry const*)'
/usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: lib64/libclangTidyMiscModule.a(IncludeCleanerCheck.cpp.o): in function `void llvm::function_ref<void (clang::include_cleaner::SymbolReference const&, llvm::ArrayRef<clang::include_cleaner::Header>)>::callback_fn<clang::tidy::misc::IncludeCleanerCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&)::{lambda(clang::include_cleaner::SymbolReference const&, llvm::ArrayRef<clang::include_cleaner::Header>)#1}>(long, clang::include_cleaner::SymbolReference const&, llvm::ArrayRef<clang::include_cleaner::Header>)':
IncludeCleanerCheck.cpp:(.text._ZN4llvm12function_refIFvRKN5clang15include_cleaner15SymbolReferenceENS_8ArrayRefINS2_6HeaderEEEEE11callback_fnIZNS1_4tidy4misc19IncludeCleanerCheck5checkERKNS1_12ast_matchers11MatchFinder11MatchResultEEUlS5_S8_E_EEvlS5_S8_+0xd1): undefined reference to `clang::include_cleaner::Includes::match(clang::include_cleaner::Header) const'
collect2: error: ld returned 1 exit status

I'm getting completely incomprehensible build errors on Gentoo from this

I'm also hitting this; it only seems to happen if building with -DLLVM_LINK_LLVM_DYLIB=ON.

My educated guess would be that clangIncludeCleaner is being linked via clang_target_link_libraries while it's not part of libclang-cpp, so it should go into regular target_link_libraries.

My educated guess would be that clangIncludeCleaner is being linked via clang_target_link_libraries while it's not part of libclang-cpp, so it should go into regular target_link_libraries.

Yes, that does seem to do the trick. LGTM from my PoV if you can push such a fix, otherwise I can try to do it in a little while...

My educated guess would be that clangIncludeCleaner is being linked via clang_target_link_libraries while it's not part of libclang-cpp, so it should go into regular target_link_libraries.

Yes, that does seem to do the trick. LGTM from my PoV if you can push such a fix, otherwise I can try to do it in a little while...

Looks like it is fixed in https://github.com/llvm/llvm-project/commit/ac0ea7555ee4ae872bcd153e04513ba0b88b8985, thanks!