[clang] 658af94 - [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 03:47:35 PDT 2020
Author: Sam McCall
Date: 2020-06-09T12:47:27+02:00
New Revision: 658af9435071d5da017c1d65298bdea19ec095e1
URL: https://github.com/llvm/llvm-project/commit/658af9435071d5da017c1d65298bdea19ec095e1
DIFF: https://github.com/llvm/llvm-project/commit/658af9435071d5da017c1d65298bdea19ec095e1.diff
LOG: [clangd] Parse std::make_unique, and emit template diagnostics at expansion.
Summary:
Parsing std::make_unique is an exception to the usual non-parsing of function
bodies in the preamble. (A hook is added to PreambleCallbacks to allow this).
This allows us to diagnose make_unique<Foo>(wrong arg list), and opens the door
to providing signature help (by detecting where the arg list is forwarded to).
This function is trivial (checked libc++ and libstdc++) and doesn't result in
any extra templates being instantiated, so this should be cheap.
This uncovered a second issue (already visible with class templates)...
Errors produced by template instantiation have primary locations within the
template, with instantiation stack reported as notes.
For templates defined in headers, these end up reported at the #include
directive, which isn't terribly helpful as the header itself is probably fine.
This patch reports them at the instantiation site (the first location in the
instantiation stack that's in the main file). This in turn required a bit of
refactoring in Diagnostics so we can delay relocating the diagnostic until all
notes are available.
https://github.com/clangd/clangd/issues/412
Reviewers: hokein, aaron.ballman
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D81351
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 f1fbaf4646ba..da554ff8c331 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -24,6 +24,7 @@
#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"
@@ -120,49 +121,96 @@ Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
return halfOpenToRange(M, R);
}
-// 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;
+// 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.
auto GetIncludeLoc = [&SM](SourceLocation SLoc) {
return SM.getIncludeLoc(SM.getFileID(SLoc));
};
- for (auto IncludeLocation = GetIncludeLoc(DiagLoc); IncludeLocation.isValid();
+ for (auto IncludeLocation = GetIncludeLoc(SM.getExpansionLoc(DiagLoc));
+ IncludeLocation.isValid();
IncludeLocation = GetIncludeLoc(IncludeLocation)) {
if (clangd::isInsideMainFile(IncludeLocation, SM)) {
- IncludeInMainFile = IncludeLocation;
- break;
+ R.start = sourceLocToPosition(SM, IncludeLocation);
+ R.end = sourceLocToPosition(
+ SM,
+ Lexer::getLocForEndOfToken(IncludeLocation, 0, SM, LangOptions()));
+ return "in included file";
}
}
- if (IncludeInMainFile.isInvalid())
- return false;
+ return nullptr;
+}
- // 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;
+// 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)
+ return false;
// 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();
+ D.Notes.emplace(D.Notes.begin());
+ Note &N = D.Notes.front();
N.AbsFile = std::string(FE->tryGetRealPathName());
N.File = std::string(FE->getName());
N.Message = "error occurred here";
- N.Range = diagnosticRange(Info, LangOpts);
+ N.Range = D.Range;
+ // 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::Twine("in included file: ", D.Message).str();
+ D.Message = llvm::formatv("{0}: {1}", Prefix, D.Message);
return true;
}
@@ -475,6 +523,7 @@ void StoreDiags::BeginSourceFile(const LangOptions &Opts,
}
void StoreDiags::EndSourceFile() {
+ flushLastDiag();
LangOpts = None;
}
@@ -512,12 +561,14 @@ 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 (!Info.getDiags()->getDiagnosticIDs()->isDefaultMappingAsError(
- Info.getID())) {
+ if (!OriginallyError) {
IgnoreDiagnostics::log(DiagLevel, Info);
return; // non-errors add too much noise, do not show them.
}
@@ -525,6 +576,8 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
flushLastDiag();
LastDiag = Diag();
+ LastDiagLoc.reset();
+ LastDiagOriginallyError = OriginallyError;
LastDiag->ID = Info.getID();
fillNonLocationData(DiagLevel, Info, *LastDiag);
LastDiag->InsideMainFile = true;
@@ -550,6 +603,7 @@ 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;
};
@@ -634,10 +688,9 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
LastPrimaryDiagnosticWasSuppressed = false;
LastDiag = Diag();
- LastDiag->ID = Info.getID();
FillDiagBase(*LastDiag);
- if (!InsideMainFile)
- LastDiagWasAdjusted = adjustDiagFromHeader(*LastDiag, Info, *LangOpts);
+ LastDiagLoc.emplace(Info.getLocation(), Info.getSourceManager());
+ LastDiagOriginallyError = OriginallyError;
if (!Info.getFixItHints().empty())
AddFix(true /* try to invent a message instead of repeating the diag */);
@@ -679,16 +732,28 @@ void StoreDiags::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
void StoreDiags::flushLastDiag() {
if (!LastDiag)
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);
+ 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;
+ }
}
- LastDiag.reset();
- LastDiagWasAdjusted = false;
+ if (!mentionsMainFile(*LastDiag))
+ return;
+ Output.push_back(std::move(*LastDiag));
}
} // namespace clangd
diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h
index ebf86ba8716a..58f69287f256 100644
--- a/clang-tools-extra/clangd/Diagnostics.h
+++ b/clang-tools-extra/clangd/Diagnostics.h
@@ -13,6 +13,7 @@
#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"
@@ -64,6 +65,7 @@ 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);
@@ -82,7 +84,6 @@ 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 {
@@ -145,9 +146,10 @@ class StoreDiags : public DiagnosticConsumer {
std::vector<Diag> Output;
llvm::Optional<LangOptions> LangOpts;
llvm::Optional<Diag> LastDiag;
- /// Set iff adjustDiagFromHeader resulted in changes to LastDiag.
- bool LastDiagWasAdjusted = false;
- llvm::DenseSet<int> IncludeLinesWithErrors;
+ 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;
bool LastPrimaryDiagnosticWasSuppressed = false;
};
diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp
index af7229ecb888..837ae7e5a5bb 100644
--- a/clang-tools-extra/clangd/Preamble.cpp
+++ b/clang-tools-extra/clangd/Preamble.cpp
@@ -13,6 +13,7 @@
#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"
@@ -98,6 +99,19 @@ 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 e675d01ad59f..dbeff4deb444 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));
}
-::testing::Matcher<const Diag &>
-WithNote(::testing::Matcher<Note> NoteMatcher) {
- return Field(&Diag::Notes, ElementsAre(NoteMatcher));
+template <typename... NoteMatcherTypes>
+::testing::Matcher<const Diag &> WithNote(NoteMatcherTypes... NoteMatcher) {
+ return Field(&Diag::Notes, ElementsAre(NoteMatcher...));
}
MATCHER_P2(Diag, Range, Message,
@@ -272,6 +272,51 @@ 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 94c9d0135013..0f7e9d895a00 100644
--- a/clang/include/clang/Frontend/PrecompiledPreamble.h
+++ b/clang/include/clang/Frontend/PrecompiledPreamble.h
@@ -34,6 +34,7 @@ class FileSystem;
namespace clang {
class CompilerInstance;
class CompilerInvocation;
+class Decl;
class DeclGroupRef;
class PCHContainerOperations;
@@ -293,6 +294,10 @@ 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 31c5f18eeeec..6cdfc595dcae 100644
--- a/clang/lib/Frontend/PrecompiledPreamble.cpp
+++ b/clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -255,6 +255,10 @@ 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