[clangd] Suppress warning about system_header pragma when editing headers

[sammccall]

Summary

Not sure it's OK to suppress this in clang itself - if we're building a PCH
or module, maybe it matters?

[sammccall]

I wasn't sure whether to suppress this in clang or clangd, wonder what you think.

[hokein]

In D116925#3230991, @sammccall wrote:

I wasn't sure whether to suppress this in clang or clangd, wonder what you think.

I think it is fine to do it in clang. pp_pragma_once_in_main_file is a similar diag, and clang has already suppressed it.

[sammccall]

add fixme

[sammccall]

In D116925#3231110, @hokein wrote:

In D116925#3230991, @sammccall wrote:

I wasn't sure whether to suppress this in clang or clangd, wonder what you think.

I think it is fine to do it in clang. pp_pragma_once_in_main_file is a similar diag, and clang has already suppressed it.

As discussed offline, if we drop the warning in clang it's not clear whether we can/should keep ignoring the pragma.
Probably the principled answer is we should respect the pragma, but the implications of the extra FileChanged call aren't clear to me.

Simply suppressing the warning in clang (but continuing to ignore the pragma) seems no more principled than this patch.
Until we work it out, I think we should just suppress here.

[hokein]

add a unittest?

[sammccall]

add test, fix build

clang-tools-extra/clangd/Diagnostics.h

private:
   bool LastDiagOriginallyError = false;      // Valid only when LastDiag is set.
   SourceManager *OrigSrcMgr = nullptr;
 
   llvm::DenseSet<std::pair<unsigned, unsigned>> IncludedErrorLocations;
 };
 
 /// Determine whether a (non-clang-tidy) diagnostic is suppressed by config.
 bool isBuiltinDiagnosticSuppressed(unsigned ID,
-                                   const llvm::StringSet<> &Suppressed);
+                                   const llvm::StringSet<> &Suppressed,
+                                   const LangOptions &);
 /// Take a user-specified diagnostic code, and convert it to a normalized form
 /// stored in the config and consumed by isBuiltinDiagnosticsSuppressed.
 ///
 /// (This strips err_ and -W prefix so we can match with or without them.)
 llvm::StringRef normalizeSuppressedCode(llvm::StringRef);
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H

clang-tools-extra/clangd/Diagnostics.cpp

if (!LastDiag->InsideMainFile && LastDiagLoc && LastDiagOriginallyError) {
     }
   }
   if (!mentionsMainFile(*LastDiag))
     return;
   Output.push_back(std::move(*LastDiag));
 }
 
 bool isBuiltinDiagnosticSuppressed(unsigned ID,
-                                   const llvm::StringSet<> &Suppress) {
+                                   const llvm::StringSet<> &Suppress,
+                                   const LangOptions &LangOpts) {
+  // Don't complain about header-only stuff in mainfiles if it's a header.
+  // FIXME: would be cleaner to suppress in clang, once we decide whether the
+  //        behavior should be to silently-ignore or respect the pragma.
+  if (ID == diag::pp_pragma_sysheader_in_main_file && LangOpts.IsHeaderFile)
+    return true;
+
   if (const char *CodePtr = getDiagnosticCode(ID)) {
     if (Suppress.contains(normalizeSuppressedCode(CodePtr)))
       return true;
   }
   StringRef Warning = DiagnosticIDs::getWarningOptionForDiag(ID);
   if (!Warning.empty() && Suppress.contains(Warning))
     return true;
   return false;

clang-tools-extra/clangd/ParsedAST.cpp

for (auto ID : {diag::ext_implicit_function_decl,
       Clang->getDiagnostics().setSeverity(ID, diag::Severity::Error,
                                           SourceLocation());
     }
 
     const Config &Cfg = Config::current();
     ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
                                   const clang::Diagnostic &Info) {
       if (Cfg.Diagnostics.SuppressAll ||
-          isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress))
+          isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
+                                        Clang->getLangOpts()))
         return DiagnosticsEngine::Ignored;
 
       auto It = OverriddenSeverity.find(Info.getID());
       if (It != OverriddenSeverity.end())
         DiagLevel = It->second;
 
       if (!CTChecks.empty()) {
         std::string CheckName = CTContext->getCheckName(Info.getID());

clang-tools-extra/clangd/Preamble.cpp

PreambleDiagnostics.setDiagCallback(
       });
   llvm::IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
       CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(),
                                           &PreambleDiagnostics, false);
   const Config &Cfg = Config::current();
   PreambleDiagnostics.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
                                            const clang::Diagnostic &Info) {
     if (Cfg.Diagnostics.SuppressAll ||
-        isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress))
+        isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress,
+                                      *CI.getLangOpts()))
       return DiagnosticsEngine::Ignored;
     switch (Info.getID()) {
     case diag::warn_no_newline_eof:
     case diag::warn_cxx98_compat_no_newline_eof:
     case diag::ext_no_newline_eof:
       // If the preamble doesn't span the whole file, drop the no newline at
       // eof warnings.
       return Bounds.Size != ContentsBuffer->getBufferSize()

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

TEST_F(ConfigCompileTests, DiagnosticSuppression) {
   Frag.Diagnostics.Suppress.emplace_back("err_unexpected_friend");
   Frag.Diagnostics.Suppress.emplace_back("warn_alloca");
   EXPECT_TRUE(compileAndApply());
   EXPECT_THAT(Conf.Diagnostics.Suppress.keys(),
               UnorderedElementsAre("bugprone-use-after-move",
                                    "unreachable-code", "unused-variable",
                                    "typecheck_bool_condition",
                                    "unexpected_friend", "warn_alloca"));
-  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(diag::warn_unreachable,
-                                            Conf.Diagnostics.Suppress));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::warn_unreachable, Conf.Diagnostics.Suppress, LangOptions()));
   // Subcategory not respected/suppressed.
-  EXPECT_FALSE(isBuiltinDiagnosticSuppressed(diag::warn_unreachable_break,
-                                             Conf.Diagnostics.Suppress));
-  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(diag::warn_unused_variable,
-                                            Conf.Diagnostics.Suppress));
+  EXPECT_FALSE(isBuiltinDiagnosticSuppressed(
+      diag::warn_unreachable_break, Conf.Diagnostics.Suppress, LangOptions()));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::warn_unused_variable, Conf.Diagnostics.Suppress, LangOptions()));
   EXPECT_TRUE(isBuiltinDiagnosticSuppressed(diag::err_typecheck_bool_condition,
-                                            Conf.Diagnostics.Suppress));
-  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(diag::err_unexpected_friend,
-                                            Conf.Diagnostics.Suppress));
-  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(diag::warn_alloca,
-                                            Conf.Diagnostics.Suppress));
+                                            Conf.Diagnostics.Suppress,
+                                            LangOptions()));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::err_unexpected_friend, Conf.Diagnostics.Suppress, LangOptions()));
+  EXPECT_TRUE(isBuiltinDiagnosticSuppressed(
+      diag::warn_alloca, Conf.Diagnostics.Suppress, LangOptions()));
 
   Frag.Diagnostics.Suppress.emplace_back("*");
   EXPECT_TRUE(compileAndApply());
   EXPECT_TRUE(Conf.Diagnostics.SuppressAll);
   EXPECT_THAT(Conf.Diagnostics.Suppress, IsEmpty());
 }
 
 TEST_F(ConfigCompileTests, Tidy) {

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

Annotations Test(R"cpp(
     [[Define]](main) // error-ok
   )cpp");
   auto TU = TestTU::withCode(Test.code());
   EXPECT_THAT(*TU.build().getDiagnostics(),
               ElementsAre(AllOf(Diag(Test.range(), "'main' must return 'int'"),
                                 Not(WithFix(_)))));
 }
 
+TEST(DiagnosticsTest, PragmaSystemHeader) {
+  Annotations Test("#pragma clang [[system_header]]\n");
+  auto TU = TestTU::withCode(Test.code());
+  EXPECT_THAT(
+      *TU.build().getDiagnostics(),
+      ElementsAre(AllOf(
+          Diag(Test.range(), "#pragma system_header ignored in main file"))));
+  TU.Filename = "TestTU.h";
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+}
+
 TEST(ClangdTest, MSAsm) {
   // Parsing MS assembly tries to use the target MCAsmInfo, which we don't link.
   // We used to crash here. Now clang emits a diagnostic, which we filter out.
   llvm::InitializeAllTargetInfos(); // As in ClangdMain
   auto TU = TestTU::withCode("void fn() { __asm { cmp cl,64 } }");
   TU.ExtraArgs = {"-fms-extensions"};
   EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
 }