[Preamble] Allow recursive inclusion of header-guarded mainfile.

[sammccall]

Summary

This is guaranteed to be a no-op without the preamble, so should be a
no-op with it too.

Partially fixes https://github.com/clangd/clangd/issues/337
This doesn't yet work for #ifndef guards, which are not recognized in preambles.
see D78038

I can't for the life of me work out how to test this outside clangd.
The original reentrant preamble diagnostic was untested, I added a test
to clangd for that too.

[kadircet]

clang/lib/Lex/PPDirectives.cpp

2070

Instead of returning here I think we should set the Action to Skip. So that relevant callbacks (InclusionDirective and FileSkipped) is invoked. (maybe have a test for that too?)

[sammccall]

clang/lib/Lex/PPDirectives.cpp

2070

FileSkipped says: Callback invoked whenever a source file is skipped as the result of header guard optimization. that's not exactly what's happening here (if it were, Action is set to Skip above and we never enter this if statement).

As for calling InclusionDirective - it looks like some error paths call it and some error paths don't (returning before vs after InclusionDirective(). "Erasing" the illegal include seem valid, as would be skipping it. I'm not sure I want to change the behavior in this patch (and maybe introduce a new bug while fixing this one).

Is there some concrete reason we need this callback?

(As mentioned, I've got no idea how to reasonably test this in a fine-grained way without boiling the ocean, it was never previously tested...)

[kadircet]

thanks, lgtm!

clang/lib/Lex/PPDirectives.cpp

2070

That's how most of the tools collect includes, there are some clang-tidy checkers and also clangd features depend on receiving that callback. In the absence of that the include will be totally invisible to clangd. I suppose its OK to not support goto/documetlink for mainfile itself, it might cause troubles in feature if we plan to make use of a preamble built for one file in other files, but we can worry about that later on.

So it is ok if you want to keep current behaviour.

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

 namespace clangd {
 namespace {
 
 using ::testing::_;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::IsEmpty;
 using ::testing::Pair;
+using testing::SizeIs;
 using ::testing::UnorderedElementsAre;
 
 ::testing::Matcher<const Diag &> WithFix(::testing::Matcher<Fix> FixMatcher) {
   return Field(&Diag::Fixes, ElementsAre(FixMatcher));
 }
 
 ::testing::Matcher<const Diag &> WithFix(::testing::Matcher<Fix> FixMatcher1,
                                          ::testing::Matcher<Fix> FixMatcher2) {
#else
       int x = y;
     #endif
     )cpp");
   EXPECT_THAT(
       TestTU::withCode(Test.code()).build().getDiagnostics(),
       ElementsAre(Diag(Test.range(), "use of undeclared identifier 'b'")));
 }
 
+// Recursive main-file include is diagnosed, and doesn't crash.
+TEST(DiagnosticsTest, RecursivePreamble) {
+  auto TU = TestTU::withCode(R"cpp(
+    #include "foo.h" // error-ok
+    int symbol;
+  )cpp");
+  TU.Filename = "foo.h";
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              ElementsAre(DiagName("pp_including_mainfile_in_preamble")));
+  EXPECT_THAT(TU.build().getLocalTopLevelDecls(), SizeIs(1));
+}
+
+// Recursive main-file include with #pragma once guard is OK.
+TEST(DiagnosticsTest, RecursivePreamblePragmaOnce) {
+  auto TU = TestTU::withCode(R"cpp(
+    #pragma once
+    #include "foo.h"
+    int symbol;
+  )cpp");
+  TU.Filename = "foo.h";
+  EXPECT_THAT(TU.build().getDiagnostics(), IsEmpty());
+  EXPECT_THAT(TU.build().getLocalTopLevelDecls(), SizeIs(1));
+}
+
+// Recursive main-file include with #ifndef guard should be OK.
+// However, it's not yet recognized (incomplete at end of preamble).
+TEST(DiagnosticsTest, RecursivePreambleIfndefGuard) {
+  auto TU = TestTU::withCode(R"cpp(
+    #ifndef FOO
+    #define FOO
+    #include "foo.h" // error-ok
+    int symbol;
+    #endif
+  )cpp");
+  TU.Filename = "foo.h";
+  // FIXME: should be no errors here.
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              ElementsAre(DiagName("pp_including_mainfile_in_preamble")));
+  EXPECT_THAT(TU.build().getLocalTopLevelDecls(), SizeIs(1));
+}
+
 TEST(DiagnosticsTest, InsideMacros) {
   Annotations Test(R"cpp(
     #define TEN 10
     #define RET(x) return x + 10
 
     int* foo() {
       RET($foo[[0]]); // error-ok
     }

clang/lib/Lex/PPDirectives.cpp

Optional<FileEntryRef> File = LookupHeaderIncludeOrImport(
       LookupFilename, RelativePath, SearchPath, SuggestedModule, isAngled);
 
   if (usingPCHWithThroughHeader() && SkippingUntilPCHThroughHeader) {
     if (File && isPCHThroughHeader(&File->getFileEntry()))
       SkippingUntilPCHThroughHeader = false;
     return {ImportAction::None};
   }
 
-  // Check for circular inclusion of the main file.
-  // We can't generate a consistent preamble with regard to the conditional
-  // stack if the main file is included again as due to the preamble bounds
-  // some directives (e.g. #endif of a header guard) will never be seen.
-  // Since this will lead to confusing errors, avoid the inclusion.
-  if (File && PreambleConditionalStack.isRecording() &&
-      SourceMgr.translateFile(&File->getFileEntry()) ==
-          SourceMgr.getMainFileID()) {
-    Diag(FilenameTok.getLocation(),
-         diag::err_pp_including_mainfile_in_preamble);
-    return {ImportAction::None};
-  }
-
   // Should we enter the source file? Set to Skip if either the source file is
   // known to have no effect beyond its effect on module visibility -- that is,
   // if it's got an include guard that is already defined, set to Import if it
   // is a modular header we've already built and should import.
   enum { Enter, Import, Skip, IncludeLimitReached } Action = Enter;
 
   if (PPOpts->SingleFileParseMode)
     Action = IncludeLimitReached;
if (Action == Enter && File &&
     // modular because we might not have imported it from this submodule before.
     //
     // FIXME: We don't do this when compiling a PCH because the AST
     // serialization layer can't cope with it. This means we get local
     // submodule visibility semantics wrong in that case.
     Action = (SuggestedModule && !getLangOpts().CompilingPCH) ? Import : Skip;
   }
 
+  // Check for circular inclusion of the main file.
+  // We can't generate a consistent preamble with regard to the conditional
+  // stack if the main file is included again as due to the preamble bounds
+  // some directives (e.g. #endif of a header guard) will never be seen.
+  // Since this will lead to confusing errors, avoid the inclusion.
+  if (Action == Enter && File && PreambleConditionalStack.isRecording() &&
+      SourceMgr.translateFile(&File->getFileEntry()) ==
+          SourceMgr.getMainFileID()) {
+    Diag(FilenameTok.getLocation(),
+         diag::err_pp_including_mainfile_in_preamble);
+    return {ImportAction::None};
+  }
+
   if (Callbacks && !IsImportDecl) {
     // Notify the callback object that we've seen an inclusion directive.
     // FIXME: Use a different callback for a pp-import?
     Callbacks->InclusionDirective(
         HashLoc, IncludeTok, LookupFilename, isAngled, FilenameRange,
         File ? &File->getFileEntry() : nullptr, SearchPath, RelativePath,
         Action == Import ? SuggestedModule.getModule() : nullptr,
         FileCharacter);