Skip to content

Commit

Permalink
[clangd] Allow diagnostics to be suppressed with configuration
Browse files Browse the repository at this point in the history
This has been specifically requested:
  clangd/vscode-clangd#114
and various issues can be addressed with this as a workaround, e.g.:
  clangd/clangd#662

Differential Revision: https://reviews.llvm.org/D95349
  • Loading branch information
sam-mccall authored and memfrob committed Oct 4, 2022
1 parent b640d69 commit 9211381
Show file tree
Hide file tree
Showing 9 changed files with 149 additions and 7 deletions.
7 changes: 7 additions & 0 deletions clang-tools-extra/clangd/Config.h
Expand Up @@ -28,6 +28,7 @@
#include "llvm/ADT/FunctionExtras.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringSet.h"
#include <string>
#include <vector>

Expand Down Expand Up @@ -77,6 +78,12 @@ struct Config {
llvm::Optional<ExternalIndexSpec> External;
} Index;

/// Controls warnings and errors when parsing code.
struct {
bool SuppressAll = false;
llvm::StringSet<> Suppress;
} Diagnostics;

/// Style of the codebase.
struct {
// Namespaces that should always be fully qualified, meaning no "using"
Expand Down
23 changes: 23 additions & 0 deletions clang-tools-extra/clangd/ConfigCompile.cpp
Expand Up @@ -27,6 +27,7 @@
#include "Config.h"
#include "ConfigFragment.h"
#include "ConfigProvider.h"
#include "Diagnostics.h"
#include "Features.inc"
#include "TidyProvider.h"
#include "support/Logger.h"
Expand Down Expand Up @@ -187,6 +188,7 @@ struct FragmentCompiler {
compile(std::move(F.If));
compile(std::move(F.CompileFlags));
compile(std::move(F.Index));
compile(std::move(F.Diagnostics));
compile(std::move(F.ClangTidy));
}

Expand Down Expand Up @@ -328,6 +330,27 @@ struct FragmentCompiler {
});
}

void compile(Fragment::DiagnosticsBlock &&F) {
std::vector<llvm::StringRef> Normalized;
for (const auto &Suppressed : F.Suppress) {
if (*Suppressed == "*") {
Out.Apply.push_back([&](const Params &, Config &C) {
C.Diagnostics.SuppressAll = true;
C.Diagnostics.Suppress.clear();
});
return;
}
Normalized.push_back(normalizeSuppressedCode(*Suppressed));
}
if (!Normalized.empty())
Out.Apply.push_back([Normalized](const Params &, Config &C) {
if (C.Diagnostics.SuppressAll)
return;
for (llvm::StringRef N : Normalized)
C.Diagnostics.Suppress.insert(N);
});
}

void compile(Fragment::StyleBlock &&F) {
if (!F.FullyQualifiedNamespaces.empty()) {
std::vector<std::string> FullyQualifiedNamespaces;
Expand Down
19 changes: 19 additions & 0 deletions clang-tools-extra/clangd/ConfigFragment.h
Expand Up @@ -181,6 +181,24 @@ struct Fragment {
};
IndexBlock Index;

/// Controls behavior of diagnostics (errors and warnings).
struct DiagnosticsBlock {
/// Diagnostic codes that should be suppressed.
///
/// Valid values are:
/// - *, to disable all diagnostics
/// - diagnostic codes exposed by clangd (e.g unknown_type, -Wunused-result)
/// - clang internal diagnostic codes (e.g. err_unknown_type)
/// - warning categories (e.g. unused-result)
/// - clang-tidy check names (e.g. bugprone-narrowing-conversions)
///
/// This is a simple filter. Diagnostics can be controlled in other ways
/// (e.g. by disabling a clang-tidy check, or the -Wunused compile flag).
/// This often has other advantages, such as skipping some analysis.
std::vector<Located<std::string>> Suppress;
};
DiagnosticsBlock Diagnostics;

// Describes the style of the codebase, beyond formatting.
struct StyleBlock {
// Namespaces that should always be fully qualified, meaning no "using"
Expand All @@ -195,6 +213,7 @@ struct Fragment {
///
/// The settings are merged with any settings found in .clang-tidy
/// configiration files with these ones taking precedence.
// FIXME: move this to Diagnostics.Tidy.
struct ClangTidyBlock {
std::vector<Located<std::string>> Add;
/// List of checks to disable.
Expand Down
18 changes: 18 additions & 0 deletions clang-tools-extra/clangd/Diagnostics.cpp
Expand Up @@ -801,5 +801,23 @@ void StoreDiags::flushLastDiag() {
Output.push_back(std::move(*LastDiag));
}

bool isBuiltinDiagnosticSuppressed(unsigned ID,
const llvm::StringSet<> &Suppress) {
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;
}

llvm::StringRef normalizeSuppressedCode(llvm::StringRef Code) {
Code.consume_front("err_");
Code.consume_front("-W");
return Code;
}

} // namespace clangd
} // namespace clang
9 changes: 9 additions & 0 deletions clang-tools-extra/clangd/Diagnostics.h
Expand Up @@ -159,6 +159,15 @@ class StoreDiags : public DiagnosticConsumer {
bool LastPrimaryDiagnosticWasSuppressed = false;
};

/// Determine whether a (non-clang-tidy) diagnostic is suppressed by config.
bool isBuiltinDiagnosticSuppressed(unsigned ID,
const llvm::StringSet<> &Suppressed);
/// 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

Expand Down
19 changes: 13 additions & 6 deletions clang-tools-extra/clangd/ParsedAST.cpp
Expand Up @@ -12,6 +12,7 @@
#include "../clang-tidy/ClangTidyModuleRegistry.h"
#include "AST.h"
#include "Compiler.h"
#include "Config.h"
#include "Diagnostics.h"
#include "Headers.h"
#include "IncludeFixer.h"
Expand Down Expand Up @@ -315,12 +316,18 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
Check->registerMatchers(&CTFinder);
}

if (!CTChecks.empty()) {
ASTDiags.setLevelAdjuster([&CTContext](DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) {
ASTDiags.setLevelAdjuster([&, &Cfg(Config::current())](
DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) {
if (Cfg.Diagnostics.SuppressAll ||
isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress))
return DiagnosticsEngine::Ignored;
if (!CTChecks.empty()) {
std::string CheckName = CTContext->getCheckName(Info.getID());
bool IsClangTidyDiag = !CheckName.empty();
if (IsClangTidyDiag) {
if (Cfg.Diagnostics.Suppress.contains(CheckName))
return DiagnosticsEngine::Ignored;
// Check for suppression comment. Skip the check for diagnostics not
// in the main file, because we don't want that function to query the
// source buffer for preamble files. For the same reason, we ask
Expand All @@ -342,9 +349,9 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
return DiagnosticsEngine::Error;
}
}
return DiagLevel;
});
}
}
return DiagLevel;
});
}

// Add IncludeFixer which can recover diagnostics caused by missing includes
Expand Down
35 changes: 35 additions & 0 deletions clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
Expand Up @@ -11,6 +11,7 @@
#include "ConfigTesting.h"
#include "Features.inc"
#include "TestFS.h"
#include "clang/Basic/DiagnosticSema.h"
#include "llvm/ADT/None.h"
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/StringRef.h"
Expand All @@ -30,6 +31,7 @@ using ::testing::ElementsAre;
using ::testing::IsEmpty;
using ::testing::SizeIs;
using ::testing::StartsWith;
using ::testing::UnorderedElementsAre;

class ConfigCompileTests : public ::testing::Test {
protected:
Expand Down Expand Up @@ -183,6 +185,39 @@ TEST_F(ConfigCompileTests, PathSpecMatch) {
}
}

TEST_F(ConfigCompileTests, DiagnosticSuppression) {
Frag.Diagnostics.Suppress.emplace_back("bugprone-use-after-move");
Frag.Diagnostics.Suppress.emplace_back("unreachable-code");
Frag.Diagnostics.Suppress.emplace_back("-Wunused-variable");
Frag.Diagnostics.Suppress.emplace_back("typecheck_bool_condition");
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));
// 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_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));

Frag.Diagnostics.Suppress.emplace_back("*");
EXPECT_TRUE(compileAndApply());
EXPECT_TRUE(Conf.Diagnostics.SuppressAll);
EXPECT_THAT(Conf.Diagnostics.Suppress, IsEmpty());
}

TEST_F(ConfigCompileTests, Tidy) {
Frag.ClangTidy.Add.emplace_back("bugprone-use-after-move");
Frag.ClangTidy.Add.emplace_back("llvm-*");
Expand Down
2 changes: 1 addition & 1 deletion clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
Expand Up @@ -98,7 +98,7 @@ TEST(ParseYAML, Locations) {
YAML.range());
}

TEST(ParseYAML, Diagnostics) {
TEST(ParseYAML, ConfigDiagnostics) {
CapturedDiags Diags;
Annotations YAML(R"yaml(
If:
Expand Down
24 changes: 24 additions & 0 deletions clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
Expand Up @@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//

#include "Annotations.h"
#include "Config.h"
#include "Diagnostics.h"
#include "ParsedAST.h"
#include "Protocol.h"
Expand All @@ -16,6 +17,7 @@
#include "TestTU.h"
#include "TidyProvider.h"
#include "index/MemIndex.h"
#include "support/Context.h"
#include "support/Path.h"
#include "clang/Basic/Diagnostic.h"
#include "clang/Basic/DiagnosticSema.h"
Expand Down Expand Up @@ -371,6 +373,28 @@ TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
DiagSource(Diag::ClangTidy), DiagName("modernize-loop-convert"))));
}

TEST(DiagnosticTest, RespectsDiagnosticConfig) {
Annotations Main(R"cpp(
// error-ok
void x() {
[[unknown]]();
$ret[[return]] 42;
}
)cpp");
auto TU = TestTU::withCode(Main.code());
EXPECT_THAT(
TU.build().getDiagnostics(),
ElementsAre(Diag(Main.range(), "use of undeclared identifier 'unknown'"),
Diag(Main.range("ret"),
"void function 'x' should not return a value")));
Config Cfg;
Cfg.Diagnostics.Suppress.insert("return-type");
WithContextValue WithCfg(Config::Key, std::move(Cfg));
EXPECT_THAT(TU.build().getDiagnostics(),
ElementsAre(Diag(Main.range(),
"use of undeclared identifier 'unknown'")));
}

TEST(DiagnosticTest, ClangTidySuppressionComment) {
Annotations Main(R"cpp(
int main() {
Expand Down

0 comments on commit 9211381

Please sign in to comment.