[clangd] Respect .clang-tidy ExtraArgs (-Wfoo only) when producing diagnostics

[sammccall]

Summary

This mechanism is used almost exclusively to enable extra warnings in clang-tidy
using ExtraArgs=-Wfoo, Checks="clang-diagnostic-foo".
Its presence is a strong signal that these flags are useful.

We choose not to actually emit them as clang-tidy diagnostics, but under their
"main" name - this ensures we show the same diagnostic in a consistent way.

We don't add the ExtraArgs to the compile command in general, but rather just
handle the -W<group> flags, which is the common case and avoids unexpected
side-effects.
And we only do this for the main file parse, when producing diagnostics.

[sammccall]

Actually we should probably handle -Wno-<group> too.

[sammccall]

Also handle -Wno-<group> flags

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/ParsedAST.cpp

270

why do we need a set here, rather than a flag?

clang-tools-extra/clangd/ParsedAST.cpp

private:
 
   const std::vector<Inclusion> Includes;
   PPCallbacks *Delegate;
   const SourceManager &SM;
   Preprocessor &PP;
   std::vector<syntax::Token> MainFileTokens;
 };
 
+// Find -W<group> and -Wno-<group> options in ExtraArgs and apply them to Diags.
+//
+// This is used to handle ExtraArgs in clang-tidy configuration.
+// We don't use clang's standard handling of this as we want slightly different
+// behavior (e.g. we want to exclude these from -Wno-error).
+void applyWarningOptions(llvm::ArrayRef<std::string> ExtraArgs,
+                         DiagnosticsEngine &Diags) {
+  for (llvm::StringRef Group : ExtraArgs) {
+    // Only handle args that are of the form -W[no-]<group>.
+    // Other flags are possible but rare and deliberately out of scope.
+    llvm::SmallVector<diag::kind> Members;
+    if (!Group.consume_front("-W") || Group.empty())
+      continue;
+    bool Enable = !Group.consume_front("no-");
+    if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
+            diag::Flavor::WarningOrError, Group, Members))
+      continue;
+
+    // Upgrade (or downgrade) the severity of each diagnostic in the group.
+    // If -Werror is on, newly added warnings will be treated as errors.
+    // We don't want this, so keep track of them to fix afterwards.
+    bool NeedsWerrorExclusion = false;
+    for (diag::kind ID : Members) {
+      if (Enable) {
+        if (Diags.getDiagnosticLevel(ID, SourceLocation()) <
+            DiagnosticsEngine::Warning) {
+          Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation());
+          if (Diags.getWarningsAsErrors())
+            NeedsWerrorExclusion = true;
+        }
+      } else {
+        Diags.setSeverity(ID, diag::Severity::Ignored, SourceLocation());
+      }
+    }
+    if (NeedsWerrorExclusion) {
+      // FIXME: there's no API to suppress -Werror for single diagnostics.
+      // In some cases with sub-groups, we may end up erroneously
+      // downgrading diagnostics that were -Werror in the compile command.
+      Diags.setDiagnosticGroupWarningAsError(Group, false);
+    }
+  }
+}
+
 } // namespace
 
 llvm::Optional<ParsedAST>
 ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
                  std::unique_ptr<clang::CompilerInvocation> CI,
                  llvm::ArrayRef<Diag> CompilerInvocationDiags,
                  std::shared_ptr<const PreambleData> Preamble) {
   trace::Span Tracer("BuildAST");
if (!Clang) {
     // The last diagnostic contains information about the reason of this
     // failure.
     std::vector<Diag> Diags(ASTDiags.take());
     elog("Failed to prepare a compiler instance: {0}",
          !Diags.empty() ? static_cast<DiagBase &>(Diags.back()).Message
                         : "unknown error");
     return None;
   }
-  if (!PreserveDiags) {
+  tidy::ClangTidyOptions ClangTidyOpts;
+  if (PreserveDiags) {
+    trace::Span Tracer("ClangTidyOpts");
+    ClangTidyOpts = getTidyOptionsForFile(Inputs.ClangTidyProvider, Filename);
+    dlog("ClangTidy configuration for file {0}: {1}", Filename,
+         tidy::configurationAsText(ClangTidyOpts));
+
+    // If clang-tidy is configured to emit clang warnings, we should too.
+    //
+    // Such clang-tidy configuration consists of two parts:
+    //   - ExtraArgs: ["-Wfoo"] causes clang to produce the warnings
+    //   - Checks: "clang-diagnostic-foo" prevents clang-tidy filtering them out
+    //
+    // We treat these as clang warnings, so the Checks part is not relevant.
+    // We must enable the warnings specified in ExtraArgs.
+    //
+    // We *don't* want to change the compile command directly. this can have
+    // too many unexpected effects: breaking the command, interactions with
+    // -- and -Werror, etc. Besides, we've already parsed the command.
+    // Instead we parse the -W<group> flags and handle them directly.
+    auto &Diags = Clang->getDiagnostics();
+    if (ClangTidyOpts.ExtraArgsBefore)
+      applyWarningOptions(*ClangTidyOpts.ExtraArgsBefore, Diags);
+    if (ClangTidyOpts.ExtraArgs)
+      applyWarningOptions(*ClangTidyOpts.ExtraArgs, Diags);
+  } else {
     // Skips some analysis.
     Clang->getDiagnosticOpts().IgnoreWarnings = true;
   }
 
   auto Action = std::make_unique<ClangdFrontendAction>();
   const FrontendInputFile &MainInput = Clang->getFrontendOpts().Inputs[0];
   if (!Action->BeginSourceFile(*Clang, MainInput)) {
     log("BeginSourceFile() failed when building AST for {0}",
ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
   std::vector<std::unique_ptr<tidy::ClangTidyCheck>> CTChecks;
   ast_matchers::MatchFinder CTFinder;
   llvm::Optional<tidy::ClangTidyContext> CTContext;
   llvm::Optional<IncludeFixer> FixIncludes;
   // No need to run clang-tidy or IncludeFixerif we are not going to surface
   // diagnostics.
   if (PreserveDiags) {
     trace::Span Tracer("ClangTidyInit");
-    tidy::ClangTidyOptions ClangTidyOpts =
-        getTidyOptionsForFile(Inputs.ClangTidyProvider, Filename);
-    dlog("ClangTidy configuration for file {0}: {1}", Filename,
-         tidy::configurationAsText(ClangTidyOpts));
     tidy::ClangTidyCheckFactories CTFactories;
     for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
       E.instantiate()->addCheckFactories(CTFactories);
     CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>(
         tidy::ClangTidyGlobalOptions(), ClangTidyOpts));
     CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
     CTContext->setASTContext(&Clang->getASTContext());
     CTContext->setCurrentFile(Filename);

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

EXPECT_THAT(
       *TU.build().getDiagnostics(),
       ifTidyChecks(UnorderedElementsAre(::testing::AllOf(
           Diag(Main.range(), "result of integer division used in a floating "
                              "point context; possible loss of precision"),
           DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"),
           DiagSeverity(DiagnosticsEngine::Error)))));
 }
 
+TidyProvider addClangArgs(std::vector<llvm::StringRef> ExtraArgs) {
+  return [ExtraArgs = std::move(ExtraArgs)](tidy::ClangTidyOptions &Opts,
+                                            llvm::StringRef) {
+    if (!Opts.ExtraArgs)
+      Opts.ExtraArgs.emplace();
+    for (llvm::StringRef Arg : ExtraArgs)
+      Opts.ExtraArgs->emplace_back(Arg);
+  };
+}
+
+TEST(DiagnosticTest, ClangTidyEnablesClangWarning) {
+  Annotations Main(R"cpp( // error-ok
+    static void [[foo]]() {}
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  // This is always emitted as a clang warning, not a clang-tidy diagnostic.
+  auto UnusedFooWarning =
+      AllOf(Diag(Main.range(), "unused function 'foo'"),
+            DiagName("-Wunused-function"), DiagSource(Diag::Clang),
+            DiagSeverity(DiagnosticsEngine::Warning));
+
+  // Check the -Wunused warning isn't initially on.
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+
+  // We enable warnings based on clang-tidy extra args.
+  TU.ClangTidyProvider = addClangArgs({"-Wunused"});
+  EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning));
+
+  // But we don't respect other args.
+  TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Dfoo=bar"});
+  EXPECT_THAT(*TU.build().getDiagnostics(), ElementsAre(UnusedFooWarning))
+      << "Not unused function 'bar'!";
+
+  // -Werror doesn't apply to warnings enabled by clang-tidy extra args.
+  TU.ExtraArgs = {"-Werror"};
+  TU.ClangTidyProvider = addClangArgs({"-Wunused"});
+  EXPECT_THAT(*TU.build().getDiagnostics(),
+              ElementsAre(DiagSeverity(DiagnosticsEngine::Warning)));
+
+  // But clang-tidy extra args won't *downgrade* errors to warnings either.
+  TU.ExtraArgs = {"-Wunused", "-Werror"};
+  TU.ClangTidyProvider = addClangArgs({"-Wunused"});
+  EXPECT_THAT(*TU.build().getDiagnostics(),
+              ElementsAre(DiagSeverity(DiagnosticsEngine::Error)));
+
+  // FIXME: we're erroneously downgrading the whole group, this should be Error.
+  TU.ExtraArgs = {"-Wunused-function", "-Werror"};
+  TU.ClangTidyProvider = addClangArgs({"-Wunused"});
+  EXPECT_THAT(*TU.build().getDiagnostics(),
+              ElementsAre(DiagSeverity(DiagnosticsEngine::Warning)));
+
+  // This looks silly, but it's the typical result if a warning is enabled by a
+  // high-level .clang-tidy file and disabled by a low-level one.
+  TU.ExtraArgs = {};
+  TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused"});
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+
+  // Overriding only works in the proper order.
+  TU.ClangTidyProvider = addClangArgs({"-Wno-unused", "-Wunused"});
+  EXPECT_THAT(*TU.build().getDiagnostics(), SizeIs(1));
+
+  // More specific vs less-specific: match clang behavior
+  TU.ClangTidyProvider = addClangArgs({"-Wunused", "-Wno-unused-function"});
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+  TU.ClangTidyProvider = addClangArgs({"-Wunused-function", "-Wno-unused"});
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+
+  // We do allow clang-tidy config to disable warnings from the compile command.
+  // It's unclear this is ideal, but it's hard to avoid.
+  TU.ExtraArgs = {"-Wunused"};
+  TU.ClangTidyProvider = addClangArgs({"-Wno-unused"});
+  EXPECT_THAT(*TU.build().getDiagnostics(), IsEmpty());
+}
+
 TEST(DiagnosticTest, LongFixMessages) {
   // We limit the size of printed code.
   Annotations Source(R"cpp(
     int main() {
       // error-ok
       int somereallyreallyreallyreallyreallyreallyreallyreallylongidentifier;
       [[omereallyreallyreallyreallyreallyreallyreallyreallylongidentifier]]= 10;
     }