[clangd] Add tests covering existing header-guard behavior. NFC

[sammccall]

Summary

A few different mechanisms here that will need some work to untangle:

• self-include in a preamble being an error even if the file is ifdef-guarded
• the is-include-guarded flag not being propagated from preamble to main ast
• preambles containing the first half on an include guard discard that info

For now just record current behavior.

Relevant to:

• https://github.com/clangd/clangd/issues/811
• https://github.com/clangd/clangd/issues/377
• https://github.com/clangd/clangd/issues/262

[kadircet]

i mostly agree with the desired behaviours laid out by the tests, mentioned a coupe extra cases and wrinkly looking parts in comments.

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

724

i think we want the same behaviour for

;
#pragma once
#include "self.h"

725

#include "self.h"
#pragma once

might also be an interesting case (with preamble/main file split variations). I think all of these should raise a warning for sure, I don't think we should mark these as pragma guarded. (interestingly clangd actually somewhat works on this case today, but it feels like an accident and this code won't actually compile, so I don't think preserving clangd's current behviour would be beneficial to anyone).

733

what about

#include "self.h"
#ifndef GUARD
#define GUARD
;
#endif

I suppose this shouldn't be header guarded and raise diags

745

similar to above might be nice checking following isn't guarded and doesn't raise diags (as we won't include main file infinitely many times).

;
#ifndef GUARD
#define GUARD
#include "self.h"
#endif

759

s/There/These

804

but this is actually wrong from compiler's perspective, right ? if user wanted to compile implementation file they would hit redefinition errors. i think we should expect a header guard/pragma once on the implementation file on the common case.

[sammccall]

Added cases to address comments, and added comments about semicolons.

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

725

Done, but only with a couple of splits, as I don't think we can cover all these edge cases exhaustively and the main behavior (diagnostic) is kinda obvious.

I don't think we should mark these as pragma guarded

I can't see any principled reason (or way) to make them not pragma guarded.
#pragma once at the end of a file is a perfectly valid header guard and not observably different from having it at the top unless the file transitively includes itself.

804

The compiler doesn't really have a perspective on this code, nobody ever tries to compile impl.h (and we probably don't have a real non-inferred compile command for it).

The include guard isn't required for the compiler/build system, as long as impl.h is only ever included from iface.h and the latter is guarded. And including impl.h directly usually won't compile (guarded or not) so there's no great reason to include-guard impl.h apart from tools.

This is common I think, and seems principled:

// impl.h
#ifndef IFACE_H // include guard from another file!
#error Do not include this directly!
#endif
// no include guard
// actual impl follows

[kadircet]

thanks!

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

725

sorry i was thinking about the header guards appearing after the self-include not the pragma case, when talking about not marking them as pragma guarded. so it all makes sense, thanks!

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

 
 namespace clang {
 namespace clangd {
 namespace {
 
 using ::testing::AllOf;
 using ::testing::ElementsAre;
 using ::testing::ElementsAreArray;
+using ::testing::IsEmpty;
 
 MATCHER_P(DeclNamed, Name, "") {
   if (NamedDecl *ND = dyn_cast<NamedDecl>(arg))
     if (ND->getName() == Name)
       return true;
   if (auto *Stream = result_listener->stream()) {
     llvm::raw_os_ostream OS(*Stream);
     arg->dump(OS);
TEST(ParsedASTTest, PatchesDeletedIncludes) {
   };
   // Ensure file proximity signals are correct.
   EXPECT_EQ(StringMapToVector(PatchedAST->getIncludeStructure().includeDepth(
                 testPath("foo.cpp"))),
             StringMapToVector(ExpectedAST.getIncludeStructure().includeDepth(
                 testPath("foo.cpp"))));
 }
 
+// Returns Code guarded by #ifndef guards
+std::string guard(llvm::StringRef Code) {
+  static int GuardID = 0;
+  std::string GuardName = ("GUARD_" + llvm::Twine(++GuardID)).str();
+  return llvm::formatv("#ifndef {0}\n#define {0}\n{1}\n#endif\n", GuardName,
+                       Code);
+}
+
+std::string once(llvm::StringRef Code) {
+  return llvm::formatv("#pragma once\n{0}\n", Code);
+}
+
+bool mainIsGuarded(const ParsedAST &AST) {
+  const auto &SM = AST.getSourceManager();
+  const FileEntry *MainFE = SM.getFileEntryForID(SM.getMainFileID());
+  return AST.getPreprocessor()
+      .getHeaderSearchInfo()
+      .isFileMultipleIncludeGuarded(MainFE);
+}
+
+MATCHER_P(Diag, Desc, "") {
+  return llvm::StringRef(arg.Message).contains(Desc);
+}
+
+// Check our understanding of whether the main file is header guarded or not.
+TEST(ParsedASTTest, HeaderGuards) {
+  TestTU TU;
+  TU.ImplicitHeaderGuard = false;
+
+  TU.Code = ";";
+  EXPECT_FALSE(mainIsGuarded(TU.build()));
+
+  TU.Code = guard(";");
+  EXPECT_FALSE(mainIsGuarded(TU.build())); // FIXME: true
+
+  TU.Code = once(";");
+  EXPECT_FALSE(mainIsGuarded(TU.build())); // FIXME: true
+
+  TU.Code = R"cpp(
+    ;
+    #pragma once
+  )cpp";
+  EXPECT_FALSE(mainIsGuarded(TU.build())); // FIXME: true
+
+  TU.Code = R"cpp(
+    ;
+    #ifndef GUARD
+    #define GUARD
+    ;
+    #endif
+  )cpp";
+  EXPECT_FALSE(mainIsGuarded(TU.build()));
+}
+
+// Check our handling of files that include themselves.
+// Ideally we allow this if the file has header guards.
+//
+// Note: the semicolons (empty statements) are significant!
+// - they force the preamble to end and the body to begin. Directives can have
+//   different effects in the preamble vs main file (which we try to hide).
+// - if the preamble would otherwise cover the whole file, a trailing semicolon
+//   forces their sizes to be different. This is significant because the file
+//   size is part of the lookup key for HeaderFileInfo, and we don't want to
+//   rely on the preamble's HFI being looked up when parsing the main file.
+TEST(ParsedASTTest, HeaderGuardsSelfInclude) {
+  TestTU TU;
+  TU.ImplicitHeaderGuard = false;
+  TU.Filename = "self.h";
+
+  TU.Code = R"cpp(
+    #include "self.h" // error-ok
+    ;
+  )cpp";
+  auto AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+              ElementsAre(Diag("recursively when building a preamble")));
+  EXPECT_FALSE(mainIsGuarded(AST));
+
+  TU.Code = R"cpp(
+    ;
+    #include "self.h" // error-ok
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(), ElementsAre(Diag("nested too deeply")));
+  EXPECT_FALSE(mainIsGuarded(AST));
+
+  TU.Code = R"cpp(
+    #pragma once
+    #include "self.h"
+    ;
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(), IsEmpty());
+  EXPECT_FALSE(mainIsGuarded(AST)); // FIXME: true
+
+  TU.Code = R"cpp(
+    #pragma once
+    ;
+    #include "self.h"
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(), IsEmpty());
+  EXPECT_TRUE(mainIsGuarded(AST));
+
+  TU.Code = R"cpp(
+    ;
+    #pragma once
+    #include "self.h"
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(), IsEmpty());
+  EXPECT_TRUE(mainIsGuarded(AST));
+
+  TU.Code = R"cpp(
+    #ifndef GUARD
+    #define GUARD
+    #include "self.h" // error-ok: FIXME, this would be nice to support
+    #endif
+    ;
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+              ElementsAre(Diag("recursively when building a preamble")));
+  EXPECT_FALSE(mainIsGuarded(AST)); // FIXME: true
+
+  TU.Code = R"cpp(
+    #ifndef GUARD
+    #define GUARD
+    ;
+    #include "self.h"
+    #endif
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(), IsEmpty());
+  EXPECT_FALSE(mainIsGuarded(AST)); // FIXME: true
+
+  // Guarded too late...
+  TU.Code = R"cpp(
+    #include "self.h" // error-ok
+    #ifndef GUARD
+    #define GUARD
+    ;
+    #endif
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+              ElementsAre(Diag("recursively when building a preamble")));
+  EXPECT_FALSE(mainIsGuarded(AST));
+
+  TU.Code = R"cpp(
+    #include "self.h" // error-ok
+    ;
+    #ifndef GUARD
+    #define GUARD
+    #endif
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+              ElementsAre(Diag("recursively when building a preamble")));
+  EXPECT_FALSE(mainIsGuarded(AST));
+
+  TU.Code = R"cpp(
+    ;
+    #ifndef GUARD
+    #define GUARD
+    #include "self.h"
+    #endif
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(), IsEmpty());
+  EXPECT_FALSE(mainIsGuarded(AST));
+
+  TU.Code = R"cpp(
+    #include "self.h" // error-ok
+    #pragma once
+    ;
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+              ElementsAre(Diag("recursively when building a preamble")));
+  EXPECT_FALSE(mainIsGuarded(AST)); // FIXME: true
+
+  TU.Code = R"cpp(
+    #include "self.h" // error-ok
+    ;
+    #pragma once
+  )cpp";
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+              ElementsAre(Diag("recursively when building a preamble")));
+  EXPECT_TRUE(mainIsGuarded(AST));
+}
+
+// Tests how we handle common idioms for splitting a header-only library
+// into interface and implementation files (e.g. *.h vs *.inl).
+// These files mutually include each other, and need careful handling of include
+// guards (which interact with preambles).
+TEST(ParsedASTTest, HeaderGuardsImplIface) {
+  std::string Interface = R"cpp(
+    // error-ok: we assert on diagnostics explicitly
+    template <class T> struct Traits {
+      unsigned size();
+    };
+    #include "impl.h"
+  )cpp";
+  std::string Implementation = R"cpp(
+    // error-ok: we assert on diagnostics explicitly
+    #include "iface.h"
+    template <class T> unsigned Traits<T>::size() {
+      return sizeof(T);
+    }
+  )cpp";
+
+  TestTU TU;
+  TU.ImplicitHeaderGuard = false; // We're testing include guard handling!
+  TU.ExtraArgs.push_back("-xc++-header");
+
+  // Editing the interface file, which is include guarded (easy case).
+  // We mostly get this right via PP if we don't recognize the include guard.
+  TU.Filename = "iface.h";
+  TU.Code = guard(Interface);
+  TU.AdditionalFiles = {{"impl.h", Implementation}};
+  auto AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(), IsEmpty());
+  EXPECT_FALSE(mainIsGuarded(AST)); // FIXME: true
+  // Slightly harder: the `#pragma once` is part of the preamble, and we
+  // need to transfer it to the main file's HeaderFileInfo.
+  TU.Code = once(Interface);
+  AST = TU.build();
+  // FIXME: empty
+  EXPECT_THAT(*AST.getDiagnostics(),
+              ElementsAre(Diag("in included file: redefinition of 'Traits'")));
+  EXPECT_TRUE(mainIsGuarded(AST));
+
+  // Editing the implementation file, which is not include guarded.
+  TU.Filename = "impl.h";
+  TU.Code = Implementation;
+  TU.AdditionalFiles = {{"iface.h", guard(Interface)}};
+  AST = TU.build();
+  // The diagnostic is unfortunate in this case, but correct per our model.
+  // Ultimately the include is skipped and the code is parsed correctly though.
+  EXPECT_THAT(*AST.getDiagnostics(),
+              ElementsAre(Diag("in included file: main file cannot be included "
+                               "recursively when building a preamble")));
+  EXPECT_FALSE(mainIsGuarded(AST));
+  // Interface is pragma once guarded, same thing.
+  TU.AdditionalFiles = {{"iface.h", once(Interface)}};
+  AST = TU.build();
+  EXPECT_THAT(*AST.getDiagnostics(),
+              ElementsAre(Diag("in included file: main file cannot be included "
+                               "recursively when building a preamble")));
+  EXPECT_FALSE(mainIsGuarded(AST));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang