[clangd] Respect preamble-patch when handling diags

[kadircet]

Summary

Depends on D143093

[kadircet]

• Rebase

[sammccall]

clang-tools-extra/clangd/Diagnostics.cpp

102

I think we answered this in the negative?

715–716

PLoc is a confusing name for something that interacts with but is not a PresumedLoc

I'd prefer avoiding auto here, too

clang-tools-extra/clangd/SourceCode.cpp

1225

just make it a function? it captures nothing

[kadircet]

• Drop FIXME for respecting all presumed locations
• Use PatchLoc instead of PLoc
• Rebase

clang-tools-extra/clangd/Diagnostics.cpp

if (!R.isValid() || M.getFileID(R.getBegin()) != M.getFileID(R.getEnd()) ||
     return false;
   return L != R.getEnd() && M.isPointWithin(L, R.getBegin(), R.getEnd());
 }
 
 // Clang diags have a location (shown as ^) and 0 or more ranges (~~~~).
 // LSP needs a single range.
 Range diagnosticRange(const clang::Diagnostic &D, const LangOptions &L) {
   auto &M = D.getSourceManager();
+  auto PatchedRange = [&M](CharSourceRange &R) {
+    // FIXME: Should we handle all presumed locations?
+    R.setBegin(translatePreamblePatchLocation(R.getBegin(), M));
+    R.setEnd(translatePreamblePatchLocation(R.getEnd(), M));
+    return R;
+  };
   auto Loc = M.getFileLoc(D.getLocation());
   for (const auto &CR : D.getRanges()) {
     auto R = Lexer::makeFileCharRange(CR, M, L);
     if (locationInRange(Loc, R, M))
-      return halfOpenToRange(M, R);
+      return halfOpenToRange(M, PatchedRange(R));
   }
   // The range may be given as a fixit hint instead.
   for (const auto &F : D.getFixItHints()) {
     auto R = Lexer::makeFileCharRange(F.RemoveRange, M, L);
     if (locationInRange(Loc, R, M))
-      return halfOpenToRange(M, R);
+      return halfOpenToRange(M, PatchedRange(R));
   }
   // If the token at the location is not a comment, we use the token.
   // If we can't get the token at the location, fall back to using the location
   auto R = CharSourceRange::getCharRange(Loc);
   Token Tok;
   if (!Lexer::getRawToken(Loc, Tok, M, L, true) && Tok.isNot(tok::comment)) {
     R = CharSourceRange::getTokenRange(Tok.getLocation(), Tok.getEndLoc());
   }
-  return halfOpenToRange(M, R);
+  return halfOpenToRange(M, PatchedRange(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) {
bool tryMoveToMainFile(Diag &D, FullSourceLoc DiagLoc) {
   D.File = SM.getFileEntryRefForID(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);
   return true;
 }
 
-bool isInsideMainFile(const clang::Diagnostic &D) {
-  if (!D.hasSourceManager())
-    return false;
-
-  return clangd::isInsideMainFile(D.getLocation(), D.getSourceManager());
-}
-
 bool isNote(DiagnosticsEngine::Level L) {
   return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark;
 }
 
 llvm::StringRef diagLeveltoString(DiagnosticsEngine::Level Lvl) {
   switch (Lvl) {
   case DiagnosticsEngine::Ignored:
     return "ignored";
if (!LangOpts || !Info.hasSourceManager()) {
     return;
   }
 
   SourceManager &SM = Info.getSourceManager();
 
   auto FillDiagBase = [&](DiagBase &D) {
     fillNonLocationData(DiagLevel, Info, D);
 
-    D.InsideMainFile = isInsideMainFile(Info);
+    auto PLoc = translatePreamblePatchLocation(Info.getLocation(), SM);
+    D.InsideMainFile = isInsideMainFile(PLoc, SM);
     D.Range = diagnosticRange(Info, *LangOpts);
-    D.File = std::string(SM.getFilename(Info.getLocation()));
-    D.AbsFile = getCanonicalPath(
-        SM.getFileEntryForID(SM.getFileID(Info.getLocation())), SM);
+    auto FID = SM.getFileID(Info.getLocation());
+    if (auto *FE = SM.getFileEntryForID(FID)) {
+      D.File = FE->getName().str();
+      D.AbsFile = getCanonicalPath(FE, SM);
+    }
     D.ID = Info.getID();
     return D;
   };
 
   auto AddFix = [&](bool SyntheticMessage) -> bool {
     assert(!Info.getFixItHints().empty() &&
            "diagnostic does not have attached fix-its");
     // No point in generating fixes, if the diagnostic is for a different file.

clang-tools-extra/clangd/Preamble.h

public:
   PreambleBounds modifiedBounds() const { return ModifiedBounds; }
 
   /// Returns textual patch contents.
   llvm::StringRef text() const { return PatchContents; }
 
   /// Whether diagnostics generated using this patch are trustable.
   bool preserveDiagnostics() const;
 
+  static constexpr llvm::StringLiteral HeaderName = "__preamble_patch__.h";
+
 private:
   static PreamblePatch create(llvm::StringRef FileName,
                               const ParseInputs &Modified,
                               const PreambleData &Baseline,
                               PatchType PatchType);
 
   PreamblePatch() = default;
   std::string PatchContents;
   std::string PatchFileName;
   /// Includes that are present in both \p Baseline and \p Modified. Used for
   /// patching includes of baseline preamble.
   std::vector<Inclusion> PreambleIncludes;
   PreambleBounds ModifiedBounds = {0, false};
 };
 
-/// Translates locations inside preamble patch to their main-file equivalent
-/// using presumed locations. Returns \p Loc if it isn't inside preamble patch.
-SourceLocation translatePreamblePatchLocation(SourceLocation Loc,
-                                              const SourceManager &SM);
-
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_PREAMBLE_H

clang-tools-extra/clangd/Preamble.cpp

 #include <string>
 #include <system_error>
 #include <utility>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
-constexpr llvm::StringLiteral PreamblePatchHeaderName = "__preamble_patch__.h";
 
 bool compileCommandsAreEqual(const tooling::CompileCommand &LHS,
                              const tooling::CompileCommand &RHS) {
   // We don't check for Output, it should not matter to clangd.
   return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
          llvm::ArrayRef(LHS.CommandLine).equals(RHS.CommandLine);
 }
 
const char *spellingForIncDirective(tok::PPKeywordKind IncludeDirective) {
   case tok::pp_include_next:
     return "include_next";
   default:
     break;
   }
   llvm_unreachable("not an include directive");
 }
 
-// Checks whether \p FileName is a valid spelling of main file.
-bool isMainFile(llvm::StringRef FileName, const SourceManager &SM) {
-  auto FE = SM.getFileManager().getFile(FileName);
-  return FE && *FE == SM.getFileEntryForID(SM.getMainFileID());
-}
-
 // Accumulating wall time timer. Similar to llvm::Timer, but much cheaper,
 // it only tracks wall time.
 // Since this is a generic timer, We may want to move this to support/ if we
 // find a use case outside of FS time tracking.
 class WallTimer {
 public:
   WallTimer() : TotalTime(std::chrono::steady_clock::duration::zero()) {}
   // [Re-]Start the timer.
PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
   if ((PatchType == PatchType::MacroDirectives || !IncludesChanged) &&
       !DirectivesChanged)
     return PreamblePatch::unmodified(Baseline);
 
   PreamblePatch PP;
   // This shouldn't coincide with any real file name.
   llvm::SmallString<128> PatchName;
   llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName),
-                          PreamblePatchHeaderName);
+                          PreamblePatch::HeaderName);
   PP.PatchFileName = PatchName.str().str();
   PP.ModifiedBounds = ModifiedScan->Bounds;
 
   llvm::raw_string_ostream Patch(PP.PatchContents);
   // Set default filename for subsequent #line directives
   Patch << "#line 0 \"";
   // FileName part of a line directive is subject to backslash escaping, which
   // might lead to problems on windows especially.

 
 PreamblePatch PreamblePatch::unmodified(const PreambleData &Preamble) {
   PreamblePatch PP;
   PP.PreambleIncludes = Preamble.Includes.MainFileIncludes;
   PP.ModifiedBounds = Preamble.Preamble.getBounds();
   return PP;
 }
 
-SourceLocation translatePreamblePatchLocation(SourceLocation Loc,
-                                              const SourceManager &SM) {
-  auto DefFile = SM.getFileID(Loc);
-  if (auto FE = SM.getFileEntryRefForID(DefFile)) {
-    auto IncludeLoc = SM.getIncludeLoc(DefFile);
-    // Preamble patch is included inside the builtin file.
-    if (IncludeLoc.isValid() && SM.isWrittenInBuiltinFile(IncludeLoc) &&
-        FE->getName().endswith(PreamblePatchHeaderName)) {
-      auto Presumed = SM.getPresumedLoc(Loc);
-      // Check that line directive is pointing at main file.
-      if (Presumed.isValid() && Presumed.getFileID().isInvalid() &&
-          isMainFile(Presumed.getFilename(), SM)) {
-        Loc = SM.translateLineCol(SM.getMainFileID(), Presumed.getLine(),
-                                  Presumed.getColumn());
-      }
-    }
-  }
-  return Loc;
-}
-
 bool PreamblePatch::preserveDiagnostics() const {
   return PatchContents.empty() ||
          Config::current().Diagnostics.Mode != Config::DiagnosticsMode::Strict;
 }
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/SourceCode.h

 
 /// Returns true if Name is reserved, like _Foo or __Vector_base.
 inline bool isReservedName(llvm::StringRef Name) {
   // This doesn't catch all cases, but the most common.
   return Name.size() >= 2 && Name[0] == '_' &&
          (isUppercase(Name[1]) || Name[1] == '_');
 }
 
+/// Translates locations inside preamble patch to their main-file equivalent
+/// using presumed locations. Returns \p Loc if it isn't inside preamble patch.
+SourceLocation translatePreamblePatchLocation(SourceLocation Loc,
+                                              const SourceManager &SM);
 } // namespace clangd
 } // namespace clang
 #endif

clang-tools-extra/clangd/SourceCode.cpp

bool isProtoFile(SourceLocation Loc, const SourceManager &SM) {
   auto FID = SM.getFileID(Loc);
   // All proto generated headers should start with this line.
   static const char *ProtoHeaderComment =
       "// Generated by the protocol buffer compiler.  DO NOT EDIT!";
   // Double check that this is an actual protobuf header.
   return SM.getBufferData(FID).startswith(ProtoHeaderComment);
 }
 
+SourceLocation translatePreamblePatchLocation(SourceLocation Loc,
+                                              const SourceManager &SM) {
+  // Checks whether \p FileName is a valid spelling of main file.
+  auto IsMainFile = [](llvm::StringRef FileName, const SourceManager &SM) {
+    auto FE = SM.getFileManager().getFile(FileName);
+    return FE && *FE == SM.getFileEntryForID(SM.getMainFileID());
+  };
+
+  auto DefFile = SM.getFileID(Loc);
+  if (auto FE = SM.getFileEntryRefForID(DefFile)) {
+    auto IncludeLoc = SM.getIncludeLoc(DefFile);
+    // Preamble patch is included inside the builtin file.
+    if (IncludeLoc.isValid() && SM.isWrittenInBuiltinFile(IncludeLoc) &&
+        FE->getName().endswith(PreamblePatch::HeaderName)) {
+      auto Presumed = SM.getPresumedLoc(Loc);
+      // Check that line directive is pointing at main file.
+      if (Presumed.isValid() && Presumed.getFileID().isInvalid() &&
+          IsMainFile(Presumed.getFilename(), SM)) {
+        Loc = SM.translateLineCol(SM.getMainFileID(), Presumed.getLine(),
+                                  Presumed.getColumn());
+      }
+    }
+  }
+  return Loc;
+}
 } // namespace clangd
 } // namespace clang

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

 #include "gtest/gtest.h"
 #include <memory>
 #include <optional>
 #include <string>
 #include <vector>
 
 using testing::Contains;
 using testing::ElementsAre;
+using testing::ElementsAreArray;
 using testing::Field;
 using testing::IsEmpty;
 using testing::Matcher;
 using testing::MatchesRegex;
 using testing::Not;
 using testing::UnorderedElementsAre;
 using testing::UnorderedElementsAreArray;
 
TEST(PreamblePatch, DiagnosticsToPreamble) {
   {
     llvm::StringLiteral BaselinePreamble = "#define [[FOO]] 1\n";
     Annotations Code(BaselinePreamble);
     // Check ranges for notes.
     Annotations NewCode(("#define BARXYZ 1\n" + BaselinePreamble +
                          "void foo();\n#define [[FOO]] 2")
                             .str());
     auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
-    // FIXME: We should be pointing at the first definition of FOO in NewCode.
     EXPECT_THAT(mainFileDiagRanges(*AST),
-                UnorderedElementsAre(NewCode.ranges()[1]));
+                UnorderedElementsAreArray(NewCode.ranges()));
   }
 }
 
 TEST(PreamblePatch, TranslatesIncludeDiagnosticsInPreamble) {
   Config Cfg;
   Cfg.Diagnostics.Mode = Config::DiagnosticsMode::Fast;
   WithContextValue WithCfg(Config::Key, std::move(Cfg));
 
llvm::StringLiteral BaselinePreamble("#include [[<foo>]]\n");
     // FIXME: We should point at the NewCode.
     EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
   }
   {
     // Drop line with diags.
     Annotations Code(BaselinePreamble);
     Annotations NewCode("#define BAR\n#include [[<bar>]]\n");
     auto AST = createPatchedAST(Code.code(), NewCode.code());
-    // FIXME: We should point at the NewCode.
-    EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+    // FIXME: We should only have ranges from the NewCode.
+    EXPECT_THAT(mainFileDiagRanges(*AST),
+                ElementsAreArray({Code.range(), NewCode.range()}));
   }
 }
 
 TEST(PreamblePatch, TranslatesNonIncludeDiagnosticsInPreamble) {
   Config Cfg;
   Cfg.Diagnostics.Mode = Config::DiagnosticsMode::Fast;
   WithContextValue WithCfg(Config::Key, std::move(Cfg));
 
ASSERT_THAT(TestTU::withCode(Annotations(BaselinePreamble).code())
     // FIXME: We should point at the NewCode.
     EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
   }
   {
     // Drop line with diags.
     Annotations Code(BaselinePreamble);
     Annotations NewCode("#define BAR\n#include [[<bar>]]\n#include <baz>\n");
     auto AST = createPatchedAST(Code.code(), NewCode.code());
-    // FIXME: We should point at the NewCode.
-    EXPECT_THAT(mainFileDiagRanges(*AST), ElementsAre(Code.range()));
+    // FIXME: We should only have the range from NewCode.
+    EXPECT_THAT(mainFileDiagRanges(*AST),
+                ElementsAreArray({Code.range(), NewCode.range()}));
   }
 }
 } // namespace
 } // namespace clangd
 } // namespace clang