This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add misc-header-include-cycle check
ClosedPublic

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

Diff Detail

Event Timeline

PiotrZSL created this revision.Feb 26 2023, 8:31 AM
Herald added a project: Restricted Project. · View Herald Transcript
PiotrZSL updated this revision to Diff 500591.Feb 26 2023, 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.Feb 28 2023, 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.Feb 28 2023, 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.Feb 28 2023, 12:57 PM
PiotrZSL marked 4 inline comments as done.Mar 5 2023, 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.Mar 5 2023, 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.Mar 5 2023, 1:15 PM

Option need to be added

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

Ready for review, added new check option.

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

Rebase due to failed clangd tests on windows

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

Rebase due to broken baseline

LegalizeAdulthood resigned from this revision.Mar 29 2023, 8:03 AM
PiotrZSL updated this revision to Diff 513694.Apr 14 2023, 11:42 AM

Ping + Rebase

njames93 added inline comments.Apr 15 2023, 3:34 AM
clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
135–136

Is this a case that can ever come up?
If it does come up, surely any notes emitted after that would be garbage, in which case wouldn't it make more sense to break rather than continue?

147

Again whats the rationale for using continue over break here?

149–151

Is this a case that is expected to come up, if so can there be a test added.
Likewise if we do have a file that was included from the command line, surely we should break after emitting that diagnostic, It's not like the command line can be included from another file.

155–162

Dont reinvent the wheel - llvm::sys::path::filename does the same thing

PiotrZSL marked 4 inline comments as done.Apr 15 2023, 4:19 AM
PiotrZSL added inline comments.
clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
135–136

I will remove this...

147

Idea is to provide any information, even if it could be missing something.

149–151

I didn't found any case it could be, added this as an preclusion.

PiotrZSL updated this revision to Diff 513886.Apr 15 2023, 4:19 AM
PiotrZSL marked 3 inline comments as done.

Small redesign (review comments)

njames93 accepted this revision.Apr 15 2023, 5:37 AM

LGTM: Just one small nit.

clang-tools-extra/clang-tidy/misc/HeaderIncludeCycleCheck.cpp
149–151
This revision is now accepted and ready to land.Apr 15 2023, 5:37 AM
PiotrZSL updated this revision to Diff 513896.Apr 15 2023, 5:43 AM
PiotrZSL marked an inline comment as done.

Fix one small nit.

This revision was landed with ongoing or failed builds.Apr 15 2023, 5:44 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Apr 15 2023, 6:13 AM

This breaks tests: http://45.33.8.238/linux/104455/step_8.txt

Please take a look and revert for now if it takes a while to fix.

PiotrZSL reopened this revision.Apr 15 2023, 6:21 AM
This revision is now accepted and ready to land.Apr 15 2023, 6:21 AM
PiotrZSL updated this revision to Diff 513898.Apr 15 2023, 6:30 AM

Small changes remove, file name

PiotrZSL added a comment.EditedApr 15 2023, 6:31 AM

To be honest I have no idea why it failed, simply because there is CHECK-MESSAGES for that log. I will probably split this single test into few smaller tests.

PiotrZSL planned changes to this revision.Apr 15 2023, 8:52 AM

TODO:
Split tests into multiple files (to isolate issue).
If possible remove usage of FileCheck.

This revision is now accepted and ready to land.Jun 23 2023, 7:14 AM
This revision was automatically updated to reflect the committed changes.
PiotrZSL reopened this revision.Jun 24 2023, 3:00 AM
This revision is now accepted and ready to land.Jun 24 2023, 3:00 AM
PiotrZSL updated this revision to Diff 534183.Jun 24 2023, 3:00 AM

Reorder warnigns & copy .cpp file to Output

This revision was automatically updated to reflect the committed changes.