[clang] 665dbe9 - Revert "[clangd] Parse std::make_unique, and emit template diagnostics at expansion."
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Tue Jun 9 06:42:34 PDT 2020
Author: Sam McCall
Date: 2020-06-09T15:42:22+02:00
New Revision: 665dbe91f2ed97796691f3608db7e28519f43978
URL: https://github.com/llvm/llvm-project/commit/665dbe91f2ed97796691f3608db7e28519f43978
DIFF: https://github.com/llvm/llvm-project/commit/665dbe91f2ed97796691f3608db7e28519f43978.diff
LOG: 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.
Added:
Modified:
clang-tools-extra/clangd/Diagnostics.cpp
clang-tools-extra/clangd/Diagnostics.h
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
clang/include/clang/Frontend/PrecompiledPreamble.h
clang/lib/Frontend/PrecompiledPreamble.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index da554ff8c331..f1fbaf4646ba 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 58f69287f256..ebf86ba8716a 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 @@ class StoreDiags : public DiagnosticConsumer {
std::vector<Diag> Output;
llvm::Optional<LangOptions> LangOpts;
llvm::Optional<Diag> LastDiag;
- llvm::Optional<FullSourceLoc> LastDiagLoc; // Valid only when LastDiag is set.
- bool LastDiagOriginallyError = false; // Valid only when LastDiag is set.
-
- llvm::DenseSet<std::pair<unsigned, unsigned>> IncludedErrorLocations;
+ /// Set iff adjustDiagFromHeader resulted in changes to LastDiag.
+ bool LastDiagWasAdjusted = false;
+ llvm::DenseSet<int> IncludeLinesWithErrors;
bool LastPrimaryDiagnosticWasSuppressed = false;
};
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index 837ae7e5a5bb..af7229ecb888 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 @@ class CppFilePreambleCallbacks : public PreambleCallbacks {
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<clang::FunctionTemplateDecl>(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 dbeff4deb444..e675d01ad59f 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -45,9 +45,9 @@ ::testing::Matcher<const Diag &> WithFix(::testing::Matcher<Fix> FixMatcher1,
return Field(&Diag::Fixes, UnorderedElementsAre(FixMatcher1, FixMatcher2));
}
-template <typename... NoteMatcherTypes>
-::testing::Matcher<const Diag &> WithNote(NoteMatcherTypes... NoteMatcher) {
- return Field(&Diag::Notes, ElementsAre(NoteMatcher...));
+::testing::Matcher<const Diag &>
+WithNote(::testing::Matcher<Note> 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<int> [[y]]; // error-ok
- )cpp");
- Annotations Header(R"cpp(
- template <typename T>
- 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<int>' 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]]<S>(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 <typename T> T&& forward(T& t) { return static_cast<T&&>(t); }
- template <typename T, typename... A> T* make_unique(A&&... args) {
- return new T(std::forward<A>(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 <typename T> struct Foo {
diff --git a/clang/include/clang/Frontend/PrecompiledPreamble.h b/clang/include/clang/Frontend/PrecompiledPreamble.h
index 0f7e9d895a00..94c9d0135013 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 @@ class PreambleCallbacks {
virtual std::unique_ptr<PPCallbacks> 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 6cdfc595dcae..31c5f18eeeec 100644
--- a/clang/lib/Frontend/PrecompiledPreamble.cpp
+++ b/clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -255,10 +255,6 @@ class PrecompilePreambleConsumer : public PCHGenerator {
Action.setEmittedPreamblePCH(getWriter());
}
- bool shouldSkipFunctionBody(Decl *D) override {
- return Action.Callbacks.shouldSkipFunctionBody(D);
- }
-
private:
PrecompilePreambleAction &Action;
std::unique_ptr<raw_ostream> Out;
More information about the cfe-commits
mailing list