[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

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

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 @@ private:
   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;
 };
 

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 @@ 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<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;

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

@@ -45,9 +45,9 @@ using ::testing::UnorderedElementsAre;
   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 {

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 @@ public:
   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 {

clang/lib/Frontend/PrecompiledPreamble.cpp

@@ -255,6 +255,10 @@ public:
     Action.setEmittedPreamblePCH(getWriter());
   }
 
+  bool shouldSkipFunctionBody(Decl *D) override {
+    return Action.Callbacks.shouldSkipFunctionBody(D);
+  }
+
 private:
   PrecompilePreambleAction &Action;
   std::unique_ptr<raw_ostream> Out;