From 665dbe91f2ed97796691f3608db7e28519f43978 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Tue, 9 Jun 2020 15:36:29 +0200 Subject: [PATCH] Revert "[clangd] Parse std::make_unique, and emit template diagnostics at expansion." This reverts commit 658af9435071d5da017c1d65298bdea19ec095e1. Breaks tests on windows: http://45.33.8.238/win/17229/step_9.txt I think this is uncovering a latent bug when a late-parsed preamble is used with an eagerly-parsed file. --- clang-tools-extra/clangd/Diagnostics.cpp | 147 ++++++--------------- clang-tools-extra/clangd/Diagnostics.h | 10 +- clang-tools-extra/clangd/Preamble.cpp | 14 -- .../clangd/unittests/DiagnosticsTests.cpp | 51 +------ clang/include/clang/Frontend/PrecompiledPreamble.h | 5 - clang/lib/Frontend/PrecompiledPreamble.cpp | 4 - 6 files changed, 48 insertions(+), 183 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index da554ff..f1fbaf4 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -24,7 +24,6 @@ #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/Optional.h" #include "llvm/ADT/STLExtras.h" -#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringRef.h" #include "llvm/ADT/Twine.h" @@ -121,96 +120,49 @@ Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) { return halfOpenToRange(M, R); } -// Try to find a location in the main-file to report the diagnostic D. -// Returns a description like "in included file", or nullptr on failure. -const char *getMainFileRange(const Diag &D, const SourceManager &SM, - SourceLocation DiagLoc, Range &R) { - // Look for a note in the main file indicating template instantiation. - for (const auto &N : D.Notes) { - if (N.InsideMainFile) { - switch (N.ID) { - case diag::note_template_class_instantiation_was_here: - case diag::note_template_class_explicit_specialization_was_here: - case diag::note_template_class_instantiation_here: - case diag::note_template_member_class_here: - case diag::note_template_member_function_here: - case diag::note_function_template_spec_here: - case diag::note_template_static_data_member_def_here: - case diag::note_template_variable_def_here: - case diag::note_template_enum_def_here: - case diag::note_template_nsdmi_here: - case diag::note_template_type_alias_instantiation_here: - case diag::note_template_exception_spec_instantiation_here: - case diag::note_template_requirement_instantiation_here: - case diag::note_evaluating_exception_spec_here: - case diag::note_default_arg_instantiation_here: - case diag::note_default_function_arg_instantiation_here: - case diag::note_explicit_template_arg_substitution_here: - case diag::note_function_template_deduction_instantiation_here: - case diag::note_deduced_template_arg_substitution_here: - case diag::note_prior_template_arg_substitution: - case diag::note_template_default_arg_checking: - case diag::note_concept_specialization_here: - case diag::note_nested_requirement_here: - case diag::note_checking_constraints_for_template_id_here: - case diag::note_checking_constraints_for_var_spec_id_here: - case diag::note_checking_constraints_for_class_spec_id_here: - case diag::note_checking_constraints_for_function_here: - case diag::note_constraint_substitution_here: - case diag::note_constraint_normalization_here: - case diag::note_parameter_mapping_substitution_here: - R = N.Range; - return "in template"; - default: - break; - } - } - } - // Look for where the file with the error was #included. +// Returns whether the \p D is modified. +bool adjustDiagFromHeader(Diag &D, const clang::Diagnostic &Info, + const LangOptions &LangOpts) { + // We only report diagnostics with at least error severity from headers. + // Use default severity to avoid noise with -Werror. + if (!Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError( + Info.getID())) + return false; + + const SourceManager &SM = Info.getSourceManager(); + const SourceLocation &DiagLoc = SM.getExpansionLoc(Info.getLocation()); + SourceLocation IncludeInMainFile; auto GetIncludeLoc = [&SM](SourceLocation SLoc) { return SM.getIncludeLoc(SM.getFileID(SLoc)); }; - for (auto IncludeLocation = GetIncludeLoc(SM.getExpansionLoc(DiagLoc)); - IncludeLocation.isValid(); + for (auto IncludeLocation = GetIncludeLoc(DiagLoc); IncludeLocation.isValid(); IncludeLocation = GetIncludeLoc(IncludeLocation)) { if (clangd::isInsideMainFile(IncludeLocation, SM)) { - R.start = sourceLocToPosition(SM, IncludeLocation); - R.end = sourceLocToPosition( - SM, - Lexer::getLocForEndOfToken(IncludeLocation, 0, SM, LangOptions())); - return "in included file"; + IncludeInMainFile = IncludeLocation; + break; } } - return nullptr; -} - -// Place the diagnostic the main file, rather than the header, if possible: -// - for errors in included files, use the #include location -// - for errors in template instantiation, use the instantation location -// In both cases, add the original header location as a note. -bool tryMoveToMainFile(Diag &D, FullSourceLoc DiagLoc) { - const SourceManager &SM = DiagLoc.getManager(); - DiagLoc = DiagLoc.getExpansionLoc(); - Range R; - const char *Prefix = getMainFileRange(D, SM, DiagLoc, R); - if (!Prefix) + if (IncludeInMainFile.isInvalid()) return false; + // 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)); + D.InsideMainFile = true; + // Add a note that will point to real diagnostic. const auto *FE = SM.getFileEntryForID(SM.getFileID(DiagLoc)); - D.Notes.emplace(D.Notes.begin()); - Note &N = D.Notes.front(); + D.Notes.emplace_back(); + Note &N = D.Notes.back(); N.AbsFile = std::string(FE->tryGetRealPathName()); N.File = std::string(FE->getName()); N.Message = "error occurred here"; - N.Range = D.Range; + N.Range = diagnosticRange(Info, LangOpts); - // Update diag to point at include inside main file. - D.File = SM.getFileEntryForID(SM.getMainFileID())->getName().str(); - D.Range = std::move(R); - D.InsideMainFile = true; // Update message to mention original file. - D.Message = llvm::formatv("{0}: {1}", Prefix, D.Message); + D.Message = llvm::Twine("in included file: ", D.Message).str(); return true; } @@ -523,7 +475,6 @@ void StoreDiags::BeginSourceFile(const LangOptions &Opts, } void StoreDiags::EndSourceFile() { - flushLastDiag(); LangOpts = None; } @@ -561,14 +512,12 @@ static void fillNonLocationData(DiagnosticsEngine::Level DiagLevel, void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, const clang::Diagnostic &Info) { DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info); - bool OriginallyError = - Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError( - Info.getID()); if (Info.getLocation().isInvalid()) { // Handle diagnostics coming from command-line arguments. The source manager // is *not* available at this point, so we cannot use it. - if (!OriginallyError) { + if (!Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError( + Info.getID())) { IgnoreDiagnostics::log(DiagLevel, Info); return; // non-errors add too much noise, do not show them. } @@ -576,8 +525,6 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, flushLastDiag(); LastDiag = Diag(); - LastDiagLoc.reset(); - LastDiagOriginallyError = OriginallyError; LastDiag->ID = Info.getID(); fillNonLocationData(DiagLevel, Info, *LastDiag); LastDiag->InsideMainFile = true; @@ -603,7 +550,6 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, D.File = std::string(SM.getFilename(Info.getLocation())); D.AbsFile = getCanonicalPath( SM.getFileEntryForID(SM.getFileID(Info.getLocation())), SM); - D.ID = Info.getID(); return D; }; @@ -688,9 +634,10 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, LastPrimaryDiagnosticWasSuppressed = false; LastDiag = Diag(); + LastDiag->ID = Info.getID(); FillDiagBase(*LastDiag); - LastDiagLoc.emplace(Info.getLocation(), Info.getSourceManager()); - LastDiagOriginallyError = OriginallyError; + if (!InsideMainFile) + LastDiagWasAdjusted = adjustDiagFromHeader(*LastDiag, Info, *LangOpts); if (!Info.getFixItHints().empty()) AddFix(true /* try to invent a message instead of repeating the diag */); @@ -732,28 +679,16 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel, void StoreDiags::flushLastDiag() { if (!LastDiag) return; - auto Finish = llvm::make_scope_exit([&, NDiags(Output.size())] { - if (Output.size() == NDiags) // No new diag emitted. - vlog("Dropped diagnostic: {0}: {1}", LastDiag->File, LastDiag->Message); - LastDiag.reset(); - }); - - if (isBlacklisted(*LastDiag)) - return; - // Move errors that occur from headers into main file. - if (!LastDiag->InsideMainFile && LastDiagLoc && LastDiagOriginallyError) { - if (tryMoveToMainFile(*LastDiag, *LastDiagLoc)) { - // Suppress multiple errors from the same inclusion. - if (!IncludedErrorLocations - .insert({LastDiag->Range.start.line, - LastDiag->Range.start.character}) - .second) - return; - } + if (!isBlacklisted(*LastDiag) && mentionsMainFile(*LastDiag) && + (!LastDiagWasAdjusted || + // Only report the first diagnostic coming from each particular header. + IncludeLinesWithErrors.insert(LastDiag->Range.start.line).second)) { + Output.push_back(std::move(*LastDiag)); + } else { + vlog("Dropped diagnostic: {0}: {1}", LastDiag->File, LastDiag->Message); } - if (!mentionsMainFile(*LastDiag)) - return; - Output.push_back(std::move(*LastDiag)); + LastDiag.reset(); + LastDiagWasAdjusted = false; } } // namespace clangd diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h index 58f6928..ebf86ba 100644 --- a/clang-tools-extra/clangd/Diagnostics.h +++ b/clang-tools-extra/clangd/Diagnostics.h @@ -13,7 +13,6 @@ #include "support/Path.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/LangOptions.h" -#include "clang/Basic/SourceLocation.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/None.h" @@ -65,7 +64,6 @@ struct DiagBase { // Since File is only descriptive, we store a separate flag to distinguish // diags from the main file. bool InsideMainFile = false; - unsigned ID; // e.g. member of clang::diag, or clang-tidy assigned ID. }; llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const DiagBase &D); @@ -84,6 +82,7 @@ struct Note : DiagBase {}; /// A top-level diagnostic that may have Notes and Fixes. struct Diag : DiagBase { + unsigned ID; // e.g. member of clang::diag, or clang-tidy assigned ID. std::string Name; // if ID was recognized. // The source of this diagnostic. enum { @@ -146,10 +145,9 @@ private: std::vector Output; llvm::Optional LangOpts; llvm::Optional LastDiag; - llvm::Optional LastDiagLoc; // Valid only when LastDiag is set. - bool LastDiagOriginallyError = false; // Valid only when LastDiag is set. - - llvm::DenseSet> IncludedErrorLocations; + /// Set iff adjustDiagFromHeader resulted in changes to LastDiag. + bool LastDiagWasAdjusted = false; + llvm::DenseSet IncludeLinesWithErrors; bool LastPrimaryDiagnosticWasSuppressed = false; }; diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp index 837ae7e..af7229e 100644 --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -13,7 +13,6 @@ #include "support/FSProvider.h" #include "support/Logger.h" #include "support/Trace.h" -#include "clang/AST/DeclTemplate.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/LangOptions.h" #include "clang/Basic/SourceLocation.h" @@ -99,19 +98,6 @@ public: return IWYUHandler.get(); } - bool shouldSkipFunctionBody(Decl *D) override { - // Generally we skip function bodies in preambles for speed. - // We can make exceptions for functions that are cheap to parse and - // instantiate, widely used, and valuable (e.g. commonly produce errors). - if (const auto *FT = llvm::dyn_cast(D)) { - if (const auto *II = FT->getDeclName().getAsIdentifierInfo()) - // std::make_unique is trivial, and we diagnose bad constructor calls. - if (II->isStr("make_unique") && FT->isInStdNamespace()) - return false; - } - return true; - } - private: PathRef File; PreambleParsedCallback ParsedCallback; diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index dbeff4d..e675d01 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -45,9 +45,9 @@ using ::testing::UnorderedElementsAre; return Field(&Diag::Fixes, UnorderedElementsAre(FixMatcher1, FixMatcher2)); } -template -::testing::Matcher WithNote(NoteMatcherTypes... NoteMatcher) { - return Field(&Diag::Notes, ElementsAre(NoteMatcher...)); +::testing::Matcher +WithNote(::testing::Matcher NoteMatcher) { + return Field(&Diag::Notes, ElementsAre(NoteMatcher)); } MATCHER_P2(Diag, Range, Message, @@ -272,51 +272,6 @@ TEST(DiagnosticsTest, ClangTidy) { "use a trailing return type for this function"))))); } -TEST(DiagnosticTest, TemplatesInHeaders) { - // Diagnostics from templates defined in headers are placed at the expansion. - Annotations Main(R"cpp( - Derived [[y]]; // error-ok - )cpp"); - Annotations Header(R"cpp( - template - struct Derived : [[T]] {}; - )cpp"); - TestTU TU = TestTU::withCode(Main.code()); - TU.HeaderCode = Header.code().str(); - EXPECT_THAT( - TU.build().getDiagnostics(), - ElementsAre(AllOf( - Diag(Main.range(), "in template: base specifier must name a class"), - WithNote(Diag(Header.range(), "error occurred here"), - Diag(Main.range(), "in instantiation of template class " - "'Derived' requested here"))))); -} - -TEST(DiagnosticTest, MakeUnique) { - // We usually miss diagnostics from header functions as we don't parse them. - // std::make_unique is an exception. - Annotations Main(R"cpp( - struct S { S(char*); }; - auto x = std::[[make_unique]](42); // error-ok - )cpp"); - TestTU TU = TestTU::withCode(Main.code()); - TU.HeaderCode = R"cpp( - namespace std { - // These mocks aren't quite right - we omit unique_ptr for simplicity. - // forward is included to show its body is not needed to get the diagnostic. - template T&& forward(T& t) { return static_cast(t); } - template T* make_unique(A&&... args) { - return new T(std::forward(args)...); - } - } - )cpp"; - EXPECT_THAT(TU.build().getDiagnostics(), - UnorderedElementsAre( - Diag(Main.range(), - "in template: " - "no matching constructor for initialization of 'S'"))); -} - TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) { Annotations Main(R"cpp( template struct Foo { diff --git a/clang/include/clang/Frontend/PrecompiledPreamble.h b/clang/include/clang/Frontend/PrecompiledPreamble.h index 0f7e9d8..94c9d01 100644 --- a/clang/include/clang/Frontend/PrecompiledPreamble.h +++ b/clang/include/clang/Frontend/PrecompiledPreamble.h @@ -34,7 +34,6 @@ class FileSystem; namespace clang { class CompilerInstance; class CompilerInvocation; -class Decl; class DeclGroupRef; class PCHContainerOperations; @@ -294,10 +293,6 @@ public: virtual std::unique_ptr createPPCallbacks(); /// The returned CommentHandler will be added to the preprocessor if not null. virtual CommentHandler *getCommentHandler(); - /// Determines which function bodies are parsed, by default skips everything. - /// Only used if FrontendOpts::SkipFunctionBodies is true. - /// See ASTConsumer::shouldSkipFunctionBody. - virtual bool shouldSkipFunctionBody(Decl *D) { return true; } }; enum class BuildPreambleError { diff --git a/clang/lib/Frontend/PrecompiledPreamble.cpp b/clang/lib/Frontend/PrecompiledPreamble.cpp index 6cdfc59..31c5f18 100644 --- a/clang/lib/Frontend/PrecompiledPreamble.cpp +++ b/clang/lib/Frontend/PrecompiledPreamble.cpp @@ -255,10 +255,6 @@ public: Action.setEmittedPreamblePCH(getWriter()); } - bool shouldSkipFunctionBody(Decl *D) override { - return Action.Callbacks.shouldSkipFunctionBody(D); - } - private: PrecompilePreambleAction &Action; std::unique_ptr Out; -- 2.7.4