diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -311,7 +311,63 @@ : "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 flags and handle them directly. + auto ProcessWarningOptions = [&](llvm::ArrayRef ExtraArgs) { + auto &Diags = Clang->getDiagnostics(); + for (llvm::StringRef Group : ExtraArgs) { + // Only handle args that are of the form -W. + // Other flags are possible but rare and deliberately out of scope. + llvm::SmallVector Members; + if (!Group.consume_front("-W") || Group.empty()) + continue; + if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup( + diag::Flavor::WarningOrError, Group, Members)) + continue; + + // Upgrade the severity of each diagnostic in the group to warning. + // If -Werror is on, these will be treated as errors, which we must fix. + llvm::DenseSet NeedsWerrorExclusion; + for (diag::kind ID : Members) { + if (Diags.getDiagnosticLevel(ID, SourceLocation()) < + DiagnosticsEngine::Warning) { + Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation()); + if (Diags.getWarningsAsErrors()) + NeedsWerrorExclusion.insert(ID); + } + } + if (!NeedsWerrorExclusion.empty()) { + // 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); + } + } + }; + + if (ClangTidyOpts.ExtraArgs) + ProcessWarningOptions(*ClangTidyOpts.ExtraArgs); + if (ClangTidyOpts.ExtraArgsBefore) + ProcessWarningOptions(*ClangTidyOpts.ExtraArgsBefore); + } else { // Skips some analysis. Clang->getDiagnosticOpts().IgnoreWarnings = true; } @@ -348,10 +404,6 @@ // 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); diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -517,6 +517,58 @@ DiagSeverity(DiagnosticsEngine::Error))))); } +TidyProvider addClangArgs(std::vector 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))); +} + TEST(DiagnosticTest, LongFixMessages) { // We limit the size of printed code. Annotations Source(R"cpp(