[clangd] don't lower severity of clang-tidy modernize-* checks to remarks

[5chmidti]

Starting with a59b24b, the severity of
diagnostics that have the 'Deprecated' tag is lowered to 'Remark'.
Because the Deprecated tag is applied to clang-tidy checks in
the modernize category, this leads to inconsistencies in reported
clang-tidy diagnostics.

[llvmbot]

@llvm/pr-subscribers-clangd

Author: Julian Schmidt (5chmidti)

Changes

Starting with a59b24b, the severity of
diagnostics that have the 'Deprecated' tag is lowered to 'Remark'.
Because the Deprecated tag is applied to clang-tidy checks in
the modernize category, this leads to inconsistencies in reported
clang-tidy diagnostics.

---

Full diff: https://github.com/llvm/llvm-project/pull/75706.diff

3 Files Affected:

• (modified) clang-tools-extra/clangd/Diagnostics.cpp (+4-2)
• (modified) clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp (+12-2)
• (modified) clang-tools-extra/docs/ReleaseNotes.rst (+3)

diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp
index 704e61b1e4dd79..a56d450fb44da1 100644
--- a/clang-tools-extra/clangd/Diagnostics.cpp
+++ b/clang-tools-extra/clangd/Diagnostics.cpp
@@ -474,8 +474,10 @@ void toLSPDiags(
   // We downgrade severity for certain noisy warnings, like deprecated
   // declartions. These already have visible decorations inside the editor and
   // most users find the extra clutter in the UI (gutter, minimap, diagnostics
-  // views) overwhelming.
-  if (D.Severity == DiagnosticsEngine::Warning) {
+  // views) overwhelming. Because clang-tidy's modernize checks get the
+  // `Deprecated` tag, guard against lowering clang-tidy diagnostic levels.
+  if (D.Severity == DiagnosticsEngine::Warning &&
+      D.Source != Diag::DiagSource::ClangTidy) {
     if (llvm::is_contained(D.Tags, DiagnosticTag::Deprecated))
       Main.severity = getSeverity(DiagnosticsEngine::Remark);
   }
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 37643e5afa2304..b4f7cb71f15e57 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1811,10 +1811,10 @@ TEST(ParsedASTTest, ModuleSawDiag) {
   TestTU TU;
 
   auto AST = TU.build();
-        #if 0
+#if 0
   EXPECT_THAT(AST.getDiagnostics(),
               testing::Contains(Diag(Code.range(), KDiagMsg.str())));
-        #endif
+#endif
 }
 
 TEST(Preamble, EndsOnNonEmptyLine) {
@@ -1882,6 +1882,16 @@ TEST(Diagnostics, DeprecatedDiagsAreHints) {
   EXPECT_EQ(Diag->severity, getSeverity(DiagnosticsEngine::Remark));
   Diag.reset();
 
+  // Don't downgrade warnings from clang-tidy that have deprecated tags
+  D.Source = Diag::DiagSource::ClangTidy;
+  toLSPDiags(D, {}, Opts,
+             [&](clangd::Diagnostic LSPDiag, ArrayRef<clangd::Fix>) {
+               Diag = std::move(LSPDiag);
+             });
+  EXPECT_EQ(Diag->severity, getSeverity(DiagnosticsEngine::Warning));
+  D.Source = Diag::DiagSource::Unknown;
+  Diag.reset();
+
   // Preserve errors.
   D.Severity = DiagnosticsEngine::Error;
   toLSPDiags(D, {}, Opts,
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 6d91748e4cef18..2d8d57a535e4a8 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -54,6 +54,9 @@ Inlay hints
 Diagnostics
 ^^^^^^^^^^^
 
+- Fixed a bug where the severity of diagnostics from clang-tidy modernize-* checks were
+  lowered to remarks.
+
 Semantic Highlighting
 ^^^^^^^^^^^^^^^^^^^^^
 

[5chmidti]

Explicitly pinging @kadircet and @sam-mccall for your involvement in the mentioned commit. What are your thoughts on this? I looked into this because someone on discord asked about this difference.

[5chmidti]

The tag for modernize-* checks gets added here:

llvm-project/clang-tools-extra/clangd/Diagnostics.cpp

Lines 392 to 397 in f49e2b0

	if (D.Source == Diag::ClangTidy) {
	if (llvm::StringRef(D.Name).starts_with("misc-unused-"))
	D.Tags.push_back(DiagnosticTag::Unnecessary);
	if (llvm::StringRef(D.Name).starts_with("modernize-"))
	D.Tags.push_back(DiagnosticTag::Deprecated);
	}

[5chmidti]

This formatting change slipped through, should I remove it?

[HighCommander4]

Explicitly pinging @kadircet and @sam-mccall for your involvement in the mentioned commit. What are your thoughts on this? I looked into this because someone on discord asked about this difference.

Added Sam and Kadir as reviewers.

This patch seems to contradict the explicit motivation of the previous one expressed in this code comment:

  // We downgrade severity for certain noisy warnings, like deprecated
  // declartions. These already have visible decorations inside the editor and
  // most users find the extra clutter in the UI (gutter, minimap, diagnostics
  // views) overwhelming.

I think clang-tidy "modernize" warnings can definitely be noisy in this way.

That said, we've also received feedback that the strike-through style that some editors apply for the Deprecated tag is even more distracting than the underline (clangd/vscode-clangd#482).

[kadircet]

hi @5chmidti and @HighCommander4!

it was deliberate to cover modernize- checks as they're pretty similar to -wdeprecated in nature, and they're both explicit, user needs to annotate code or opt-in for particular clang-tidy checks.

feedback we received in the past was, these kind of findings are hard to surface during builds, due to sheer amount of such findings in an existing code base. surfacing these findings on editing/review workflows OTOH is quite desirable to decrease the amount of new violations, but it can easily clutter any aggregate finding views (e.g. VSCode's problems panel).

Hence we went with emitting these as Hints with Remarks tags. So that editors have some signals to prevent such cluttering, while still displaying these findings, especially near the cursor where new code usually lives.

That being said, what exactly is the motivation for this patch? You mentioned inconsistencies, is it inside clangd or in other places that surface clang-tidy findings?

[HighCommander4]

You mentioned inconsistencies, is it inside clangd or in other places that surface clang-tidy findings?

I understood it as, the appearance of modernize-* clang-tidy diagnostics in the editor is not consistent with the appearance of other (non-modernize-*) clang-tidy diagnostics.

(I don't have a strong opinion on this change, just providing clarification.)

[5chmidti]

There is also the use-case to keep a relatively modern code-base in a modern state. In that case, the user might want to know about modernizations/deprecations right in the editor.
Example (vscode):

'After removing the unused variable, I have no more problems/diagnostics', but the for-loop could be a range-for, and the
function can use a trailing return type.

The mentioned 'inconsistency' is that vscode will not tell you about remarks as problems (right side of the Problems tab):

and the only way to find the diagnostics modernize- or deprecated is to find code with strike-throughs.

Maybe a configuration option would be better?

Diagnostics:
  ModernizationDiagnosticSeverity: <>
  [W]DeprecatedDiagnosticSeverity: <>

or

Diagnostics:
  Severity:
    Modernize: <>
    Deprecated: <>

Or change the severity from Remark to Note (getSeverity shows that Note has a higher severity than Remark, which differs from DiagnosticEngine::Level/DiagnosticIDs), which will show up more visible and as a distinct entry in Problems and can be filtered by (screenshot above). The inline highlighting is kept even when filtering in the Problems tab.

This pr is probably not the way to do this, my personal take is that the configuration option is the most flexible. I guess an issue would have been a better fit to talk about this :)

[marzojr]

Coming from the issue I posted on vscode-clangd, I think it was a bad decision to make modernize-* checks basically disappear from the GUI. I feel less strongly with regards to misc-unused-* checks, but it is probably also true for those.

While trying to remove clutter is a good thing, I think this was done in the wrong way: there is no way to opt into the clutter if one wants to (which, one might argue, the user already has opted into when they enabled those checks in clang-tidy in the first place). As is, these warnings become nearly invisible, to the point of being useless -- if you want to address them, you either have to manually run clang-tidy from the command line, or go file by file scanning for strike-through text (which, in some cases, can be a single character wide), hovering over with the mouse to see if you want to fix it.

[5chmidti]

I'm closing this because it is not a good way to solve this. IMO, a good approach would be to let users write the diagnostic severity they want to have for all or for specific diagnostics groups in their clangd config file (like described above). I've opened clangd/clangd#1937 to discuss how to move forward with a better plan.