diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 4b96725de441..732c36813800 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -246,6 +246,49 @@ private: std::vector MainFileTokens; }; +// Find -W and -Wno- 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 ExtraArgs, + DiagnosticsEngine &Diags) { + for (llvm::StringRef Group : ExtraArgs) { + // Only handle args that are of the form -W[no-]. + // Other flags are possible but rare and deliberately out of scope. + llvm::SmallVector 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 @@ -311,7 +354,32 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, : "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 &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; } @@ -348,10 +416,6 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, // 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 index 5cf68c9c8e9c..3d4bc1cea87c 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -517,6 +517,80 @@ TEST(DiagnosticTest, ClangTidyWarningAsError) { 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))); + + // 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(