[PATCH] D81785: [clangd] Fix readability-else-after-return 'Adding a note without main diagnostic' crash

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: sammccall, hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, MaskRay, ilya-biryukov.
Herald added a project: clang.
njames93 added a comment.

The actual fix in `ElseAfterReturnCheck.cpp` is needed. 
However clangd's handling of Remarks is a little suspicious. Remarks are supposed to be different to notes in the sense they are designed to be stand alone, unlike notes which depend on a another diagnostic. see Add 'remark' diagnostic type in 'clang' <https://github.com/llvm/llvm-project/commit/741602461d2079c682916bce6701c39acb08bbd3>
This seems to be how clangd determines what a note is:

  bool isNote(DiagnosticsEngine::Level L) {
    return L == DiagnosticsEngine::Note || L == DiagnosticsEngine::Remark;
  }


Fix a crash in clangd caused by an (admittidly incorrect) Remark diagnositic being emitted from readability-else-after-return.
This crash doesn't occur in clang-tidy so there are no tests there for this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D81785

Files:
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -299,6 +299,25 @@
           DiagSource(Diag::ClangTidy), DiagName("modernize-loop-convert"))));
 }
 
+TEST(DiagnosticTest, NoAddingANoteWithoutMainDiagnostic) {
+  Annotations Main(R"cc(
+    void foo() {
+      if (int X = 0) {
+        return;
+      } [[else]] {
+        X++;
+      }
+    }
+  )cc");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "readability-else-after-return";
+  EXPECT_THAT(TU.build().getDiagnostics(),
+              UnorderedElementsAre(::testing::AllOf(
+                  Diag(Main.range(), "do not use 'else' after 'return'"),
+                  DiagSource(Diag::ClangTidy),
+                  DiagName("readability-else-after-return"))));
+}
+
 TEST(DiagnosticTest, ClangTidySuppressionComment) {
   Annotations Main(R"cpp(
     int main() {
Index: clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
@@ -188,9 +188,8 @@
     if (IsLastInScope) {
       // If the if statement is the last statement its enclosing statements
       // scope, we can pull the decl out of the if statement.
-      DiagnosticBuilder Diag =
-          diag(ElseLoc, WarningMessage, clang::DiagnosticIDs::Level::Remark)
-          << ControlFlowInterruptor;
+      DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
+                               << ControlFlowInterruptor;
       if (checkInitDeclUsageInElse(If) != nullptr) {
         Diag << tooling::fixit::createReplacement(
                     SourceRange(If->getIfLoc()),


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D81785.270569.patch
Type: text/x-patch
Size: 1945 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200613/f62ada82/attachment.bin>