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"]] _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits