Limit source range of "do not use 'else' after 'return'" diagnostic to just the 'else' keyword

[Quuxplusone]

Bugzilla Link	PR47809
Status	REOPENED
Importance	P enhancement
Reported by	Nathan Ridge (zeratul976@hotmail.com)
Reported on	2020-10-12 14:08:04 -0700
Last modified on	2020-11-28 16:39:58 -0800
Version	unspecified
Hardware	PC Linux
CC	alexfh@google.com, djasper@google.com, klimek@google.com, N.James93@hotmail.co.uk
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

Currently, the source range of the "do not use 'else' after 'return'" diagnostic is the entire "else" branch, which means the editor will highlight multiple lines (or even a significant part of a page) with a yellow squiggly over this minor stylistic nit.

It would be much better to restrict the source range of this diagnostic to just the "else" keyword.

[Quuxplusone]

The source range of the diagnostic is literally just the else location. However
I can see you are referring to how in clangd workflows it likes to indicate the
entirety of the else branch for the error. This is the result of a little hack
to get clang-tidy to reformat the entire else block range when it removes the
else branch. However, it results in needing to essentially create a fix-it for
the whole else branch.
Ironically in clangd there isn't even that benefit as it wont run clang-
format(right now) when applying fix-it diagnostics, so you get the needlessly
long error without the formatting that comes with it.

What would be much nicer is an interface, likely in FixItHint, that enables
clang-tidy(and other tools) to specify a region of code that will need
reformatting once a fix-it is applied.

[Quuxplusone]

I re-tested this with a more recent LLVM trunk and the long squiggly is gone.

It's not clear to me whether something on the clangd side or clang-tidy side has changed to resolve this.

[Quuxplusone]

I've tried to figure out what fixed this, but haven't been able to. The source range of the diagnostic itself is indeed just the "else" location, and information about the quick fix is not sent together with the diagnostic (only later, when you click on it, via a separate codeAction request).

I'm just going to close this, and reopen if I come across it again.

[Quuxplusone]

Never mind, it's still happening, it just requires an "else if" rather than a
plain "else". Reopening.

Specifically, for this code:

int foo(int cond) {
  if (cond == 1) {
    return 42;
  } else if (cond == 2) {
    return 43;
  }
  return 44;
}

clangd sends the following diagnostics:

{
  "jsonrpc": "2.0",
  "method": "textDocument/publishDiagnostics",
  "params": {
    "diagnostics": [
      {
        "code": "llvm-else-after-return",
        "message": "Do not use 'else' after 'return' (fix available)",
        "range": {
          "end": {
            "character": 3,
            "line": 5
          },
          "start": {
            "character": 4,
            "line": 3
          }
        },
        "relatedInformation": [],
        "severity": 2,
        "source": "clang-tidy"
      }
    ],
    "uri": "<redacted>",
    "version": 1
  }
}

note the diagnostic range starts on line 3 and ends on line 5.

[Quuxplusone]

Clangd does do some processing [1] to come up with the range to send to LSP, so the multi-line range could be introduced there.

[1] https://searchfox.org/llvm/rev/e8dc17a2b7710a2f055220a9d5cca68817787736/clang-tools-extra/clangd/Diagnostics.cpp#98

[Quuxplusone]

The processing that causes this is here https://github.com/llvm/llvm-project/blob/2d9097a06aad8cc1c272395756d22be2e849f40b/clang-tools-extra/clangd/Diagnostics.cpp#L109

Maybe if in the check we add a range to the diagnostic of just the else( if)? that would solve this issue

[Quuxplusone]

(In reply to Nathan James from comment #6)
> Maybe if in the check we add a range to the diagnostic of just the else(
> if)? that would solve this issue

Sounds like that should work, yeah.