[clangd] Include Cleaner: ignore headers with IWYU export pragmas

[kbobyrev]

Summary

Disable the warnings with IWYU pragma: export or begin_exports +
end_exports until we have support for these pragmas. There are too many
false-positive warnings for the headers that have the correct pragmas for now
and it makes the user experience very unpleasant.

[kbobyrev]

Remove unwanted formatting changes.

[kbobyrev]

Remove redundant comment.

[kadircet]

clang-tools-extra/clangd/Headers.cpp

134

the split seems to be increasing code duplication (we've already copied a bug with it 😓), what about:

bool Err = false;
    llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err);
    if (!Err || (!Text.consume_front(IWYUPragmaKeep) &&
        !Text.consume_front(IWYUPragmaExport)))
      return false;
    if(inMainFile()) {
      unsigned Offset = SM.getFileOffset(Range.getBegin());
      LastPragmaKeepInMainFileLine =
          SM.getLineNumber(SM.getMainFileID(), Offset) - 1;
    } else {
      Out->HasIWYUPragmas.insert(
        *Out->getID(SM.getFileEntryForID(SM.getFileID(Range.getBegin()))));
    }
    return false;

135

i don't think the asserts carry their weight (here and also in the headerfile comment handler). we're not really performing anything dubious if the asserts don't hold. getCharacterData will return non-sense (but valid) buffers in case of a macroid, and we're already reading data whether we're in the main file or not. moreover these are private and controlled at the entry by the public interface (HandleComment).

178

shouldn't this be if (Err || (!Text.consume ... && !Text.consume...)) return false; ? same above for the main file comment handling

clang-tools-extra/clangd/IncludeCleaner.cpp

270–271

can you also move this comment below, next to the isFileMultipleHeaderGuarded check?

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

427

no need for testPaths here and below.

438

this is passing not because you don't have a IWYU pragma comment here, but because you don't have any comments at all. can you try inserting an arbitrary comment into the file?

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

603

do you mean just foo.h is unused why are we talking about bar.h also being unused here?

[kbobyrev]

Address review comments: the structure is a bit different but the bug is now
actually removed.

[kbobyrev]

Also rebase on top of main.

[sammccall]

Great! A few more nits

clang-tools-extra/clangd/Headers.cpp

151

you never use Text again, so consume_front is unneccesary and confusing here, we can just use starts_with

151

add a FIXME that begin_export is not handled here (now that the other branch supports it)

clang-tools-extra/clangd/Headers.h

148

hasIWYUExport?
(if the file contains non-export pragmas we need to return false)

clang-tools-extra/clangd/IncludeCleaner.cpp

274

actual support for export pragmas => more precise tracking of exported headers?

(current wording doesn't really say what's missing)

[kbobyrev]

Address review comments.

clang-tools-extra/clangd/Headers.h

StringRef getRealPath(HeaderID ID) const {
     assert(static_cast<unsigned>(ID) <= RealPathNames.size());
     return RealPathNames[static_cast<unsigned>(ID)];
   }
 
   bool isSelfContained(HeaderID ID) const {
     return !NonSelfContained.contains(ID);
   }
 
+  bool hasIWYUExport(HeaderID ID) const {
+    return HasIWYUPragmas.contains(ID);
+  }
+
   // Return all transitively reachable files.
   llvm::ArrayRef<std::string> allHeaders() const { return RealPathNames; }
 
   // Return all transitively reachable files, and their minimum include depth.
   // All transitive includes (absolute paths), with their minimum include depth.
   // Root --> 0, #included file --> 1, etc.
   // Root is the ID of the header being visited first.
   llvm::DenseMap<HeaderID, unsigned>
private:
   // Identifying files in a way that persists from preamble build to subsequent
   // builds is surprisingly hard. FileID is unavailable in InclusionDirective(),
   // and RealPathName and UniqueID are not preserved in
   // the preamble.
   llvm::DenseMap<llvm::sys::fs::UniqueID, HeaderID> UIDToIndex;
   // Contains HeaderIDs of all non self-contained entries in the
   // IncludeStructure.
   llvm::DenseSet<HeaderID> NonSelfContained;
+  // Contains a set of headers that have either "IWYU pragma: export" or "IWYU
+  // pragma: begin_exports".
+  llvm::DenseSet<HeaderID> HasIWYUPragmas;
 };
 
 // Calculates insertion edit for including a new header in a file.
 class IncludeInserter {
 public:
   // If \p HeaderSearchInfo is nullptr (e.g. when compile command is
   // infeasible), this will only try to insert verbatim headers, and
   // include path of non-verbatim header will not be shortened.

clang-tools-extra/clangd/Headers.cpp

 #include "llvm/Support/Path.h"
 #include <cstring>
 
 namespace clang {
 namespace clangd {
 
 const char IWYUPragmaKeep[] = "// IWYU pragma: keep";
 const char IWYUPragmaExport[] = "// IWYU pragma: export";
+const char IWYUPragmaBeginExports[] = "// IWYU pragma: begin_exports";
 
 class IncludeStructure::RecordHeaders : public PPCallbacks,
                                         public CommentHandler {
 public:
   RecordHeaders(const CompilerInstance &CI, IncludeStructure *Out)
       : SM(CI.getSourceManager()),
         HeaderInfo(CI.getPreprocessor().getHeaderSearchInfo()), Out(Out) {}
 
case PPCallbacks::ExitFile: {
       break;
     }
     case PPCallbacks::RenameFile:
     case PPCallbacks::SystemHeaderPragma:
       break;
     }
   }
 
+  bool HandleComment(Preprocessor &PP, SourceRange Range) override {
+    bool Err = false;
+    llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err);
+    if (Err)
+      return false;
+    if (inMainFile()) {
       // Given:
       //
       // #include "foo.h"
       // #include "bar.h" // IWYU pragma: keep
       //
       // The order in which the callbacks will be triggered:
       //
       // 1. InclusionDirective("foo.h")
-  // 2. HandleComment("// IWYU pragma: keep")
+      // 2. handleCommentInMainFile("// IWYU pragma: keep")
       // 3. InclusionDirective("bar.h")
       //
-  // HandleComment will store the last location of "IWYU pragma: keep" (or
-  // export) comment in the main file, so that when InclusionDirective is
-  // called, it will know that the next inclusion is behind the IWYU pragma.
-  bool HandleComment(Preprocessor &PP, SourceRange Range) override {
-    if (!inMainFile() || Range.getBegin().isMacroID())
-      return false;
-    bool Err = false;
-    llvm::StringRef Text = SM.getCharacterData(Range.getBegin(), &Err);
-    if (Err && !Text.consume_front(IWYUPragmaKeep) &&
-        !Text.consume_front(IWYUPragmaExport))
+      // This code stores the last location of "IWYU pragma: keep" (or export)
+      // comment in the main file, so that when InclusionDirective is called, it
+      // will know that the next inclusion is behind the IWYU pragma.
+      // FIXME: Support "IWYU pragma: begin_exports" and "IWYU pragma:
+      // end_exports".
+      if (!Text.startswith(IWYUPragmaExport) &&
+          !Text.startswith(IWYUPragmaKeep))
         return false;
       unsigned Offset = SM.getFileOffset(Range.getBegin());
       LastPragmaKeepInMainFileLine =
-        SM.getLineNumber(SM.getFileID(Range.getBegin()), Offset) - 1;
+          SM.getLineNumber(SM.getMainFileID(), Offset) - 1;
+    } else {
+      // Memorize headers that that have export pragmas in them. Include Cleaner
+      // does not support them properly yet, so they will be not marked as
+      // unused.
+      // FIXME: Once IncludeCleaner supports export pragmas, remove this.
+      if (!Text.startswith(IWYUPragmaExport) &&
+          !Text.startswith(IWYUPragmaBeginExports))
+        return false;
+      Out->HasIWYUPragmas.insert(
+          *Out->getID(SM.getFileEntryForID(SM.getFileID(Range.getBegin()))));
+    }
     return false;
   }
 
 private:
   // Keeps track of include depth for the current file. It's 1 for main file.
   int Level = 0;
   bool inMainFile() const { return Level == 1; }
 
   const SourceManager &SM;
   HeaderSearch &HeaderInfo;
   // Set after entering the <built-in> file.
   FileID BuiltinFile;
   // Indicates whether <built-in> file is part of include stack.
   bool InBuiltinFile = false;
 
   IncludeStructure *Out;
 
if (Entry == MainFileEntry) {
     return static_cast<IncludeStructure::HeaderID>(0u);
   }
   auto It = UIDToIndex.find(Entry->getUniqueID());
   if (It == UIDToIndex.end())
     return llvm::None;
   return It->second;
 }
 
-IncludeStructure::HeaderID
-IncludeStructure::getOrCreateID(FileEntryRef Entry) {
+IncludeStructure::HeaderID IncludeStructure::getOrCreateID(FileEntryRef Entry) {
   // Main file's FileEntry was not known at IncludeStructure creation time.
   if (&Entry.getFileEntry() == MainFileEntry) {
     if (RealPathNames.front().empty())
       RealPathNames.front() = MainFileEntry->tryGetRealPathName().str();
     return MainFileID;
   }
   auto R = UIDToIndex.try_emplace(
       Entry.getUniqueID(),

clang-tools-extra/clangd/IncludeCleaner.cpp

static bool mayConsiderUnused(const Inclusion &Inc, ParsedAST &AST,
 
   // FIXME(kirillbobyrev): We currently do not support the umbrella headers.
   // System headers are likely to be standard library headers.
   // Until we have good support for umbrella headers, don't warn about them.
   if (Inc.Written.front() == '<') {
     if (AnalyzeStdlib && tooling::stdlib::Header::named(Inc.Written))
       return true;
     return false;
   }
-  // Headers without include guards have side effects and are not
-  // self-contained, skip them.
   assert(Inc.HeaderID);
+  auto HID = static_cast<IncludeStructure::HeaderID>(*Inc.HeaderID);
+  // FIXME: Ignore the headers with IWYU export pragmas for now, remove this
+  // check when we have more precise tracking of exported headers.
+  if (AST.getIncludeStructure().hasIWYUExport(HID))
+    return false;
   auto FE = AST.getSourceManager().getFileManager().getFileRef(
-      AST.getIncludeStructure().getRealPath(
-          static_cast<IncludeStructure::HeaderID>(*Inc.HeaderID)));
+      AST.getIncludeStructure().getRealPath(HID));
   assert(FE);
+  // 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",
          FE->getName());
     return false;
   }
   for (auto &Filter : Cfg.Diagnostics.Includes.IgnoreHeader) {
     // Convert the path to Unix slashes and try to match aginast the fiilter.

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

)cpp";
   # error You have to have PP directive set to include this one!
   #endif
 )cpp";
   FS.Files["recursive.h"] = R"cpp(
   #ifndef RECURSIVE_H
   #define RECURSIVE_H
 
   #include "recursive.h"
-  
   #endif // RECURSIVE_H
 )cpp";
 
   auto Includes = collectIncludes();
   EXPECT_TRUE(Includes.isSelfContained(getID("pragmaguarded.h", Includes)));
   EXPECT_TRUE(Includes.isSelfContained(getID("includeguarded.h", Includes)));
   EXPECT_TRUE(Includes.isSelfContained(getID("recursive.h", Includes)));
   EXPECT_FALSE(Includes.isSelfContained(getID("nonguarded.h", Includes)));
   EXPECT_FALSE(Includes.isSelfContained(getID("pp_depend.h", Includes)));
 }
 
+TEST_F(HeadersTest, HasIWYUPragmas) {
+  FS.Files[MainFile] = R"cpp(
+#include "export.h"
+#include "begin_exports.h"
+#include "none.h"
+)cpp";
+  FS.Files["export.h"] = R"cpp(
+#pragma once
+#include "none.h" // IWYU pragma: export
+)cpp";
+  FS.Files["begin_exports.h"] = R"cpp(
+#pragma once
+// IWYU pragma: begin_exports
+#include "none.h"
+// IWYU pragma: end_exports
+)cpp";
+  FS.Files["none.h"] = R"cpp(
+#pragma once
+// Not a pragma.
+)cpp";
+
+  auto Includes = collectIncludes();
+  EXPECT_TRUE(Includes.hasIWYUExport(getID("export.h", Includes)));
+  EXPECT_TRUE(Includes.hasIWYUExport(getID("begin_exports.h", Includes)));
+  EXPECT_FALSE(Includes.hasIWYUExport(getID("none.h", Includes)));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

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

TEST(IncludeCleaner, IWYUPragmas) {
   EXPECT_EQ(ReferencedFiles.SpelledUmbrellas.size(), 1u);
   EXPECT_EQ(ReferencedFiles.SpelledUmbrellas.begin()->getKey(), "\"public.h\"");
   EXPECT_EQ(ReferencedFiles.User.size(), 2u);
   EXPECT_TRUE(
       ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_TRUE(
       ReferencedFiles.User.contains(AST.getSourceManager().getMainFileID()));
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 TEST(IncludeCleaner, RecursiveInclusion) {
   TestTU TU;
   TU.Code = R"cpp(
     #include "foo.h"
 
     void baz() {
TU.AdditionalFiles["bar.h"] = guard(R"cpp(
     #include "foo.h"
   )cpp");
   ParsedAST AST = TU.build();
 
   auto ReferencedFiles = findReferencedFiles(
       findReferencedLocations(AST), AST.getIncludeStructure(),
       AST.getCanonicalIncludes(), AST.getSourceManager());
   EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
-  auto Unused = computeUnusedIncludes(AST);
-  EXPECT_THAT(Unused, IsEmpty());
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
+}
+
+TEST(IncludeCleaner, IWYUPragmaExport) {
+  TestTU TU;
+  TU.Code = R"cpp(
+    #include "foo.h"
+    )cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+    #ifndef FOO_H
+    #define FOO_H
+
+    #include "bar.h" // IWYU pragma: export
+
+    #endif
+  )cpp";
+  TU.AdditionalFiles["bar.h"] = guard(R"cpp(
+    void bar() {}
+  )cpp");
+  ParsedAST AST = TU.build();
+
+  auto ReferencedFiles = findReferencedFiles(
+      findReferencedLocations(AST), AST.getIncludeStructure(),
+      AST.getCanonicalIncludes(), AST.getSourceManager());
+  EXPECT_THAT(AST.getDiagnostics(), llvm::ValueIs(IsEmpty()));
+  // FIXME: This is not correct: foo.h is unused but is not diagnosed as such
+  // because we ignore headers with IWYU export pragmas for now.
+  EXPECT_THAT(computeUnusedIncludes(AST), IsEmpty());
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang