kbobyrev updated this revision to Diff 381190.
kbobyrev added a comment.
[clangd] IncludeCleaner: Implement more complicated rules for considering enums
forward declarations "used"
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111870/new/
https://reviews.llvm.org/D111870
Files:
clang-tools-extra/clangd/Config.h
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigYAML.cpp
clang-tools-extra/clangd/Diagnostics.cpp
clang-tools-extra/clangd/Diagnostics.h
clang-tools-extra/clangd/IncludeCleaner.cpp
clang-tools-extra/clangd/IncludeCleaner.h
clang-tools-extra/clangd/ParsedAST.cpp
clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
Index: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
+++ clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
@@ -103,6 +103,34 @@
"struct ^X { enum ^Language { ^CXX = 42, Python = 9000}; };",
"int Lang = X::CXX;",
},
+ {
+ "enum class Color;",
+ "enum class Color {};",
+ },
+ {
+ "enum class ^Color : int;",
+ "enum class Color : int {};",
+ },
+ {
+ "enum class Color : int {};",
+ "enum class Color : int;",
+ },
+ {
+ "enum class Color;",
+ "enum class Color {}; Color c;",
+ },
+ {
+ "enum class ^Color : char;",
+ "Color c;",
+ },
+ {
+ "enum class ^Color : char {};",
+ "Color c;",
+ },
+ {
+ "enum class ^Color;",
+ "Color c;",
+ },
{
// When a type is resolved via a using declaration, the
// UsingShadowDecl is not referenced in the AST.
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1464,6 +1464,45 @@
AllOf(Diag(Test.range("deprecated"), "'bar' is deprecated"),
WithTag(DiagnosticTag::Deprecated))));
}
+
+TEST(DiagnosticsTest, IncludeCleaner) {
+ Annotations Test(R"cpp(
+$fix[[ $diag[[#include "unused.h"]]
+]] #include "used.h"
+
+ void foo() {
+ used();
+ }
+ )cpp");
+ TestTU TU;
+ TU.Code = Test.code().str();
+ TU.AdditionalFiles["unused.h"] = R"cpp(
+ void unused() {}
+ )cpp";
+ TU.AdditionalFiles["used.h"] = R"cpp(
+ void used() {}
+ )cpp";
+ // Off by default.
+ EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+ Config Cfg;
+ Cfg.Diagnostics.IncludeCleaner.UnusedHeaders =
+ Config::UnusedHeadersPolicy::Strict;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+ EXPECT_THAT(
+ *TU.build().getDiagnostics(),
+ UnorderedElementsAre(
+ AllOf(Diag(Test.range("diag"), "included header is not used"),
+ WithTag(DiagnosticTag::Unnecessary), DiagSource(Diag::Clangd),
+ WithFix(Fix(Test.range("fix"), "", "remove unused header")))));
+ Cfg.Diagnostics.SuppressAll = true;
+ WithContextValue SuppressAllWithCfg(Config::Key, std::move(Cfg));
+ EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+ Cfg.Diagnostics.SuppressAll = false;
+ Cfg.Diagnostics.Suppress = {"clangd-unused-header"};
+ WithContextValue SuppressFilterWithCfg(Config::Key, std::move(Cfg));
+ EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -67,6 +67,8 @@
CheckOptions:
IgnoreMacros: true
example-check.ExampleOption: 0
+ IncludeCleaner:
+ UnusedHeaders: Strict
)yaml";
auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
EXPECT_THAT(Diags.Diagnostics, IsEmpty());
@@ -83,6 +85,9 @@
EXPECT_THAT(Results[3].Diagnostics.ClangTidy.CheckOptions,
ElementsAre(PairVal("IgnoreMacros", "true"),
PairVal("example-check.ExampleOption", "0")));
+ EXPECT_TRUE(Results[3].Diagnostics.IncludeCleaner.UnusedHeaders);
+ EXPECT_EQ("Strict",
+ *Results[3].Diagnostics.IncludeCleaner.UnusedHeaders.getValue());
}
TEST(ParseYAML, Locations) {
Index: clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -244,6 +244,25 @@
}
}
+TEST_F(ConfigCompileTests, DiagnosticsIncludeCleaner) {
+ // Defaults to None.
+ EXPECT_TRUE(compileAndApply());
+ EXPECT_EQ(Conf.Diagnostics.IncludeCleaner.UnusedHeaders,
+ Config::UnusedHeadersPolicy::None);
+
+ Frag = {};
+ Frag.Diagnostics.IncludeCleaner.UnusedHeaders.emplace("None");
+ EXPECT_TRUE(compileAndApply());
+ EXPECT_EQ(Conf.Diagnostics.IncludeCleaner.UnusedHeaders,
+ Config::UnusedHeadersPolicy::None);
+
+ Frag = {};
+ Frag.Diagnostics.IncludeCleaner.UnusedHeaders.emplace("Strict");
+ EXPECT_TRUE(compileAndApply());
+ EXPECT_EQ(Conf.Diagnostics.IncludeCleaner.UnusedHeaders,
+ Config::UnusedHeadersPolicy::Strict);
+}
+
TEST_F(ConfigCompileTests, DiagnosticSuppression) {
Frag.Diagnostics.Suppress.emplace_back("bugprone-use-after-move");
Frag.Diagnostics.Suppress.emplace_back("unreachable-code");
Index: clang-tools-extra/clangd/ParsedAST.cpp
===================================================================
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -18,6 +18,7 @@
#include "FeatureModule.h"
#include "Headers.h"
#include "HeuristicResolver.h"
+#include "IncludeCleaner.h"
#include "IncludeFixer.h"
#include "Preamble.h"
#include "SourceCode.h"
@@ -342,6 +343,7 @@
llvm::Optional<IncludeFixer> FixIncludes;
// No need to run clang-tidy or IncludeFixerif we are not going to surface
// diagnostics.
+ const Config &Cfg = Config::current();
if (PreserveDiags) {
trace::Span Tracer("ClangTidyInit");
tidy::ClangTidyOptions ClangTidyOpts =
@@ -366,7 +368,6 @@
Check->registerMatchers(&CTFinder);
}
- const Config &Cfg = Config::current();
ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) {
if (Cfg.Diagnostics.SuppressAll ||
@@ -515,10 +516,18 @@
Diags->insert(Diags->end(), D.begin(), D.end());
}
}
- return ParsedAST(Inputs.Version, std::move(Preamble), std::move(Clang),
+ ParsedAST Result(Inputs.Version, std::move(Preamble), std::move(Clang),
std::move(Action), std::move(Tokens), std::move(Macros),
std::move(Marks), std::move(ParsedDecls), std::move(Diags),
std::move(Includes), std::move(CanonIncludes));
+ if (Result.Diags) {
+ auto UnusedHeadersDiags =
+ issueUnusedHeadersDiagnostics(Result, Cfg, Inputs.Contents);
+ Result.Diags->insert(Result.Diags->end(),
+ make_move_iterator(UnusedHeadersDiags.begin()),
+ make_move_iterator(UnusedHeadersDiags.end()));
+ }
+ return Result;
}
ParsedAST::ParsedAST(ParsedAST &&Other) = default;
Index: clang-tools-extra/clangd/IncludeCleaner.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -21,6 +21,7 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_CLEANER_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INCLUDE_CLEANER_H
+#include "Config.h"
#include "Headers.h"
#include "ParsedAST.h"
#include "clang/Basic/SourceLocation.h"
@@ -63,6 +64,10 @@
std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
+std::vector<Diag> issueUnusedHeadersDiagnostics(ParsedAST &AST,
+ const Config &Cfg,
+ llvm::StringRef Code);
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/IncludeCleaner.cpp
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.cpp
+++ clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -7,9 +7,13 @@
//===----------------------------------------------------------------------===//
#include "IncludeCleaner.h"
+#include "Config.h"
+#include "Protocol.h"
+#include "SourceCode.h"
#include "support/Logger.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/Basic/SourceLocation.h"
+#include "llvm/Support/Casting.h"
namespace clang {
namespace clangd {
@@ -35,6 +39,16 @@
}
bool VisitTagType(TagType *TT) {
+ // For enumerations we will require only the definition if it's present and
+ // the underlying type is not specified.
+ if (TT->isEnumeralType()) {
+ const auto *Enum = llvm::dyn_cast<EnumDecl>(TT->getDecl());
+ assert(Enum);
+ if (!Enum->getIntegerTypeSourceInfo() && TT->getDecl()->getDefinition()) {
+ Result.insert(TT->getDecl()->getDefinition()->getLocation());
+ return true;
+ }
+ }
add(TT->getDecl());
return true;
}
@@ -67,29 +81,35 @@
}
bool TraverseType(QualType T) {
- if (isNew(T.getTypePtrOrNull())) { // don't care about quals
+ if (isNew(T.getTypePtrOrNull())) // don't care about quals
Base::TraverseType(T);
- }
return true;
}
bool VisitUsingDecl(UsingDecl *D) {
- for (const auto *Shadow : D->shadows()) {
+ for (const auto *Shadow : D->shadows())
add(Shadow->getTargetDecl());
- }
return true;
}
+ // Require redeclarations only for definitions and only when the underlying
+ // type is specified.
+ bool VisitEnumDecl(EnumDecl *D) {
+ if (D != D->getDefinition() || !D->getIntegerTypeSourceInfo())
+ return false;
+ for (const auto *Redecl : D->redecls())
+ Result.insert(Redecl->getLocation());
+ return false;
+ }
+
private:
using Base = RecursiveASTVisitor<ReferencedLocationCrawler>;
void add(const Decl *D) {
- if (!D || !isNew(D->getCanonicalDecl())) {
+ if (!D || !isNew(D->getCanonicalDecl()))
return;
- }
- for (const Decl *Redecl : D->redecls()) {
+ for (const Decl *Redecl : D->redecls())
Result.insert(Redecl->getLocation());
- }
}
bool isNew(const void *P) { return P && Visited.insert(P).second; }
@@ -133,6 +153,31 @@
}
};
+// Returns the range starting at '#' and ending at EOL.
+clangd::Range getDiagnosticRange(llvm::StringRef Code, unsigned Line,
+ unsigned HashOffset) {
+ clangd::Range Result;
+ Result.start.line = Line;
+ Result.end.line = Line;
+
+ // Calculate the offset from beginning of the file, EOL to '#'.
+ llvm::StringRef Prefix = Code.drop_back(Code.size() - HashOffset);
+ while (!Prefix.empty() && Prefix.back() != '\n' && Prefix.back() != '\r') {
+ Prefix = Prefix.drop_back();
+ }
+ unsigned HashLineOffset = lspLength(
+ Code.drop_back(Code.size() - HashOffset).drop_front(Prefix.size()));
+
+ // Span the warning until the EOL or EOF.
+ Result.start.character = HashLineOffset;
+ Result.end.character =
+ HashLineOffset +
+ lspLength(Code.drop_front(HashOffset).take_until([](char C) {
+ return C == '\n' || C == '\r';
+ }));
+ return Result;
+}
+
} // namespace
ReferencedLocations findReferencedLocations(ParsedAST &AST) {
@@ -206,5 +251,39 @@
return getUnused(AST.getIncludeStructure(), ReferencedFiles);
}
+std::vector<Diag> issueUnusedHeadersDiagnostics(ParsedAST &AST,
+ const Config &Cfg,
+ llvm::StringRef Code) {
+ if (Cfg.Diagnostics.IncludeCleaner.UnusedHeaders !=
+ Config::UnusedHeadersPolicy::Strict ||
+ Cfg.Diagnostics.SuppressAll ||
+ Cfg.Diagnostics.Suppress.contains("clangd-unused-header"))
+ return {};
+ std::vector<Diag> Result;
+ std::string FileName =
+ AST.getSourceManager()
+ .getFileEntryForID(AST.getSourceManager().getMainFileID())
+ ->getName()
+ .str();
+ for (const auto *Inc : computeUnusedIncludes(AST)) {
+ Diag D;
+ D.Message = "included header is not used";
+ D.Name = "unused-header";
+ D.Source = Diag::DiagSource::Clangd;
+ D.File = FileName;
+ D.Severity = DiagnosticsEngine::Warning;
+ D.Tags.push_back(Unnecessary);
+ D.Range = getDiagnosticRange(Code, Inc->HashLine, Inc->HashOffset);
+ D.Fixes.emplace_back();
+ D.Fixes.back().Message = "remove unused header";
+ D.Fixes.back().Edits.emplace_back();
+ D.Fixes.back().Edits.back().range.start.line = Inc->HashLine;
+ D.Fixes.back().Edits.back().range.end.line = Inc->HashLine + 1;
+ D.InsideMainFile = true;
+ Result.push_back(std::move(D));
+ }
+ return Result;
+}
+
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/Diagnostics.h
===================================================================
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -101,6 +101,7 @@
Unknown,
Clang,
ClangTidy,
+ Clangd,
ClangdConfig,
} Source = Unknown;
/// Elaborate on the problem, usually pointing to a related piece of code.
Index: clang-tools-extra/clangd/Diagnostics.cpp
===================================================================
--- clang-tools-extra/clangd/Diagnostics.cpp
+++ clang-tools-extra/clangd/Diagnostics.cpp
@@ -485,6 +485,9 @@
case Diag::ClangTidy:
Main.source = "clang-tidy";
break;
+ case Diag::Clangd:
+ Main.source = "clangd";
+ break;
case Diag::ClangdConfig:
Main.source = "clangd-config";
break;
Index: clang-tools-extra/clangd/ConfigYAML.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigYAML.cpp
+++ clang-tools-extra/clangd/ConfigYAML.cpp
@@ -118,10 +118,19 @@
if (auto Values = scalarValues(N))
F.Suppress = std::move(*Values);
});
+ Dict.handle("IncludeCleaner", [&](Node &N) { parse(F.IncludeCleaner, N); });
Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); });
Dict.parse(N);
}
+ void parse(Fragment::DiagnosticsBlock::IncludeCleanerBlock &F, Node &N) {
+ DictParser Dict("IncludeCleaner", this);
+ Dict.handle("UnusedHeaders", [&](Node &N) {
+ F.UnusedHeaders = scalarValue(N, "UnusedIncludes");
+ });
+ Dict.parse(N);
+ }
+
void parse(Fragment::DiagnosticsBlock::ClangTidyBlock &F, Node &N) {
DictParser Dict("ClangTidy", this);
Dict.handle("Add", [&](Node &N) {
Index: clang-tools-extra/clangd/ConfigFragment.h
===================================================================
--- clang-tools-extra/clangd/ConfigFragment.h
+++ clang-tools-extra/clangd/ConfigFragment.h
@@ -210,6 +210,22 @@
/// This often has other advantages, such as skipping some analysis.
std::vector<Located<std::string>> Suppress;
+ /// Controls how clangd will treat and warn users on suboptimal include
+ /// usage.
+ struct IncludeCleanerBlock {
+ /// When set to Strict, IncludeCleaner will warn about all headers that
+ /// are not used (no symbols referenced in the main file come from that
+ /// header) in the main file but are directly included from it. Removing
+ /// that header might break the code if it transitively includes used
+ /// headers and these headers are not included directly.
+ ///
+ /// Valid values are:
+ /// - Strict
+ /// - None
+ llvm::Optional<Located<std::string>> UnusedHeaders;
+ };
+ IncludeCleanerBlock IncludeCleaner;
+
/// Controls how clang-tidy will run over the code base.
///
/// The settings are merged with any settings found in .clang-tidy
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ clang-tools-extra/clangd/ConfigCompile.cpp
@@ -414,6 +414,7 @@
C.Diagnostics.Suppress.insert(N);
});
+ compile(std::move(F.IncludeCleaner));
compile(std::move(F.ClangTidy));
}
@@ -458,6 +459,18 @@
CurSpec += Str;
}
+ void compile(Fragment::DiagnosticsBlock::IncludeCleanerBlock &&F) {
+ if (F.UnusedHeaders)
+ if (auto Val = compileEnum<Config::UnusedHeadersPolicy>("IncludeCleaner",
+ **F.UnusedHeaders)
+ .map("Strict", Config::UnusedHeadersPolicy::Strict)
+ .map("None", Config::UnusedHeadersPolicy::None)
+ .value())
+ Out.Apply.push_back([Val](const Params &, Config &C) {
+ C.Diagnostics.IncludeCleaner.UnusedHeaders = *Val;
+ });
+ }
+
void compile(Fragment::DiagnosticsBlock::ClangTidyBlock &&F) {
std::string Checks;
for (auto &CheckGlob : F.Add)
Index: clang-tools-extra/clangd/Config.h
===================================================================
--- clang-tools-extra/clangd/Config.h
+++ clang-tools-extra/clangd/Config.h
@@ -86,6 +86,7 @@
ExternalIndexSpec External;
} Index;
+ enum UnusedHeadersPolicy { Strict, None };
/// Controls warnings and errors when parsing code.
struct {
bool SuppressAll = false;
@@ -97,6 +98,10 @@
std::string Checks;
llvm::StringMap<std::string> CheckOptions;
} ClangTidy;
+
+ struct {
+ UnusedHeadersPolicy UnusedHeaders = None;
+ } IncludeCleaner;
} Diagnostics;
/// Style of the codebase.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
