[clang-tools-extra] 2676759 - [clangd] Add fixes for clang "include <foo.h>" diagnostics
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 8 07:17:00 PST 2021
Author: Sam McCall
Date: 2021-12-08T16:16:53+01:00
New Revision: 2676759bf22e45def4d37da74f09261b26a98d21
URL: https://github.com/llvm/llvm-project/commit/2676759bf22e45def4d37da74f09261b26a98d21
DIFF: https://github.com/llvm/llvm-project/commit/2676759bf22e45def4d37da74f09261b26a98d21.diff
LOG: [clangd] Add fixes for clang "include <foo.h>" diagnostics
Clang doesn't offer these fixes I guess for a couple of reasons:
- where to insert includes is a formatting concern, and clang shouldn't
depend on clang-format
- the way clang prints diagnostics, we'd show a bunch of basically irrelevant
context of "this is where we'd want to insert the include"
Maybe it's possible to hack around 1, but 2 is still a concern.
Meanwhile, bolting this onto include-fixer gets the job done.
Fixes https://github.com/clangd/clangd/issues/355
Fixes https://github.com/clangd/clangd/issues/937
Differential Revision: https://reviews.llvm.org/D114667
Added:
Modified:
clang-tools-extra/clangd/Diagnostics.cpp
clang-tools-extra/clangd/Diagnostics.h
clang-tools-extra/clangd/IncludeFixer.cpp
clang-tools-extra/clangd/IncludeFixer.h
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index 6dd69192497fa..b90a6a8fa9452 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -823,6 +823,18 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
if (LastDiag->Severity == DiagnosticsEngine::Ignored)
return;
+ // Give include-fixer a chance to replace a note with a fix.
+ if (Fixer) {
+ auto ReplacementFixes = Fixer(LastDiag->Severity, Info);
+ if (!ReplacementFixes.empty()) {
+ assert(Info.getNumFixItHints() == 0 &&
+ "Include-fixer replaced a note with clang fix-its attached!");
+ LastDiag->Fixes.insert(LastDiag->Fixes.end(), ReplacementFixes.begin(),
+ ReplacementFixes.end());
+ return;
+ }
+ }
+
if (!Info.getFixItHints().empty()) {
// A clang note with fix-it is not a separate diagnostic in clangd. We
// attach it as a Fix to the main diagnostic instead.
diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
index a422c785db34b..99de0e1a611e9 100644
--- a/clang-tools-extra/clangd/Diagnostics.h
+++ b/clang-tools-extra/clangd/Diagnostics.h
@@ -144,6 +144,8 @@ class StoreDiags : public DiagnosticConsumer {
void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) override;
+ /// When passed a main diagnostic, returns fixes to add to it.
+ /// When passed a note diagnostic, returns fixes to replace it with.
using DiagFixer = std::function<std::vector<Fix>(DiagnosticsEngine::Level,
const clang::Diagnostic &)>;
using LevelAdjuster = std::function<DiagnosticsEngine::Level(
diff --git a/clang-tools-extra/clangd/IncludeFixer.cpp b/clang-tools-extra/clangd/IncludeFixer.cpp
index babc3426b868f..24d9b4918543b 100644
--- a/clang-tools-extra/clangd/IncludeFixer.cpp
+++ b/clang-tools-extra/clangd/IncludeFixer.cpp
@@ -31,7 +31,6 @@
#include "clang/Sema/Scope.h"
#include "clang/Sema/Sema.h"
#include "clang/Sema/TypoCorrection.h"
-#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/None.h"
#include "llvm/ADT/Optional.h"
@@ -47,6 +46,27 @@
namespace clang {
namespace clangd {
+namespace {
+
+llvm::Optional<llvm::StringRef> getArgStr(const clang::Diagnostic &Info,
+ unsigned Index) {
+ switch (Info.getArgKind(Index)) {
+ case DiagnosticsEngine::ak_c_string:
+ return llvm::StringRef(Info.getArgCStr(Index));
+ case DiagnosticsEngine::ak_std_string:
+ return llvm::StringRef(Info.getArgStdStr(Index));
+ default:
+ return llvm::None;
+ }
+}
+
+std::vector<Fix> only(llvm::Optional<Fix> F) {
+ if (F)
+ return {std::move(*F)};
+ return {};
+}
+
+} // namespace
std::vector<Fix> IncludeFixer::fix(DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) const {
@@ -102,10 +122,53 @@ std::vector<Fix> IncludeFixer::fix(DiagnosticsEngine::Level DiagLevel,
LastUnresolvedName->Loc == Info.getLocation())
return fixUnresolvedName();
}
+ break;
+ // Cases where clang explicitly knows which header to include.
+ // (There's no fix provided for boring formatting reasons).
+ case diag::err_implied_std_initializer_list_not_found:
+ return only(insertHeader("<initializer_list>"));
+ case diag::err_need_header_before_typeid:
+ return only(insertHeader("<typeid>"));
+ case diag::err_need_header_before_ms_uuidof:
+ return only(insertHeader("<guiddef.h>"));
+ case diag::err_need_header_before_placement_new:
+ case diag::err_implicit_coroutine_std_nothrow_type_not_found:
+ return only(insertHeader("<new>"));
+ case diag::err_omp_implied_type_not_found:
+ case diag::err_omp_interop_type_not_found:
+ return only(insertHeader("<omp.h>"));
+ case diag::err_implied_coroutine_type_not_found:
+ return only(insertHeader("<coroutine>"));
+ case diag::err_implied_comparison_category_type_not_found:
+ return only(insertHeader("<compare>"));
+ case diag::note_include_header_or_declare:
+ if (Info.getNumArgs() > 0)
+ if (auto Header = getArgStr(Info, 0))
+ return only(insertHeader(("<" + *Header + ">").str(),
+ getArgStr(Info, 1).getValueOr("")));
+ break;
}
+
return {};
}
+llvm::Optional<Fix> IncludeFixer::insertHeader(llvm::StringRef Spelled,
+ llvm::StringRef Symbol) const {
+ Fix F;
+
+ if (auto Edit = Inserter->insert(Spelled))
+ F.Edits.push_back(std::move(*Edit));
+ else
+ return llvm::None;
+
+ if (Symbol.empty())
+ F.Message = llvm::formatv("Include {0}", Spelled);
+ else
+ F.Message = llvm::formatv("Include {0} for symbol {1}", Spelled, Symbol);
+
+ return F;
+}
+
std::vector<Fix> IncludeFixer::fixIncompleteType(const Type &T) const {
// Only handle incomplete TagDecl type.
const TagDecl *TD = T.getAsTagDecl();
@@ -160,14 +223,11 @@ std::vector<Fix> IncludeFixer::fixesForSymbols(const SymbolSlab &Syms) const {
for (const auto &Inc : getRankedIncludes(Sym)) {
if (auto ToInclude = Inserted(Sym, Inc)) {
if (ToInclude->second) {
- auto I = InsertedHeaders.try_emplace(ToInclude->first);
- if (!I.second)
+ if (!InsertedHeaders.try_emplace(ToInclude->first).second)
continue;
- if (auto Edit = Inserter->insert(ToInclude->first))
- Fixes.push_back(Fix{std::string(llvm::formatv(
- "Add include {0} for symbol {1}{2}",
- ToInclude->first, Sym.Scope, Sym.Name)),
- {std::move(*Edit)}});
+ if (auto Fix =
+ insertHeader(ToInclude->first, (Sym.Scope + Sym.Name).str()))
+ Fixes.push_back(std::move(*Fix));
}
} else {
vlog("Failed to calculate include insertion for {0} into {1}: {2}", Inc,
diff --git a/clang-tools-extra/clangd/IncludeFixer.h b/clang-tools-extra/clangd/IncludeFixer.h
index b9128584eb43d..73b0cd4c03c28 100644
--- a/clang-tools-extra/clangd/IncludeFixer.h
+++ b/clang-tools-extra/clangd/IncludeFixer.h
@@ -40,6 +40,7 @@ class IncludeFixer {
IndexRequestLimit(IndexRequestLimit) {}
/// Returns include insertions that can potentially recover the diagnostic.
+ /// If Info is a note and fixes are returned, they should *replace* the note.
std::vector<Fix> fix(DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) const;
@@ -55,6 +56,9 @@ class IncludeFixer {
/// Generates header insertion fixes for all symbols. Fixes are deduplicated.
std::vector<Fix> fixesForSymbols(const SymbolSlab &Syms) const;
+ llvm::Optional<Fix> insertHeader(llvm::StringRef Name,
+ llvm::StringRef Symbol = "") const;
+
struct UnresolvedName {
std::string Name; // E.g. "X" in foo::X.
SourceLocation Loc; // Start location of the unresolved name.
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 89a3cc060f925..2c4a9aec1d3f1 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -867,58 +867,58 @@ class T {
"incomplete type 'ns::X' named in nested name specifier"),
DiagName("incomplete_nested_name_spec"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(Diag(Test.range("base"), "base class has incomplete type"),
DiagName("incomplete_base_class"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(Diag(Test.range("access"),
"member access into incomplete type 'ns::X'"),
DiagName("incomplete_member_access"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(
Diag(
Test.range("type"),
"incomplete type 'ns::X' where a complete type is required"),
DiagName("incomplete_type"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(Diag(Test.range("incomplete"),
"variable has incomplete type 'ns::X'"),
DiagName("typecheck_decl_incomplete_type"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(
Diag(Test.range("tag"), "incomplete definition of type 'ns::X'"),
DiagName("typecheck_incomplete_tag"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(Diag(Test.range("use"),
"invalid use of incomplete type 'ns::X'"),
DiagName("invalid_incomplete_type_use"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(Diag(Test.range("sizeof"),
"invalid application of 'sizeof' to "
"an incomplete type 'ns::X'"),
DiagName("sizeof_alignof_incomplete_or_sizeless_type"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(Diag(Test.range("for"),
"cannot use incomplete type 'ns::X' as a range"),
DiagName("for_range_incomplete_type"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(Diag(Test.range("return"),
"incomplete result type 'ns::X' in function definition"),
DiagName("func_def_incomplete_result"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(Diag(Test.range("field"), "field has incomplete type 'ns::X'"),
DiagName("field_incomplete_or_sizeless"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X")))}))
+ "Include \"x.h\" for symbol ns::X")))}))
<< Test.code();
}
@@ -984,28 +984,28 @@ using Type = ns::$template[[Foo]]<int>;
AllOf(Diag(Test.range("unqualified1"), "unknown type name 'X'"),
DiagName("unknown_typename"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
Diag(Test.range("unqualified2"), "use of undeclared identifier 'X'"),
AllOf(Diag(Test.range("qualified1"),
"no type named 'X' in namespace 'ns'"),
DiagName("typename_nested_not_found"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(Diag(Test.range("qualified2"),
"no member named 'X' in namespace 'ns'"),
DiagName("no_member"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::X"))),
+ "Include \"x.h\" for symbol ns::X"))),
AllOf(Diag(Test.range("global"),
"no type named 'Global' in the global namespace"),
DiagName("typename_nested_not_found"),
WithFix(Fix(Test.range("insert"), "#include \"global.h\"\n",
- "Add include \"global.h\" for symbol Global"))),
+ "Include \"global.h\" for symbol Global"))),
AllOf(Diag(Test.range("template"),
"no template named 'Foo' in namespace 'ns'"),
DiagName("no_member_template"),
WithFix(Fix(Test.range("insert"), "#include \"foo.h\"\n",
- "Add include \"foo.h\" for symbol ns::Foo")))));
+ "Include \"foo.h\" for symbol ns::Foo")))));
}
TEST(IncludeFixerTest, MultipleMatchedSymbols) {
@@ -1029,12 +1029,12 @@ void foo() {
Diag(Test.range("unqualified"), "unknown type name 'X'"),
DiagName("unknown_typename"),
WithFix(Fix(Test.range("insert"), "#include \"a.h\"\n",
- "Add include \"a.h\" for symbol na::X"),
+ "Include \"a.h\" for symbol na::X"),
Fix(Test.range("insert"), "#include \"b.h\"\n",
- "Add include \"b.h\" for symbol na::nb::X")))));
+ "Include \"b.h\" for symbol na::nb::X")))));
}
-TEST(IncludeFixerTest, NoCrashMemebrAccess) {
+TEST(IncludeFixerTest, NoCrashMemberAccess) {
Annotations Test(R"cpp(// error-ok
struct X { int xyz; };
void g() { X x; x.$[[xy]]; }
@@ -1085,8 +1085,7 @@ void bar(X *x) {
ADD_FAILURE() << "D.Fixes.size() != 1";
continue;
}
- EXPECT_EQ(D.Fixes[0].Message,
- std::string("Add include \"a.h\" for symbol X"));
+ EXPECT_EQ(D.Fixes[0].Message, std::string("Include \"a.h\" for symbol X"));
}
}
@@ -1106,11 +1105,11 @@ void g() { ns::$[[scope]]::X_Y(); }
EXPECT_THAT(
*TU.build().getDiagnostics(),
- UnorderedElementsAre(AllOf(
- Diag(Test.range(), "no member named 'scope' in namespace 'ns'"),
- DiagName("no_member"),
- WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol ns::scope::X_Y")))));
+ UnorderedElementsAre(
+ AllOf(Diag(Test.range(), "no member named 'scope' in namespace 'ns'"),
+ DiagName("no_member"),
+ WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+ "Include \"x.h\" for symbol ns::scope::X_Y")))));
}
TEST(IncludeFixerTest, UnresolvedSpecifierWithSemaCorrection) {
@@ -1135,32 +1134,29 @@ void f() {
EXPECT_THAT(
*TU.build().getDiagnostics(),
UnorderedElementsAre(
- AllOf(
- Diag(Test.range("q1"), "use of undeclared identifier 'clangd'; "
- "did you mean 'clang'?"),
- DiagName("undeclared_var_use_suggest"),
- WithFix(_, // change clangd to clang
- Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol clang::clangd::X"))),
- AllOf(
- Diag(Test.range("x"), "no type named 'X' in namespace 'clang'"),
- DiagName("typename_nested_not_found"),
- WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol clang::clangd::X"))),
+ AllOf(Diag(Test.range("q1"), "use of undeclared identifier 'clangd'; "
+ "did you mean 'clang'?"),
+ DiagName("undeclared_var_use_suggest"),
+ WithFix(_, // change clangd to clang
+ Fix(Test.range("insert"), "#include \"x.h\"\n",
+ "Include \"x.h\" for symbol clang::clangd::X"))),
+ AllOf(Diag(Test.range("x"), "no type named 'X' in namespace 'clang'"),
+ DiagName("typename_nested_not_found"),
+ WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+ "Include \"x.h\" for symbol clang::clangd::X"))),
AllOf(
Diag(Test.range("q2"), "use of undeclared identifier 'clangd'; "
"did you mean 'clang'?"),
DiagName("undeclared_var_use_suggest"),
- WithFix(
- _, // change clangd to clang
- Fix(Test.range("insert"), "#include \"y.h\"\n",
- "Add include \"y.h\" for symbol clang::clangd::ns::Y"))),
+ WithFix(_, // change clangd to clang
+ Fix(Test.range("insert"), "#include \"y.h\"\n",
+ "Include \"y.h\" for symbol clang::clangd::ns::Y"))),
AllOf(Diag(Test.range("ns"),
"no member named 'ns' in namespace 'clang'"),
DiagName("no_member"),
- WithFix(Fix(
- Test.range("insert"), "#include \"y.h\"\n",
- "Add include \"y.h\" for symbol clang::clangd::ns::Y")))));
+ WithFix(
+ Fix(Test.range("insert"), "#include \"y.h\"\n",
+ "Include \"y.h\" for symbol clang::clangd::ns::Y")))));
}
TEST(IncludeFixerTest, SpecifiedScopeIsNamespaceAlias) {
@@ -1181,7 +1177,7 @@ namespace c {
Diag(Test.range(), "no type named 'X' in namespace 'a'"),
DiagName("typename_nested_not_found"),
WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
- "Add include \"x.h\" for symbol a::X")))));
+ "Include \"x.h\" for symbol a::X")))));
}
TEST(IncludeFixerTest, NoCrashOnTemplateInstantiations) {
@@ -1206,6 +1202,26 @@ TEST(IncludeFixerTest, NoCrashOnTemplateInstantiations) {
ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'")));
}
+TEST(IncludeFixerTest, HeaderNamedInDiag) {
+ Annotations Test(R"cpp(
+ $insert[[]]int main() {
+ [[printf]]("");
+ }
+ )cpp");
+ auto TU = TestTU::withCode(Test.code());
+ TU.ExtraArgs = {"-xc"};
+ auto Index = buildIndexWithSymbol({});
+ TU.ExternalIndex = Index.get();
+
+ EXPECT_THAT(
+ *TU.build().getDiagnostics(),
+ ElementsAre(AllOf(
+ Diag(Test.range(), "implicitly declaring library function 'printf' "
+ "with type 'int (const char *, ...)'"),
+ WithFix(Fix(Test.range("insert"), "#include <stdio.h>\n",
+ "Include <stdio.h> for symbol printf")))));
+}
+
TEST(DiagsInHeaders, DiagInsideHeader) {
Annotations Main(R"cpp(
#include [["a.h"]]
More information about the cfe-commits
mailing list