Page MenuHomePhabricator

[clang-tidy] Add misc-header-include-cycle check
Needs ReviewPublic

Authored by PiotrZSL on Sun, Feb 26, 8:31 AM.

Details

Summary

Check detects cyclic #include dependencies between user-defined headers.

Diff Detail

Event Timeline

PiotrZSL created this revision.Sun, Feb 26, 8:31 AM
Herald added a project: Restricted Project. · View Herald Transcript
PiotrZSL updated this revision to Diff 500591.Sun, Feb 26, 8:32 AM

Typo fix.

Eugene.Zelenko added a project: Restricted Project.

Findings on llvm:

clang-tools-extra/unittests/clang-doc/ClangDocTest.h:12:10: warning: direct self-inclusion of header file 'ClangDocTest.h' [misc-header-include-cycle]
libc/src/__support/FPUtil/x86_64/LongDoubleBits.h:21:10: warning: circular header file dependency detected while including 'FPBits.h', please check the include path: 'FPBits.h' -> 'LongDoubleBits.h' -> 'FPBits.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBAddress.h:13:10: warning: circular header file dependency detected while including 'SBModule.h', please check the include path: 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' -> 'SBAddress.h' -> 'SBModule.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBBlock.h:14:10: warning: circular header file dependency detected while including 'SBTarget.h', please check the include path: 'SBTarget.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBFunction.h:12:10: warning: circular header file dependency detected while including 'SBAddress.h', please check the include path: 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBFunction.h' -> 'SBAddress.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBLineEntry.h:12:10: warning: circular header file dependency detected while including 'SBAddress.h', please check the include path: 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBLineEntry.h' -> 'SBAddress.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBModule.h:15:10: warning: circular header file dependency detected while including 'SBSymbolContext.h', please check the include path: 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbol.h:12:10: warning: circular header file dependency detected while including 'SBAddress.h', please check the include path: 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBSymbol.h' -> 'SBAddress.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbol.h:15:10: warning: circular header file dependency detected while including 'SBTarget.h', please check the include path: 'SBTarget.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBSymbol.h' -> 'SBTarget.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContext.h:12:10: warning: circular header file dependency detected while including 'SBBlock.h', please check the include path: 'SBBlock.h' -> 'SBTarget.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBBlock.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContext.h:15:10: warning: circular header file dependency detected while including 'SBFunction.h', please check the include path: 'SBFunction.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBFunction.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContext.h:16:10: warning: circular header file dependency detected while including 'SBLineEntry.h', please check the include path: 'SBLineEntry.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBLineEntry.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContext.h:17:10: warning: circular header file dependency detected while including 'SBModule.h', please check the include path: 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBModule.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContext.h:18:10: warning: circular header file dependency detected while including 'SBSymbol.h', please check the include path: 'SBSymbol.h' -> 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBSymbol.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBSymbolContextList.h:13:10: warning: circular header file dependency detected while including 'SBSymbolContext.h', please check the include path: 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' -> 'SBSymbolContextList.h' -> 'SBSymbolContext.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBTarget.h:12:10: warning: circular header file dependency detected while including 'SBAddress.h', please check the include path: 'SBAddress.h' -> 'SBModule.h' -> 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' -> 'SBAddress.h' [misc-header-include-cycle]
lldb/include/lldb/API/SBTarget.h:20:10: warning: circular header file dependency detected while including 'SBSymbolContextList.h', please check the include path: 'SBSymbolContextList.h' -> 'SBSymbolContext.h' -> 'SBBlock.h' -> 'SBTarget.h' -> 'SBSymbolContextList.h' [misc-header-include-cycle]
lldb/include/lldb/lldb-private-types.h:14:10: warning: circular header file dependency detected while including 'lldb-private.h', please check the include path: 'lldb-private.h' -> 'lldb-private-types.h' -> 'lldb-private.h' [misc-header-include-cycle]
lldb/include/lldb/lldb-private.h:16:10: warning: circular header file dependency detected while including 'lldb-private-types.h', please check the include path: 'lldb-private-types.h' -> 'lldb-private.h' -> 'lldb-private-types.h' [misc-header-include-cycle]
lldb/source/Plugins/Process/scripted/ScriptedProcess.h:17:10: warning: circular header file dependency detected while including 'ScriptedThread.h', please check the include path: 'ScriptedThread.h' -> 'ScriptedProcess.h' -> 'ScriptedThread.h' [misc-header-include-cycle]
lldb/source/Plugins/Process/scripted/ScriptedThread.h:14:10: warning: circular header file dependency detected while including 'ScriptedProcess.h', please check the include path: 'ScriptedProcess.h' -> 'ScriptedThread.h' -> 'ScriptedProcess.h' [misc-header-include-cycle]
third-party/unittest/googletest/include/gtest/gtest_pred_impl.h:43:10: warning: circular header file dependency detected while including 'gtest.h', please check the include path: 'gtest.h' -> 'gtest_pred_impl.h' -> 'gtest.h' [misc-header-include-cycle]
PiotrZSL published this revision for review.Tue, Feb 28, 12:04 AM

Ready for review

This is a great check that I've been meaning to work on for ages but never had time.
It mostly looks good but there are a few issues
I feel there is another issue that this can be very spammy with diagnostics(though clang-tidy is likely suppressing the duplicated ones).
But maybe when we emit a warning for a specified include, we could put it in a set to not warn on that include again

clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
107–111

How does this handle this case where an include includes a file that self includes itself.

// Foo.hpp
#pragma once
#include "Foo.hpp"

// Bar.hpp
#pragma once
#include Foo.hpp

// Main.cpp
#include "Bar.hpp"

Where will the warning be emitted, it should appear in Foo.hpp but the logic here says it won't and would just be reported as a circular dependency below.
Can you make sure the warning is emitted in Foo.hpp and add test cases that test for this, The current tests just check include a file that self references itself with no intermediate files.

113–123

Rather than just printing out the include path, can you emit each include location as a note.
OtherInclude.h:5:1: note: 'ThisInclude.h' included here
You can pass DiagnosticLevel::Note as the 3rd parameter to Check::diag to emit a note. Just have to make sure you emit the note after you emit the warning.

Notes have better support for front end diagnostics parsing.

135

1024 entries is pushing the boat out for a small vector.

clang-tools-extra/docs/clang-tidy/checks/misc/header-include-cycle.rst
64

This tag line is just unnecessary

PiotrZSL added inline comments.Tue, Feb 28, 9:53 AM
clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
107–111

in theory it should raise an issue for self-include here..
and issue should be raised in Foo.hpp, can add check for this...

113–123

yes I wanted to do that, but at the point of FileChanged, where I fill up stack, i don't have an proper info where such file wre included.
It's possible, would need just to somehow connect info from InclusionDirective, assuming that when we got include, we go in such file, then probably keeping last processed include should be sufficient...

135

Thats just 4KB of memory pre-allocated to avoid re-allocations. Small is "muten" here.

clang-tools-extra/docs/clang-tidy/checks/misc/header-include-cycle.rst
64

maybe it is unnecessary, but I just wanted to put some advertisement slogan for a check ...
Doesn't need to be bold... But it's is catchy...

PiotrZSL planned changes to this revision.Tue, Feb 28, 12:57 PM
PiotrZSL marked 4 inline comments as done.Sun, Mar 5, 12:41 PM

"But maybe when we emit a warning for a specified include, we could put it in a set to not warn on that include again"
I don't think so, in such case we hide include paths, and most of the time header guards should help here in reduction of cyclic dependences.

PiotrZSL updated this revision to Diff 502464.Sun, Mar 5, 12:44 PM

Added support for --include
Added support for self-include of source file
Added more tests
Reworked include stack.

TODO: Add option: ExcludeHeadersRegexp

PiotrZSL planned changes to this revision.Sun, Mar 5, 1:15 PM

Option need to be added

PiotrZSL updated this revision to Diff 503122.Tue, Mar 7, 12:04 PM

Ready for review, added new check option.

PiotrZSL updated this revision to Diff 503144.Tue, Mar 7, 1:18 PM

Rebase due to failed clangd tests on windows

PiotrZSL updated this revision to Diff 503251.Wed, Mar 8, 12:18 AM

Rebase due to broken baseline