[clangd] Change the url for clang-tidy check documentation

[njames93]

Summary

In https://github.com/llvm/llvm-project/commit/6e566bc5523f743bc34a7e26f050f1f2b4d699a8, The directory structure of the documentation for clang-tidy checks was changed, however clangd wasn't updated.
Now all the links generated will point to old dead pages.
This updated clangd to use the new page structure.

[sammccall]

Thanks!

clang-tools-extra/clangd/Diagnostics.cpp

927

I don't think we should assert on this, how clang-tidy checks are named isn't something clangd can reasonably control.

Instead I'd suggest only returning a value if the condition is met.

[njames93]

clang-tools-extra/clangd/Diagnostics.cpp

927

The logic behind the assert was that it wouldn't cause any issues on release, but if someone added a check not to the convention it would break debug builds.
But it's not an issue that's expected anyway so can be safely removed.

[kadircet]

luckily this document links were introduced only recently, hence they didn't make it to clangd-14, but they'll start being around from clangd-15 and such changes will be breaking all the links in existing clangd's.
i wonder if we should actually point these at releases.llvm.org/CLANG_VERSION/... instead of top of the head:

• downside, we don't have a release until it's there, so everything will be broken with a non-released clangd.

i suppose we can make use of different urls based on build configuration, but i couldn't find any existing mechanisms for doing so. hence this needs to be introduced.
another option would be to have a redirection from releases.llvm.org/15.0.0 to clang.llvm.org, until it's released, but the directory structure/subdomain is different so this will also need some adjustments.

the other option is, when we do such changes, we introduce redirections from old page schemes so that those links keep working, at least for a couple releases.

I don't think such changes will occur quite often (or ever again) in practice, so this is probably not worth the effort. but if any of these redirections are actually easy to add, we might consider doing so. as in addition to preventing breakages through such changes, it'll also make sure people are seeing the documentation for the check as it was released with clangd.

[njames93]

Pointing the documentation links based on build configuration and version definitely makes sense as evolving checks could easily confuse users.
Guess the idea is snapshot builds will use the in progress release notes, and release builds can point to the actual release documentation

[njames93]

Emit documentation links for the version of clangd built.

[njames93]

Fix lit cfg typo

[njames93]

Any issues with this now for getting correct version?

[sammccall]

Hmm, this version looks complicated to me.
And also fragile: downstream we have CLANG_VERSION_STRINGs that don't match upstream, Apple has their own versioning scheme, linux distros tend to do things like 6.0.1~ubuntu3...
Let me sync with @kadircet

[sammccall]

In D128379#3622128, @sammccall wrote:

Hmm, this version looks complicated to me.
And also fragile: downstream we have CLANG_VERSION_STRINGs that don't match upstream, Apple has their own versioning scheme, linux distros tend to do things like 6.0.1~ubuntu3...
Let me sync with @kadircet

The good news is that the ~ubuntu3 isn't part of CLANG_VERSION_STRING I think.
Bad news #1 is that it still may not match llvm.org versions: e.g. our internal distribution is "trunk", Apple's CLANG_VERSION_STRING is 10.0.1 on my machine, but it's approximately LLVM version 7.
Bad news #2 is that the documentation isn't actually available for all these versions: none of 14.0.1->14.0.5 exist, the point releases for 9-13 all have documentation but not 8.0.1. Looking at other projects, the set of docs available is inconsistent.

I don't think this substantially more reliable than just pointing at the HEAD docs, and it certainly doesn't seem "better enough" to be worth any build complexity. Can we revert to the simple version?

(I do think changing the URLs of the clang-tidy check documentation was unfortunate, and setting up server-side redirects for those would be nice to have if it's easy)

[njames93]

In D128379#3622286, @sammccall wrote:

In D128379#3622128, @sammccall wrote:

Hmm, this version looks complicated to me.
And also fragile: downstream we have CLANG_VERSION_STRINGs that don't match upstream, Apple has their own versioning scheme, linux distros tend to do things like 6.0.1~ubuntu3...
Let me sync with @kadircet

The good news is that the ~ubuntu3 isn't part of CLANG_VERSION_STRING I think.
Bad news #1 is that it still may not match llvm.org versions: e.g. our internal distribution is "trunk", Apple's CLANG_VERSION_STRING is 10.0.1 on my machine, but it's approximately LLVM version 7.
Bad news #2 is that the documentation isn't actually available for all these versions: none of 14.0.1->14.0.5 exist, the point releases for 9-13 all have documentation but not 8.0.1. Looking at other projects, the set of docs available is inconsistent.

I don't think this substantially more reliable than just pointing at the HEAD docs, and it certainly doesn't seem "better enough" to be worth any build complexity. Can we revert to the simple version?

(I do think changing the URLs of the clang-tidy check documentation was unfortunate, and setting up server-side redirects for those would be nice to have if it's easy)

That's a good point about the point releases. The best acceptable compromise would be to just point the docs to the <MAJOR_VERSION>.0.0. This should always be a valid
The only issue here is that if there was any issue relating to the documentation URL it would surface after we have already published the release.

I did raise the issue about redirects for the check documentation when the structure was changed but it wasn't implemented.

[njames93]

Changed to only use the major version.
If this still seems too flaky, happy to go back to just linking to the in progress release notes.

[njames93]

What's the plan with this, a fix wouod likely need to be backportsd for this.

[kadircet]

sorry this has slipped through. since we're going to backport it, let's make it minimal and always point to in-progress release notes (so sorry for reverting back to original version of the patch).

i think having clangd point to versioned docs would be much nicer, but it seems like we don't really have precise release guarantees for docs, especially in point releases. so i think we should first figure that story out, before going down that path (i don't think not pointing to exact version is a win, we might still be stale due to backports and there isn't much value in it).

[njames93]

Revert to first version, just so we can sort out the 15.X branch.

[kadircet]

thanks a lot, lgtm!

clang-tools-extra/clangd/Diagnostics.cpp

llvm::Optional<std::string> getDiagnosticDocURI(Diag::DiagSource Source,
   switch (Source) {
   case Diag::Unknown:
     break;
   case Diag::Clang:
     // There is a page listing many warning flags, but it provides too little
     // information to be worth linking.
     // https://clang.llvm.org/docs/DiagnosticsReference.html
     break;
-  case Diag::ClangTidy:
-    return {("https://clang.llvm.org/extra/clang-tidy/checks/" + Name + ".html")
-                .str()};
+  case Diag::ClangTidy: {
+    StringRef Module, Check;
+    // This won't correctly get the module for clang-analyzer checks, but as we
+    // don't link in the analyzer that shouldn't be an issue.
+    // This would also need updating if anyone decides to create a module with a
+    // '-' in the name.
+    std::tie(Module, Check) = Name.split('-');
+    if (Module.empty() || Check.empty())
+      return llvm::None;
+    return ("https://clang.llvm.org/extra/clang-tidy/checks/" + Module + "/" +
+            Check + ".html")
+        .str();
+  }
   case Diag::Clangd:
     if (Name == "unused-includes")
       return {"https://clangd.llvm.org/guides/include-cleaner"};
     break;
   case Diag::ClangdConfig:
     // FIXME: we should link to https://clangd.llvm.org/config
     // However we have no diagnostic codes, which the link should describe!
     break;
   }
   return llvm::None;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/test/diagnostics-tidy.test

 # REQUIRES: clangd-tidy-checks
 # RUN: clangd -lit-test -clang-tidy-checks=bugprone-sizeof-expression < %s | FileCheck -strict-whitespace %s
 {"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
 ---
 {"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///foo.c","languageId":"c","text":"int main() {\n(void)sizeof(42);\n}"}}}
 #      CHECK:  "method": "textDocument/publishDiagnostics",
 # CHECK-NEXT:  "params": {
 # CHECK-NEXT:    "diagnostics": [
 # CHECK-NEXT:      {
 # CHECK-NEXT:        "code": "bugprone-sizeof-expression",
 # CHECK-NEXT:        "codeDescription": {
-# CHECK-NEXT:          "href": "https://clang.llvm.org/extra/clang-tidy/checks/bugprone-sizeof-expression.html"
+# CHECK-NEXT:          "href": "https://clang.llvm.org/extra/clang-tidy/checks/bugprone/sizeof-expression.html"
 # CHECK-NEXT:        },
 # CHECK-NEXT:        "message": "Suspicious usage of 'sizeof(K)'; did you mean 'K'?",
 # CHECK-NEXT:        "range": {
 # CHECK-NEXT:          "end": {
 # CHECK-NEXT:            "character": 12,
 # CHECK-NEXT:            "line": 1
 # CHECK-NEXT:          },
 # CHECK-NEXT:          "start": {