[clang-tools-extra] 6d314ee - [clangd] Add a way to enable IncludeCleaner through config
Kirill Bobyrev via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 26 03:53:21 PDT 2021
Author: Kirill Bobyrev
Date: 2021-10-26T12:53:05+02:00
New Revision: 6d314ee570975610ef85e8c56659f00857b49744
URL: https://github.com/llvm/llvm-project/commit/6d314ee570975610ef85e8c56659f00857b49744
DIFF: https://github.com/llvm/llvm-project/commit/6d314ee570975610ef85e8c56659f00857b49744.diff
LOG: [clangd] Add a way to enable IncludeCleaner through config
This is useful for dogfooding the feature and spotting bugs.
Reviewed By: sammccall
Differential Revision: https://reviews.llvm.org/D111870
Added:
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h
index 72694788cfbf..0e48d2f0ceb6 100644
--- a/clang-tools-extra/clangd/Config.h
+++ b/clang-tools-extra/clangd/Config.h
@@ -86,6 +86,7 @@ struct Config {
ExternalIndexSpec External;
} Index;
+ enum UnusedIncludesPolicy { Strict, None };
/// Controls warnings and errors when parsing code.
struct {
bool SuppressAll = false;
@@ -97,6 +98,8 @@ struct Config {
std::string Checks;
llvm::StringMap<std::string> CheckOptions;
} ClangTidy;
+
+ UnusedIncludesPolicy UnusedIncludes = None;
} Diagnostics;
/// Style of the codebase.
diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index ad117b3d77be..a7b881a28ac0 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -414,6 +414,16 @@ struct FragmentCompiler {
C.Diagnostics.Suppress.insert(N);
});
+ if (F.UnusedIncludes)
+ if (auto Val = compileEnum<Config::UnusedIncludesPolicy>(
+ "UnusedIncludes", **F.UnusedIncludes)
+ .map("Strict", Config::UnusedIncludesPolicy::Strict)
+ .map("None", Config::UnusedIncludesPolicy::None)
+ .value())
+ Out.Apply.push_back([Val](const Params &, Config &C) {
+ C.Diagnostics.UnusedIncludes = *Val;
+ });
+
compile(std::move(F.ClangTidy));
}
diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h
index 8e4110e6dba1..a9ca7b462ec1 100644
--- a/clang-tools-extra/clangd/ConfigFragment.h
+++ b/clang-tools-extra/clangd/ConfigFragment.h
@@ -210,6 +210,20 @@ struct Fragment {
/// This often has other advantages, such as skipping some analysis.
std::vector<Located<std::string>> Suppress;
+ /// Controls how clangd will correct "unnecessary #include directives.
+ /// clangd can warn if a header is `#include`d but not used, and suggest
+ /// removing it.
+ //
+ /// Strict means a header is unused if it does not *directly* provide any
+ /// symbol used in the file. Removing it may still break compilation if it
+ /// transitively includes headers that are used. This should be fixed by
+ /// including those headers directly.
+ ///
+ /// Valid values are:
+ /// - Strict
+ /// - None
+ llvm::Optional<Located<std::string>> UnusedIncludes;
+
/// Controls how clang-tidy will run over the code base.
///
/// The settings are merged with any settings found in .clang-tidy
diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp
index 8a493346d2ce..331c18333045 100644
--- a/clang-tools-extra/clangd/ConfigYAML.cpp
+++ b/clang-tools-extra/clangd/ConfigYAML.cpp
@@ -118,6 +118,9 @@ class Parser {
if (auto Values = scalarValues(N))
F.Suppress = std::move(*Values);
});
+ Dict.handle("UnusedIncludes", [&](Node &N) {
+ F.UnusedIncludes = scalarValue(N, "UnusedIncludes");
+ });
Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); });
Dict.parse(N);
}
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index c4f174993e2d..6dd69192497f 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -485,6 +485,9 @@ void toLSPDiags(
case Diag::ClangTidy:
Main.source = "clang-tidy";
break;
+ case Diag::Clangd:
+ Main.source = "clangd";
+ break;
case Diag::ClangdConfig:
Main.source = "clangd-config";
break;
diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
index b9155ae4a7d1..a422c785db34 100644
--- a/clang-tools-extra/clangd/Diagnostics.h
+++ b/clang-tools-extra/clangd/Diagnostics.h
@@ -101,6 +101,7 @@ struct Diag : DiagBase {
Unknown,
Clang,
ClangTidy,
+ Clangd,
ClangdConfig,
} Source = Unknown;
/// Elaborate on the problem, usually pointing to a related piece of code.
diff --git a/clang-tools-extra/clangd/IncludeCleaner.cpp b/clang-tools-extra/clangd/IncludeCleaner.cpp
index 4178b2785725..50f5a926111e 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.cpp
+++ b/clang-tools-extra/clangd/IncludeCleaner.cpp
@@ -7,9 +7,14 @@
//===----------------------------------------------------------------------===//
#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/FormatVariadic.h"
+#include "llvm/Support/Path.h"
namespace clang {
namespace clangd {
@@ -149,6 +154,20 @@ struct ReferencedFiles {
}
};
+// Returns the range starting at '#' and ending at EOL. Escaped newlines are not
+// handled.
+clangd::Range getDiagnosticRange(llvm::StringRef Code, unsigned HashOffset) {
+ clangd::Range Result;
+ Result.end = Result.start = offsetToPosition(Code, HashOffset);
+
+ // Span the warning until the EOL or EOF.
+ Result.end.character +=
+ lspLength(Code.drop_front(HashOffset).take_until([](char C) {
+ return C == '\n' || C == '\r';
+ }));
+ return Result;
+}
+
} // namespace
ReferencedLocations findReferencedLocations(ParsedAST &AST) {
@@ -222,5 +241,47 @@ std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST) {
return getUnused(AST.getIncludeStructure(), ReferencedFiles);
}
+std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST,
+ llvm::StringRef Code) {
+ const Config &Cfg = Config::current();
+ if (Cfg.Diagnostics.UnusedIncludes != Config::UnusedIncludesPolicy::Strict ||
+ Cfg.Diagnostics.SuppressAll ||
+ Cfg.Diagnostics.Suppress.contains("unused-includes"))
+ 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 =
+ llvm::formatv("included header {0} is not used",
+ llvm::sys::path::filename(
+ Inc->Written.substr(1, Inc->Written.size() - 2),
+ llvm::sys::path::Style::posix));
+ D.Name = "unused-includes";
+ D.Source = Diag::DiagSource::Clangd;
+ D.File = FileName;
+ D.Severity = DiagnosticsEngine::Warning;
+ D.Tags.push_back(Unnecessary);
+ D.Range = getDiagnosticRange(Code, Inc->HashOffset);
+ // FIXME(kirillbobyrev): Removing inclusion might break the code if the
+ // used headers are only reachable transitively through this one. Suggest
+ // including them directly instead.
+ // FIXME(kirillbobyrev): Add fix suggestion for adding IWYU pragmas
+ // (keep/export) remove the warning once we support IWYU pragmas.
+ D.Fixes.emplace_back();
+ D.Fixes.back().Message = "remove #include directive";
+ 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
diff --git a/clang-tools-extra/clangd/IncludeCleaner.h b/clang-tools-extra/clangd/IncludeCleaner.h
index b232c09cae58..19d471e2e761 100644
--- a/clang-tools-extra/clangd/IncludeCleaner.h
+++ b/clang-tools-extra/clangd/IncludeCleaner.h
@@ -63,6 +63,9 @@ getUnused(const IncludeStructure &Includes,
std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
+std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST,
+ llvm::StringRef Code);
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 5a3d909eb929..a62169c8c241 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/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"
@@ -515,10 +516,18 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
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 =
+ issueUnusedIncludesDiagnostics(Result, 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;
diff --git a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
index 564629aa7d3a..87d8b9d976f0 100644
--- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
@@ -244,6 +244,25 @@ TEST_F(ConfigCompileTests, PathSpecMatch) {
}
}
+TEST_F(ConfigCompileTests, DiagnosticsIncludeCleaner) {
+ // Defaults to None.
+ EXPECT_TRUE(compileAndApply());
+ EXPECT_EQ(Conf.Diagnostics.UnusedIncludes,
+ Config::UnusedIncludesPolicy::None);
+
+ Frag = {};
+ Frag.Diagnostics.UnusedIncludes.emplace("None");
+ EXPECT_TRUE(compileAndApply());
+ EXPECT_EQ(Conf.Diagnostics.UnusedIncludes,
+ Config::UnusedIncludesPolicy::None);
+
+ Frag = {};
+ Frag.Diagnostics.UnusedIncludes.emplace("Strict");
+ EXPECT_TRUE(compileAndApply());
+ EXPECT_EQ(Conf.Diagnostics.UnusedIncludes,
+ Config::UnusedIncludesPolicy::Strict);
+}
+
TEST_F(ConfigCompileTests, DiagnosticSuppression) {
Frag.Diagnostics.Suppress.emplace_back("bugprone-use-after-move");
Frag.Diagnostics.Suppress.emplace_back("unreachable-code");
diff --git a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
index d8fbf1a949a9..bc88cb126066 100644
--- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
@@ -67,6 +67,7 @@ CompileFlags: { Add: [foo, bar] }
CheckOptions:
IgnoreMacros: true
example-check.ExampleOption: 0
+ UnusedIncludes: Strict
)yaml";
auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback());
EXPECT_THAT(Diags.Diagnostics, IsEmpty());
@@ -83,6 +84,8 @@ CompileFlags: { Add: [foo, bar] }
EXPECT_THAT(Results[3].Diagnostics.ClangTidy.CheckOptions,
ElementsAre(PairVal("IgnoreMacros", "true"),
PairVal("example-check.ExampleOption", "0")));
+ EXPECT_TRUE(Results[3].Diagnostics.UnusedIncludes);
+ EXPECT_EQ("Strict", *Results[3].Diagnostics.UnusedIncludes.getValue());
}
TEST(ParseYAML, Locations) {
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 1b778e4d0388..c3c56bdbbe03 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1479,6 +1479,44 @@ TEST(Diagnostics, Tags) {
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.UnusedIncludes = Config::UnusedIncludesPolicy::Strict;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+ EXPECT_THAT(
+ *TU.build().getDiagnostics(),
+ UnorderedElementsAre(AllOf(
+ Diag(Test.range("diag"), "included header unused.h is not used"),
+ WithTag(DiagnosticTag::Unnecessary), DiagSource(Diag::Clangd),
+ WithFix(Fix(Test.range("fix"), "", "remove #include directive")))));
+ Cfg.Diagnostics.SuppressAll = true;
+ WithContextValue SuppressAllWithCfg(Config::Key, std::move(Cfg));
+ EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+ Cfg.Diagnostics.SuppressAll = false;
+ Cfg.Diagnostics.Suppress = {"unused-includes"};
+ WithContextValue SuppressFilterWithCfg(Config::Key, std::move(Cfg));
+ EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+}
+
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list