From 01efe64c2d60e44bb035501205fa595f80028ca7 Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Mon, 29 Apr 2019 10:25:44 +0000 Subject: [PATCH] [clangd] Surface diagnostics from headers inside main file Reviewers: ioeric, ilya-biryukov Subscribers: MaskRay, jkorous, arphaman, jdoerfert, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D59302 llvm-svn: 359432 --- clang-tools-extra/clangd/Diagnostics.cpp | 55 +++++++- clang-tools-extra/clangd/Diagnostics.h | 4 + .../clangd/unittests/DiagnosticsTests.cpp | 138 +++++++++++++++++++-- clang-tools-extra/clangd/unittests/TestTU.cpp | 12 +- clang-tools-extra/clangd/unittests/TestTU.h | 8 ++ 5 files changed, 198 insertions(+), 19 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index a734f23..c004fa3 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -13,11 +13,18 @@ #include "Protocol.h" #include "SourceCode.h" #include "clang/Basic/AllDiagnostics.h" +#include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticIDs.h" +#include "clang/Basic/FileManager.h" #include "clang/Basic/SourceManager.h" #include "clang/Lex/Lexer.h" +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/ADT/Twine.h" #include "llvm/Support/Capacity.h" #include "llvm/Support/Path.h" +#include "llvm/Support/ScopedPrinter.h" +#include "llvm/Support/Signals.h" #include namespace clang { @@ -102,6 +109,39 @@ Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) { return halfOpenToRange(M, R); } +void adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info, + const LangOptions &LangOpts) { + const SourceLocation &DiagLoc = Info.getLocation(); + const SourceManager &SM = Info.getSourceManager(); + SourceLocation IncludeInMainFile; + auto GetIncludeLoc = [&SM](SourceLocation SLoc) { + return SM.getIncludeLoc(SM.getFileID(SLoc)); + }; + for (auto IncludeLocation = GetIncludeLoc(DiagLoc); IncludeLocation.isValid(); + IncludeLocation = GetIncludeLoc(IncludeLocation)) + IncludeInMainFile = IncludeLocation; + if (IncludeInMainFile.isInvalid()) + return; + + // Update diag to point at include inside main file. + D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str(); + D.Range.start = sourceLocToPosition(SM, IncludeInMainFile); + D.Range.end = sourceLocToPosition( + SM, Lexer::getLocForEndOfToken(IncludeInMainFile, 0, SM, LangOpts)); + + // Add a note that will point to real diagnostic. + const auto *FE = SM.getFileEntryForID(SM.getFileID(DiagLoc)); + D.Notes.emplace_back(); + Note &N = D.Notes.back(); + N.AbsFile = FE->tryGetRealPathName(); + N.File = FE->getName(); + N.Message = "error occurred here"; + N.Range = diagnosticRange(Info, LangOpts); + + // Update message to mention original file. + D.Message = llvm::Twine("in included file: ", D.Message).str(); +} + bool isInsideMainFile(const SourceLocation Loc, const SourceManager &M) { return Loc.isValid() && M.isWrittenInMainFile(M.getFileLoc(Loc)); } @@ -378,7 +418,7 @@ std::vector StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) { Msg.resize(Rest.size()); }; CleanMessage(Diag.Message); - for (auto& Note : Diag.Notes) + for (auto &Note : Diag.Notes) CleanMessage(Note.Message); continue; } @@ -477,6 +517,7 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, LastDiag = Diag(); LastDiag->ID = Info.getID(); FillDiagBase(*LastDiag); + adjustDiagFromHeader(*LastDiag, Info, *LangOpts); if (!Info.getFixItHints().empty()) AddFix(true /* try to invent a message instead of repeating the diag */); @@ -511,11 +552,15 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, void StoreDiags::flushLastDiag() { if (!LastDiag) return; - if (mentionsMainFile(*LastDiag)) + // Only keeps diagnostics inside main file or the first one coming from a + // header. + if (mentionsMainFile(*LastDiag) || + (LastDiag->Severity >= DiagnosticsEngine::Level::Error && + IncludeLinesWithErrors.insert(LastDiag->Range.start.line).second)) { Output.push_back(std::move(*LastDiag)); - else - vlog("Dropped diagnostic outside main file: {0}: {1}", LastDiag->File, - LastDiag->Message); + } else { + vlog("Dropped diagnostic: {0}: {1}", LastDiag->File, LastDiag->Message); + } LastDiag.reset(); } diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h index c804580..a0ab7c6 100644 --- a/clang-tools-extra/clangd/Diagnostics.h +++ b/clang-tools-extra/clangd/Diagnostics.h @@ -14,6 +14,9 @@ #include "clang/Basic/Diagnostic.h" #include "clang/Basic/LangOptions.h" #include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/None.h" +#include "llvm/ADT/Optional.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringSet.h" #include @@ -135,6 +138,7 @@ private: std::vector Output; llvm::Optional LangOpts; llvm::Optional LastDiag; + llvm::DenseSet IncludeLinesWithErrors; }; } // namespace clangd diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index a742c9d..ed12897 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -9,10 +9,11 @@ #include "Annotations.h" #include "ClangdUnit.h" #include "Diagnostics.h" +#include "Path.h" #include "Protocol.h" #include "SourceCode.h" -#include "TestIndex.h" #include "TestFS.h" +#include "TestIndex.h" #include "TestTU.h" #include "index/MemIndex.h" #include "clang/Basic/Diagnostic.h" @@ -20,6 +21,7 @@ #include "llvm/Support/ScopedPrinter.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include namespace clang { namespace clangd { @@ -61,7 +63,8 @@ MATCHER_P(EqualToLSPDiag, LSPDiag, "LSP diagnostic " + llvm::to_string(LSPDiag)) { if (toJSON(arg) != toJSON(LSPDiag)) { *result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}", - toJSON(LSPDiag), toJSON(arg)).str(); + toJSON(LSPDiag), toJSON(arg)) + .str(); return false; } return true; @@ -83,7 +86,6 @@ MATCHER_P(EqualToFix, Fix, "LSP fix " + llvm::to_string(Fix)) { return true; } - // Helper function to make tests shorter. Position pos(int line, int character) { Position Res; @@ -114,8 +116,7 @@ o]](); // This range spans lines. AllOf(Diag(Test.range("typo"), "use of undeclared identifier 'goo'; did you mean 'foo'?"), - DiagSource(Diag::Clang), - DiagName("undeclared_var_use_suggest"), + DiagSource(Diag::Clang), DiagName("undeclared_var_use_suggest"), WithFix( Fix(Test.range("typo"), "foo", "change 'go\\ o' to 'foo'")), // This is a pretty normal range. @@ -301,7 +302,8 @@ TEST(DiagnosticsTest, ToLSP) { MainLSP.severity = getSeverity(DiagnosticsEngine::Error); MainLSP.code = "enum_class_reference"; MainLSP.source = "clang"; - MainLSP.message = R"(Something terrible happened (fix available) + MainLSP.message = + R"(Something terrible happened (fix available) main.cpp:6:7: remark: declared somewhere in the main file @@ -354,9 +356,8 @@ main.cpp:2:3: error: something terrible happened)"; NoteInHeaderDRI.location.range = NoteInHeader.Range; NoteInHeaderDRI.location.uri = HeaderFile; MainLSP.relatedInformation = {NoteInMainDRI, NoteInHeaderDRI}; - EXPECT_THAT( - LSPDiags, - ElementsAre(Pair(EqualToLSPDiag(MainLSP), ElementsAre(EqualToFix(F))))); + EXPECT_THAT(LSPDiags, ElementsAre(Pair(EqualToLSPDiag(MainLSP), + ElementsAre(EqualToFix(F))))); } struct SymbolWithHeader { @@ -646,7 +647,124 @@ namespace c { "Add include \"x.h\" for symbol a::X"))))); } +TEST(DiagsInHeaders, DiagInsideHeader) { + Annotations Main(R"cpp( + #include [["a.h"]] + void foo() {})cpp"); + Annotations Header("[[no_type_spec]];"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = {{"a.h", Header.code()}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre(AllOf( + Diag(Main.range(), "in included file: C++ requires a " + "type specifier for all declarations"), + WithNote(Diag(Header.range(), "error occurred here"))))); +} + +TEST(DiagsInHeaders, DiagInTransitiveInclude) { + Annotations Main(R"cpp( + #include [["a.h"]] + void foo() {})cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = {{"a.h", "#include \"b.h\""}, {"b.h", "no_type_spec;"}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range(), "in included file: C++ requires a " + "type specifier for all declarations"))); +} + +TEST(DiagsInHeaders, DiagInMultipleHeaders) { + Annotations Main(R"cpp( + #include $a[["a.h"]] + #include $b[["b.h"]] + void foo() {})cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = {{"a.h", "no_type_spec;"}, {"b.h", "no_type_spec;"}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range("a"), "in included file: C++ requires a type " + "specifier for all declarations"), + Diag(Main.range("b"), "in included file: C++ requires a type " + "specifier for all declarations"))); +} + +TEST(DiagsInHeaders, PreferExpansionLocation) { + Annotations Main(R"cpp( + #include [["a.h"]] + #include "b.h" + void foo() {})cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = {{"a.h", "#include \"b.h\"\n"}, + {"b.h", "#ifndef X\n#define X\nno_type_spec;\n#endif"}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre(Diag(Main.range(), + "in included file: C++ requires a type " + "specifier for all declarations"))); +} + +TEST(DiagsInHeaders, PreferExpansionLocationMacros) { + Annotations Main(R"cpp( + #define X + #include "a.h" + #undef X + #include [["b.h"]] + void foo() {})cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = {{"a.h", "#include \"c.h\"\n"}, + {"b.h", "#include \"c.h\"\n"}, + {"c.h", "#ifndef X\n#define X\nno_type_spec;\n#endif"}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range(), "in included file: C++ requires a " + "type specifier for all declarations"))); +} + +TEST(DiagsInHeaders, LimitDiagsOutsideMainFile) { + Annotations Main(R"cpp( + #include [["a.h"]] + #include "b.h" + void foo() {})cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = {{"a.h", "#include \"c.h\"\n"}, + {"b.h", "#include \"c.h\"\n"}, + {"c.h", R"cpp( + #ifndef X + #define X + no_type_spec_0; + no_type_spec_1; + no_type_spec_2; + no_type_spec_3; + no_type_spec_4; + no_type_spec_5; + no_type_spec_6; + no_type_spec_7; + no_type_spec_8; + no_type_spec_9; + no_type_spec_10; + #endif)cpp"}}; + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre( + Diag(Main.range(), "in included file: C++ requires a " + "type specifier for all declarations"))); +} + +TEST(DiagsInHeaders, OnlyErrorOrFatal) { + Annotations Main(R"cpp( + #include [["a.h"]] + void foo() {})cpp"); + Annotations Header(R"cpp( + [[no_type_spec]]; + int x = 5/0;)cpp"); + TestTU TU = TestTU::withCode(Main.code()); + TU.AdditionalFiles = {{"a.h", Header.code()}}; + auto diags = TU.build().getDiagnostics(); + EXPECT_THAT(TU.build().getDiagnostics(), + UnorderedElementsAre(AllOf( + Diag(Main.range(), "in included file: C++ requires " + "a type specifier for all declarations"), + WithNote(Diag(Header.range(), "error occurred here"))))); +} } // namespace + } // namespace clangd } // namespace clang - diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp index 05c7fbf..8f48eab 100644 --- a/clang-tools-extra/clangd/unittests/TestTU.cpp +++ b/clang-tools-extra/clangd/unittests/TestTU.cpp @@ -21,11 +21,17 @@ ParsedAST TestTU::build() const { std::string FullFilename = testPath(Filename), FullHeaderName = testPath(HeaderFilename), ImportThunk = testPath("import_thunk.h"); - std::vector Cmd = {"clang", FullFilename.c_str()}; // We want to implicitly include HeaderFilename without messing up offsets. // -include achieves this, but sometimes we want #import (to simulate a header // guard without messing up offsets). In this case, use an intermediate file. std::string ThunkContents = "#import \"" + FullHeaderName + "\"\n"; + + llvm::StringMap Files(AdditionalFiles); + Files[FullFilename] = Code; + Files[FullHeaderName] = HeaderCode; + Files[ImportThunk] = ThunkContents; + + std::vector Cmd = {"clang", FullFilename.c_str()}; // FIXME: this shouldn't need to be conditional, but it breaks a // GoToDefinition test for some reason (getMacroArgExpandedLocation fails). if (!HeaderCode.empty()) { @@ -39,9 +45,7 @@ ParsedAST TestTU::build() const { Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()}; Inputs.CompileCommand.Directory = testRoot(); Inputs.Contents = Code; - Inputs.FS = buildTestFS({{FullFilename, Code}, - {FullHeaderName, HeaderCode}, - {ImportThunk, ThunkContents}}); + Inputs.FS = buildTestFS(Files); Inputs.Opts = ParseOptions(); Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks; Inputs.Index = ExternalIndex; diff --git a/clang-tools-extra/clangd/unittests/TestTU.h b/clang-tools-extra/clangd/unittests/TestTU.h index 0f59516..6ac4c86 100644 --- a/clang-tools-extra/clangd/unittests/TestTU.h +++ b/clang-tools-extra/clangd/unittests/TestTU.h @@ -18,8 +18,13 @@ #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H #include "ClangdUnit.h" +#include "Path.h" #include "index/Index.h" +#include "llvm/ADT/StringMap.h" #include "gtest/gtest.h" +#include +#include +#include namespace clang { namespace clangd { @@ -45,6 +50,9 @@ struct TestTU { std::string HeaderCode; std::string HeaderFilename = "TestTU.h"; + // Name and contents of each file. + llvm::StringMap AdditionalFiles; + // Extra arguments for the compiler invocation. std::vector ExtraArgs; -- 2.7.4