kbobyrev updated this revision to Diff 382242.
kbobyrev marked 4 inline comments as done.
kbobyrev added a comment.
Resolve the review comments.
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
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1479,6 +1479,44 @@
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
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,7 @@
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 @@
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) {
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.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");
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"
@@ -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 =
+ 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;
Index: clang-tools-extra/clangd/IncludeCleaner.h
===================================================================
--- clang-tools-extra/clangd/IncludeCleaner.h
+++ clang-tools-extra/clangd/IncludeCleaner.h
@@ -63,6 +63,9 @@
std::vector<const Inclusion *> computeUnusedIncludes(ParsedAST &AST);
+std::vector<Diag> issueUnusedIncludesDiagnostics(ParsedAST &AST,
+ 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,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 @@
}
};
+// 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 @@
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
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,6 +118,9 @@
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);
}
Index: clang-tools-extra/clangd/ConfigFragment.h
===================================================================
--- clang-tools-extra/clangd/ConfigFragment.h
+++ clang-tools-extra/clangd/ConfigFragment.h
@@ -210,6 +210,20 @@
/// 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
Index: clang-tools-extra/clangd/ConfigCompile.cpp
===================================================================
--- clang-tools-extra/clangd/ConfigCompile.cpp
+++ clang-tools-extra/clangd/ConfigCompile.cpp
@@ -414,6 +414,16 @@
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));
}
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 UnusedIncludesPolicy { Strict, None };
/// Controls warnings and errors when parsing code.
struct {
bool SuppressAll = false;
@@ -97,6 +98,8 @@
std::string Checks;
llvm::StringMap<std::string> CheckOptions;
} ClangTidy;
+
+ UnusedIncludesPolicy UnusedIncludes = None;
} Diagnostics;
/// Style of the codebase.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
