Skip to content

[clang-tidy][IncludeCleaner] Fix analysis supression in presence of verbatim spellings #68185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 5, 2023
Merged

[clang-tidy][IncludeCleaner] Fix analysis supression in presence of verbatim spellings #68185

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91ce80c
[clang-tidy][IncludeCleaner] Fix analysis supression in presence of v…
kadircet Oct 4, 2023
22f7e18
Update comment
kadircet Oct 4, 2023
1fd5860
Update docs
kadircet Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 13 additions & 7 deletions clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "clang/Lex/Preprocessor.h"
#include "clang/Tooling/Core/Replacement.h"
#include "clang/Tooling/Inclusions/HeaderIncludes.h"
#include "clang/Tooling/Inclusions/StandardLibrary.h"
#include "llvm/ADT/DenseSet.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallVector.h"
Expand Down Expand Up @@ -97,9 +98,12 @@ bool IncludeCleanerCheck::shouldIgnore(const include_cleaner::Header &H) {
return llvm::any_of(IgnoreHeadersRegex, [&H](const llvm::Regex &R) {
switch (H.kind()) {
case include_cleaner::Header::Standard:
// We don't trim braces around standard library headers deliberately, so
// that they are only matched as <vector>, otherwise having just
// `.*/vector` might yield false positives.
return R.match(H.standard().name());
case include_cleaner::Header::Verbatim:
return R.match(H.verbatim());
return R.match(H.verbatim().trim("<>\""));
case include_cleaner::Header::Physical:
return R.match(H.physical().getFileEntry().tryGetRealPathName());
}
Expand Down Expand Up @@ -179,12 +183,14 @@ void IncludeCleanerCheck::check(const MatchFinder::MatchResult &Result) {
if (getCurrentMainFile().endswith(PHeader))
continue;
}

if (llvm::none_of(
IgnoreHeadersRegex,
[Resolved = (*I.Resolved).getFileEntry().tryGetRealPathName()](
const llvm::Regex &R) { return R.match(Resolved); }))
Unused.push_back(&I);
auto StdHeader = tooling::stdlib::Header::named(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you could move this passage into the handling of standard headers in shouldIgnore. Then the below could also be simplified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ATM shouldIgnore is operating on an include_cleaner::Header (for good reason, as we need to use it both for missing and unused includes). Hence we can't really go from a physical Header to a stdlib one inside shouldIgnore. We can add overloads to accept both a Header and an Include, then we can move this logic into the relevant overload, but I don't think that'll be any cleaner.

Did you have something else in mind?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks, then what about something like (apologies for small mistakes, just typing into the comment window):

include_cleaner::Header getHeader(include_cleaner::Include I) {
    auto StdHeader = tooling::stdlib::Header::named(I.quote(), PP->getLangOpts().CPlusPlus ? tooling::stdlib::Lang::CXX :  tooling::stdlib::Lang::C);

   if (StdHeader) 
      return include_cleaner::Header(*StdHeader);
   return include_cleaner::Header(I.Resolved);
}

auto Header = getHeader(I);
if (shouldIgnore(Header))
 continue;

I'm trying to simplify the flow in the check method, as it seems like it's getting bogged down in implementation details. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be even better, but it would be a regression to current users. ATM this code is checking both the StdHeader and the Resolved header. In your suggestion we would only check for one or the other.

I.quote(), PP->getLangOpts().CPlusPlus ? tooling::stdlib::Lang::CXX
: tooling::stdlib::Lang::C);
if (StdHeader && shouldIgnore(*StdHeader))
continue;
if (shouldIgnore(*I.Resolved))
continue;
Unused.push_back(&I);
}

llvm::StringRef Code = SM->getBufferData(SM->getMainFileID());
Expand Down
16 changes: 13 additions & 3 deletions clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,20 @@ TEST(IncludeCleanerCheckTest, SuppressUnusedIncludes) {
#include "foo/qux.h"
#include "baz/qux/qux.h"
#include <vector>
#include <list>
)";

const char *PostCode = R"(
#include "bar.h"
#include "foo/qux.h"
#include <vector>
#include <list>
)";

std::vector<ClangTidyError> Errors;
ClangTidyOptions Opts;
Opts.CheckOptions["IgnoreHeaders"] = llvm::StringRef{llvm::formatv(
"bar.h;{0};{1};vector",
"bar.h;{0};{1};vector;<list>;",
llvm::Regex::escape(appendPathFileSystemIndependent({"foo", "qux.h"})),
llvm::Regex::escape(appendPathFileSystemIndependent({"baz", "qux"})))};
EXPECT_EQ(
Expand All @@ -79,6 +81,7 @@ TEST(IncludeCleanerCheckTest, SuppressUnusedIncludes) {
PreCode, &Errors, "file.cpp", std::nullopt, Opts,
{{"bar.h", "#pragma once"},
{"vector", "#pragma once"},
{"list", "#pragma once"},
{appendPathFileSystemIndependent({"foo", "qux.h"}), "#pragma once"},
{appendPathFileSystemIndependent({"baz", "qux", "qux.h"}),
"#pragma once"}}));
Expand Down Expand Up @@ -163,30 +166,37 @@ TEST(IncludeCleanerCheckTest, SuppressMissingIncludes) {
int BarResult = bar();
int BazResult = baz();
int QuxResult = qux();
int PrivResult = test();
std::vector x;
)";

ClangTidyOptions Opts;
Opts.CheckOptions["IgnoreHeaders"] = llvm::StringRef{
"baz.h;" +
"public.h;<vector>;baz.h;" +
llvm::Regex::escape(appendPathFileSystemIndependent({"foo", "qux.h"}))};
std::vector<ClangTidyError> Errors;
EXPECT_EQ(PreCode, runCheckOnCode<IncludeCleanerCheck>(
PreCode, &Errors, "file.cpp", std::nullopt, Opts,
{{"bar.h", R"(#pragma once
#include "baz.h"
#include "foo/qux.h"
#include "private.h"
int bar();
namespace std { struct vector {}; }
)"},
{"baz.h", R"(#pragma once
int baz();
)"},
{"private.h", R"(#pragma once
// IWYU pragma: private, include "public.h"
int test();
)"},
{appendPathFileSystemIndependent({"foo", "qux.h"}),
R"(#pragma once
int qux();
)"}}));
}


TEST(IncludeCleanerCheckTest, MultipleTimeMissingInclude) {
const char *PreCode = R"(
#include "bar.h"
Expand Down