[clangd] Treat preamble patch as main file for include-cleaner analysis

[kadircet]

Summary

Since we redefine all macros in preamble-patch, and it's parsed after
consuming the preamble macros, we can get false missing-include diagnostics
while a fresh preamble is being rebuilt.

This patch makes sure preamble-patch is treated same as main file for
include-cleaner purposes.

[sammccall]

clang-tools-extra/clangd/IncludeCleaner.cpp

380

Comparing strings here every time seems odd & slow.

Is it too fragile to add a function somewhere (Preamble.h?) to get the preamble patch file ID from a source manager? (By reconstructing the path and then looking it up)

That way it can be done outside this loop, and without encoding such details here

[kadircet]

• Expose helper to get preamble patch entry

[sammccall]

clang-tools-extra/clangd/Preamble.h

144

up to you, but this could be tested I think (just make sure it points to something valid with the right name when you have a patch and not otherwise)

clang-tools-extra/clangd/IncludeCleaner.cpp

 //===--- IncludeCleaner.cpp - Unused/Missing Headers Analysis ---*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "IncludeCleaner.h"
 #include "Config.h"
 #include "Diagnostics.h"
 #include "Headers.h"
 #include "ParsedAST.h"
+#include "Preamble.h"
 #include "Protocol.h"
 #include "SourceCode.h"
 #include "URI.h"
 #include "clang-include-cleaner/Analysis.h"
 #include "clang-include-cleaner/Record.h"
 #include "clang-include-cleaner/Types.h"
 #include "support/Logger.h"
 #include "support/Path.h"
static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST,
   auto FE = AST.getSourceManager().getFileManager().getFileRef(
       AST.getIncludeStructure().getRealPath(HID));
   assert(FE);
   if (PI) {
     if (PI->shouldKeep(Inc.HashLine + 1))
       return false;
     // Check if main file is the public interface for a private header. If so we
     // shouldn't diagnose it as unused.
-    if(auto PHeader = PI->getPublic(*FE); !PHeader.empty()) {
+    if (auto PHeader = PI->getPublic(*FE); !PHeader.empty()) {
       PHeader = PHeader.trim("<>\"");
       // Since most private -> public mappings happen in a verbatim way, we
       // check textually here. This might go wrong in presence of symlinks or
       // header mappings. But that's not different than rest of the places.
-      if(AST.tuPath().endswith(PHeader))
+      if (AST.tuPath().endswith(PHeader))
         return false;
     }
   }
   // Headers without include guards have side effects and are not
   // self-contained, skip them.
   if (!AST.getPreprocessor().getHeaderSearchInfo().isFileMultipleIncludeGuarded(
           &FE->getFileEntry())) {
     dlog("{0} doesn't have header guard and will not be considered unused",

 }
 
 IncludeCleanerFindings computeIncludeCleanerFindings(ParsedAST &AST) {
   const auto &SM = AST.getSourceManager();
   const auto &Includes = AST.getIncludeStructure();
   include_cleaner::Includes ConvertedIncludes =
       convertIncludes(SM, Includes.MainFileIncludes);
   const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID());
+  auto *PreamblePatch = PreamblePatch::getPatchEntry(AST.tuPath(), SM);
 
   std::vector<include_cleaner::SymbolReference> Macros =
       collectMacroReferences(AST);
   std::vector<MissingIncludeDiagInfo> MissingIncludes;
   llvm::DenseSet<IncludeStructure::HeaderID> Used;
   trace::Span Tracer("include_cleaner::walkUsed");
   include_cleaner::walkUsed(
       AST.getLocalTopLevelDecls(), /*MacroRefs=*/Macros,
       AST.getPragmaIncludes(), SM,
       [&](const include_cleaner::SymbolReference &Ref,
           llvm::ArrayRef<include_cleaner::Header> Providers) {
         bool Satisfied = false;
         for (const auto &H : Providers) {
           if (H.kind() == include_cleaner::Header::Physical &&
-              H.physical() == MainFile) {
+              (H.physical() == MainFile || H.physical() == PreamblePatch)) {
             Satisfied = true;
             continue;
           }
           for (auto *Inc : ConvertedIncludes.match(H)) {
             Satisfied = true;
             auto HeaderID = Includes.getID(Inc->Resolved);
             assert(HeaderID.has_value() &&
                    "ConvertedIncludes only contains resolved includes.");
             Used.insert(*HeaderID);

clang-tools-extra/clangd/Preamble.h

 #include "CollectMacros.h"
 #include "Compiler.h"
 #include "Diagnostics.h"
 #include "FS.h"
 #include "Headers.h"
 #include "clang-include-cleaner/Record.h"
 #include "index/CanonicalIncludes.h"
 #include "support/Path.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 
+#include <cstddef>
+#include <functional>
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 
 /// The parsed preamble and associated data.
 ///
 /// As we must avoid re-parsing the preamble, any information that can only
public:
   /// FIXME: This only handles include directives, we should at least handle
   /// define/undef.
   static PreamblePatch createFullPatch(llvm::StringRef FileName,
                                        const ParseInputs &Modified,
                                        const PreambleData &Baseline);
   static PreamblePatch createMacroPatch(llvm::StringRef FileName,
                                         const ParseInputs &Modified,
                                         const PreambleData &Baseline);
+  /// Returns the FileEntry for the preamble patch of MainFilePath in SM, if
+  /// any.
+  static const FileEntry *getPatchEntry(llvm::StringRef MainFilePath,
+                                        const SourceManager &SM);
 
   /// Adjusts CI (which compiles the modified inputs) to be used with the
   /// baseline preamble. This is done by inserting an artifical include to the
   /// \p CI that contains new directives calculated in create.
   void apply(CompilerInvocation &CI) const;
 
   /// Returns #include directives from the \c Modified preamble that were
   /// resolved using the \c Baseline preamble. This covers the new locations of

clang-tools-extra/clangd/Preamble.cpp

static std::vector<Diag> patchDiags(llvm::ArrayRef<Diag> BaselineDiags,
   DiagPatcher Patcher(BaselineScan.Lines, ModifiedScan.Lines);
   for (auto &D : BaselineDiags) {
     if (auto NewD = Patcher.translateDiag(D))
       PatchedDiags.emplace_back(std::move(*NewD));
   }
   return PatchedDiags;
 }
 
+static std::string getPatchName(llvm::StringRef FileName) {
+  // This shouldn't coincide with any real file name.
+  llvm::SmallString<128> PatchName;
+  llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName),
+                          PreamblePatch::HeaderName);
+  return PatchName.str().str();
+}
+
 PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
                                     const ParseInputs &Modified,
                                     const PreambleData &Baseline,
                                     PatchType PatchType) {
   trace::Span Tracer("CreatePreamblePatch");
   SPAN_ATTACH(Tracer, "File", FileName);
   assert(llvm::sys::path::is_absolute(FileName) && "relative FileName!");
   // First scan preprocessor directives in Baseline and Modified. These will be
PreamblePatch PreamblePatch::create(llvm::StringRef FileName,
   bool DirectivesChanged =
       BaselineScan->TextualDirectives != ModifiedScan->TextualDirectives;
   if ((PatchType == PatchType::MacroDirectives || !IncludesChanged) &&
       !DirectivesChanged)
     return PreamblePatch::unmodified(Baseline);
 
   PreamblePatch PP;
   PP.Baseline = &Baseline;
-  // This shouldn't coincide with any real file name.
-  llvm::SmallString<128> PatchName;
-  llvm::sys::path::append(PatchName, llvm::sys::path::parent_path(FileName),
-                          PreamblePatch::HeaderName);
-  PP.PatchFileName = PatchName.str().str();
+  PP.PatchFileName = getPatchName(FileName);
   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.
   escapeBackslashAndQuotes(FileName, Patch);
llvm::ArrayRef<PragmaMark> PreamblePatch::marks() const {
   return PatchedMarks;
 }
 
 const MainFileMacros &PreamblePatch::mainFileMacros() const {
   if (PatchContents.empty())
     return Baseline->Macros;
   return PatchedMacros;
 }
+
+const FileEntry *PreamblePatch::getPatchEntry(llvm::StringRef MainFilePath,
+                                              const SourceManager &SM) {
+  auto PatchFilePath = getPatchName(MainFilePath);
+  if (auto File = SM.getFileManager().getFile(PatchFilePath))
+    return *File;
+  return nullptr;
+}
 } // namespace clangd
 } // namespace clang

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

EXPECT_THAT(*AST->getDiagnostics(),
                 ElementsAre(Diag(NewCode.range(), "missing_type_specifier")));
   }
 }
 
 TEST(PreamblePatch, DiagnosticsToPreamble) {
   Config Cfg;
   Cfg.Diagnostics.AllowStalePreamble = true;
   Cfg.Diagnostics.UnusedIncludes = Config::IncludesPolicy::Strict;
+  Cfg.Diagnostics.MissingIncludes = Config::IncludesPolicy::Strict;
   WithContextValue WithCfg(Config::Key, std::move(Cfg));
 
   llvm::StringMap<std::string> AdditionalFiles;
   AdditionalFiles["foo.h"] = "#pragma once";
   AdditionalFiles["bar.h"] = "#pragma once";
   {
     Annotations Code(R"(
 // Test comment
$foo[[#include "foo.h"]])");
     EXPECT_THAT(
         *AST->getDiagnostics(),
         UnorderedElementsAre(Diag(NewCode.range("bar"), "unused-includes"),
                              Diag(NewCode.range("foo"), "unused-includes")));
   }
   {
     Annotations Code("#define [[FOO]] 1\n");
     // Check ranges for notes.
+    // This also makes sure we don't generate missing-include diagnostics
+    // because macros are redefined in preamble-patch.
     Annotations NewCode(R"(#define BARXYZ 1
 #define $foo1[[FOO]] 1
 void foo();
 #define $foo2[[FOO]] 2)");
     auto AST = createPatchedAST(Code.code(), NewCode.code(), AdditionalFiles);
     EXPECT_THAT(
         *AST->getDiagnostics(),
         ElementsAre(AllOf(Diag(NewCode.range("foo2"), "-Wmacro-redefined"),
#endif)cpp");
     EXPECT_THAT(AST->getMacros().Names.keys(),
                 UnorderedElementsAreArray({"FOO", "BAR", "BAZ"}));
     EXPECT_THAT(AST->getMarks(),
                 UnorderedElementsAre(Mark(NewCode.range("x"), " XX"),
                                      Mark(NewCode.range("y"), " YY")));
   }
 }
 
+TEST(PreamblePatch, PatchFileEntry) {
+  Annotations Code(R"cpp(#define FOO)cpp");
+  Annotations NewCode(R"cpp(
+#define BAR
+#define FOO)cpp");
+  {
+    auto AST = createPatchedAST(Code.code(), Code.code());
+    EXPECT_EQ(
+        PreamblePatch::getPatchEntry(AST->tuPath(), AST->getSourceManager()),
+        nullptr);
+  }
+  {
+    auto AST = createPatchedAST(Code.code(), NewCode.code());
+    auto *FE =
+        PreamblePatch::getPatchEntry(AST->tuPath(), AST->getSourceManager());
+    ASSERT_NE(FE, nullptr);
+    EXPECT_THAT(FE->getName().str(),
+                testing::EndsWith(PreamblePatch::HeaderName.str()));
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang