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

[njames93]

Summary

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.

[njames93]

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'
This seems to be how clangd determines what a note is:

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

[hokein]

Thanks for spotting it!

In D81785#2091352, @njames93 wrote:

The actual fix in ElseAfterReturnCheck.cpp is needed.

yeah, the fix in clang-tidy check seems reasonable to me, this aligns the general practice. In general, we use clang::warning to emit clang-tidy check warnings.

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'
This seems to be how clangd determines what a note is:

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

yeah, I think you're right, we should fix that in clangd as well. We might not hit this in practice -- clangd runs syntax-only action, and Remark diagnostics seems to be emitted from clang optimization phrase (which is not executed in clangd).

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

191–192

this would change the output of the check, I suppose this is not covered in readability-else-after-return.cpp lit test (that test only tests warning messages), could you add a test there? then we don't need a test case in clangd.

[njames93]

• Remove clangd test case

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

191–192

For some reason, I don't know why, it doesn't appear to. I changed all diags in the check to use remark, yet the outputs all said warning. not sure why though. There is a test case already for this specific case in the check that looks for warning. maybe I should just remove the clangd test case though, its not terribly important for this.

[hokein]

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

191–192

thanks.

It seems that clang-tidy doesn't fully respect all diagnostic levels, it just supports Warning, and Error, see https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L411.

clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp

if (!IsLastInScope && containsDeclInScope(Else)) {
     }
     return;
   }
 
   if (checkConditionVarUsageInElse(If) != nullptr) {
     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)
+      DiagnosticBuilder Diag = diag(ElseLoc, WarningMessage)
                                << ControlFlowInterruptor;
       if (checkInitDeclUsageInElse(If) != nullptr) {
         Diag << tooling::fixit::createReplacement(
                     SourceRange(If->getIfLoc()),
                     (tooling::fixit::getText(*If->getInit(), *Result.Context) +
                      llvm::StringRef("\n"))
                         .str())
              << tooling::fixit::createRemoval(If->getInit()->getSourceRange());
       }