[clangd] Correctly identify self-contained headers included rercursively

[kbobyrev]

Summary

Right now when exiting the file Headers.cpp will identify the recursive
inclusion (with a new FileID) as non self-contained and will add it to the set
from which it will never be removed. As a result, we get incorrect results in
the IncludeStructure and Include Cleaner. This patch is a fix.

[kbobyrev]

Remove unused include.

[sammccall]

clang-tools-extra/clangd/Headers.cpp

109–116

nit: reverse the if condition to avoid double-negative.

110

second sentence just echoes the code: say why, not what

isSelfContainedHeader only returns true once the full header-guard structure has been seen, i.e. when exiting the *outer* copy of the file. So last result wins.

[kbobyrev]

Resolve the comments.

clang-tools-extra/clangd/Headers.cpp

case PPCallbacks::EnterFile:
       }
       break;
     case PPCallbacks::ExitFile: {
       --Level;
       if (PrevFID == BuiltinFile)
         InBuiltinFile = false;
       // At file exit time HeaderSearchInfo is valid and can be used to
       // determine whether the file was a self-contained header or not.
-      if (const FileEntry *FE = SM.getFileEntryForID(PrevFID))
-        if (!isSelfContainedHeader(FE, PrevFID, SM, HeaderInfo))
+      if (const FileEntry *FE = SM.getFileEntryForID(PrevFID)) {
+        // isSelfContainedHeader only returns true once the full header-guard
+        // structure has been seen, i.e. when exiting the *outer* copy of the
+        // file. So last result wins.
+        if (isSelfContainedHeader(FE, PrevFID, SM, HeaderInfo))
+          Out->NonSelfContained.erase(
+              *Out->getID(SM.getFileEntryForID(PrevFID)));
+        else
           Out->NonSelfContained.insert(
               *Out->getID(SM.getFileEntryForID(PrevFID)));
+      }
       break;
     }
     case PPCallbacks::RenameFile:
     case PPCallbacks::SystemHeaderPragma:
       break;
     }
   }
 

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

 
 TEST_F(HeadersTest, SelfContainedHeaders) {
   // Including through non-builtin file has no effects.
   FS.Files[MainFile] = R"cpp(
 #include "includeguarded.h"
 #include "nonguarded.h"
 #include "pp_depend.h"
 #include "pragmaguarded.h"
+#include "recursive.h"
 )cpp";
   FS.Files["pragmaguarded.h"] = R"cpp(
 #pragma once
 )cpp";
   FS.Files["includeguarded.h"] = R"cpp(
 #ifndef INCLUDE_GUARDED_H
 #define INCLUDE_GUARDED_H
 void foo();
 #endif // INCLUDE_GUARDED_H
 )cpp";
   FS.Files["nonguarded.h"] = R"cpp(
 )cpp";
   FS.Files["pp_depend.h"] = R"cpp(
   #ifndef REQUIRED_PP_DIRECTIVE
   # 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)));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

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

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());
 }
 
+TEST(IncludeCleaner, RecursiveInclusion) {
+  TestTU TU;
+  TU.Code = R"cpp(
+    #include "foo.h"
+
+    void baz() {
+      foo();
+    }
+    )cpp";
+  TU.AdditionalFiles["foo.h"] = R"cpp(
+    #ifndef FOO_H
+    #define FOO_H
+
+    void foo() {}
+
+    #include "bar.h"
+
+    #endif
+  )cpp";
+  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());
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang