[clangd] Support symbolTags for document symbol

[chouzz]

This patch:

• Move isConst, isStatic, isAbstract, isVirtual from SemanticHighlighting.cpp to AST.h/cpp to reuse it in FindSymbols.cpp
• Support symbolTags in document symbol(outline feature)

Fixes: clangd/clangd#2123

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 2e43a304f10fd801f068d0f9831f01f2c5b0b2e2 2d8224260143899f9af9a99465318da05d04722f --extensions cpp,h -- clang-tools-extra/clangd/AST.cpp clang-tools-extra/clangd/AST.h clang-tools-extra/clangd/FindSymbols.cpp clang-tools-extra/clangd/Protocol.cpp clang-tools-extra/clangd/Protocol.h clang-tools-extra/clangd/SemanticHighlighting.cpp

View the diff from clang-format here.

diff --git a/clang-tools-extra/clangd/Protocol.h b/clang-tools-extra/clangd/Protocol.h
index fb43439695..7bbae42b2a 100644
--- a/clang-tools-extra/clangd/Protocol.h
+++ b/clang-tools-extra/clangd/Protocol.h
@@ -1090,13 +1090,13 @@ struct CodeAction {
 };
 llvm::json::Value toJSON(const CodeAction &);
 
-enum class SymbolTag { 
+enum class SymbolTag {
   Deprecated = 1,
   Private = 2,
   Package = 3,
   Protected = 4,
   Public = 5,
-  Internal= 6,
+  Internal = 6,
   File = 7,
   Static = 8,
   Abstract = 9,
@@ -1584,7 +1584,6 @@ struct ResolveTypeHierarchyItemParams {
 bool fromJSON(const llvm::json::Value &, ResolveTypeHierarchyItemParams &,
               llvm::json::Path);
 
-
 /// The parameter of a `textDocument/prepareCallHierarchy` request.
 struct CallHierarchyPrepareParams : public TextDocumentPositionParams {};
 

[travkin79]

Hi @chouzz,

Thank you very much for your PR draft. I checked your changes and have a few questions.

Does the method AST.isConst(...) in your implementation mean an element declared with a const keyword (as intended in my LSP PR) or do you mean some variable that is read-only? If you mean read-only, I think, you should use the read-only tag instead or use both tags.

Having tags Constant and Read-only might be confusing. Maybe we should consider removing the Constant tag from LSP and only use Read-only. What do you think?

Especially for C++ code, it would be great if the tags declaration and definition would also be set. I'm sure, clangd has these details already, since it uses these modifiers e.g. for syntax highlighting. Do you think, you could also set the declaration and definition tags?

[chouzz]

@travkin79

It's extracted from semanticHighlihgting. As I can see, it means a symbol with a const keyword, but to be honest, const and read-only mostly mean the same thing in C/C++.

Having tags Constant and Read-only might be confusing. Maybe we should consider removing the Constant tag from LSP and only use Read-only. What do you think?

I agree. Also, I checked semanticHighlihgting feature, it uses Read-only modifier for const symbol, see

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

Line 1180 in 3de5dbb

Tok.addModifier(HighlightingModifier::Readonly);

Do you think, you could also set the declaration and definition tags?

OK, I will check how to add this.

[travkin79]

I thought about the Constant tag. In Java for example, there is no const keyword, instead the final keyword is used for variables to declare them as read-only. The keyword final means something else for methods (they cannot be overridden).
The keywords and their meaning seem to be very language-specific. Thus, I prefer removing the Constant tag completely from the LSP PR. I think, the Read-only tag makes much more sense, since it is more generic, and expresses better on a higher abstraction level what we mean.

Do you agree? Then I'd adapt my PR on the LSP project accordingly.

[chouzz]

I thought about the Constant tag. In Java for example, there is no const keyword, instead the final keyword is used for variables to declare them as read-only. The keyword final means something else for methods (they cannot be overridden). The keywords and their meaning seem to be very language-specific. Thus, I prefer removing the Constant tag completely from the LSP PR. I think, the Read-only tag makes much more sense, since it is more generic, and expresses better on a higher abstraction level what we mean.

Do you agree? Then I'd adapt my PR on the LSP project accordingly.

I have no objections to this.

[travkin79]

Thank you @chouzz for adding Declaration and Definition tags.

I'm sorry for answering late. I was quite busy last days.

Could we please also replace the Const tag with the Read-only tag? (I'm planning to remove the Const tag from my PR on the LSP spec).

What about an Abstract tag? Is there some clangd code that could be re-used for that? Oh, sorry, it's already covered here.

[chouzz]

@travkin79 I replaced Constant with Read-only tag and updated SymbolTag based on your changes.

[travkin79]

Thank you @chouzz,
I'll adapt the CDT LSP project (and maybe others like LSP4E and LSP4J) accordingly.

[travkin79]

Hi @chouzz,
I think, I have a first clangd LSP client prototype that I could start using for testing your modified clangd prototype. I guess, I have to build your fork on my machine, right?

It seems, there is one failing test in the CI pipeline. Could you try fixing it?

Failed Tests (1):
--
  | Clangd :: symbols.test

[chouzz]

Hi, @travkin79 that sounds great. I tried to build clangd from my github action, but I failed, maybe @HighCommander4 could help point out how to build clangd via github actions with specify branch?

It seems, there is one failing test in the CI pipeline. Could you try fixing it?

I will check.

[HighCommander4]

I tried to build clangd from my github action, but I failed, maybe @HighCommander4 could help point out how to build clangd via github actions with specify branch?

What is it that you're trying to do? The purpose of clangd's actions/workflows/autobuild.yaml is to publish new clangd releases to https://github.com/clangd/clangd/releases. It's not something I use for the purpose of local development / working on a patch.

Are you trying to publish something to https://github.com/chouzz/clangd/releases? Or just avoid building clangd / running tests locally?

[chouzz]

@HighCommander4 Just try to avoid building clangd locally.

[travkin79]

Hi @chouzz,

@HighCommander4 Just try to avoid building clangd locally.

I built your branch of llvm / clangd locally. I did the following on a linux machine

• do a shallow clone git clone -b support-symbolTags --depth 1 git@github.com:chouzz/llvm-project.git
• prepare the build for Eclipse IDE and an install path /usr/local/clangd-test/bin (I did use this path to keep the original installed clangd version unchanged) cmake -S llvm -B build -G "Eclipse CDT4 - Unix Makefiles" -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra" -DCMAKE_INSTALL_PREFIX=/usr/local/clangd-test -DCMAKE_BUILD_TYPE=Release
• build with cmake --build build --target clangd
• optionally run the tests (I skipped that) cmake --build build --target check-clangd
• optionally install cmake --build build --target install

I must admit, it takes some time to clone and especially to build the project.

[chouzz]

@travkin79
OK. If you already build it locally, that's fine. I will try to fix the test.

[travkin79]

Hi @chouzz,
I started testing your version of clangd that I built locally. It seems that clangd does not return any symbol tags. I'm checking if I missed something in LSP4J, LSP4E or somewhere else. I guess there is something missing when handling the json response and creating Java objects using JSONRPC, but I don't know yet.

At the moment, what I get for your example is the following response (tags field is always null or missing).

{
  "id": "103",
  "jsonrpc": "2.0",
  "result": [
    {
      "detail": "const int",
      "kind": 13,
      "name": "g_var",
      "range": {
        "end": {
          "character": 19,
          "line": 0
        },
        "start": {
          "character": 0,
          "line": 0
        }
      },
      "selectionRange": {
        "end": {
          "character": 15,
          "line": 0
        },
        "start": {
          "character": 10,
          "line": 0
        }
      }
    },
    {
      "children": [
        {
          "detail": "void ()",
          "kind": 6,
          "name": "pub",
          "range": {
            "end": {
              "character": 14,
              "line": 3
            },
            "start": {
              "character": 4,
              "line": 3
            }
          },
          "selectionRange": {
            "end": {
              "character": 12,
              "line": 3
            },
            "start": {
              "character": 9,
              "line": 3
            }
          }
        },
        {
          "detail": "void ()",
          "kind": 6,
          "name": "pri",
          "range": {
            "end": {
              "character": 14,
              "line": 5
            },
            "start": {
              "character": 4,
              "line": 5
            }
          },
          "selectionRange": {
            "end": {
              "character": 12,
              "line": 5
            },
            "start": {
              "character": 9,
              "line": 5
            }
          }
        },
        {
          "detail": "void ()",
          "kind": 6,
          "name": "pro",
          "range": {
            "end": {
              "character": 14,
              "line": 7
            },
            "start": {
              "character": 4,
              "line": 7
            }
          },
          "selectionRange": {
            "end": {
              "character": 12,
              "line": 7
            },
            "start": {
              "character": 9,
              "line": 7
            }
          }
        }
      ],
      "detail": "class",
      "kind": 5,
      "name": "P",
      "range": {
        "end": {
          "character": 1,
          "line": 8
        },
        "start": {
          "character": 0,
          "line": 1
        }
      },
      "selectionRange": {
        "end": {
          "character": 7,
          "line": 1
        },
        "start": {
          "character": 6,
          "line": 1
        }
      }
    }
  ]
}

[travkin79]

Hi @chouzz,
I found out that I get symbol tags if the LSP method textDocument/publishDiagnostics is used. Could you please check your implementation if the symbol tags are only created for diagnostics? If that's the case, could you please add symbol tags also for the following LSP methods and LSP types?

Methods

• textDocument.documentSymbol
• workspace/symbol
• textDocument/prepareCallHierarchy
• callHierarchy/incomingCalls
• callHierarchy/outgoingCalls
• textDocument/prepareTypeHierarchy
• typeHierarchy/supertypes
• typeHierarchy/subtypes

Types

• DocumentSymbol
• SymbolInformation
• WorkspaceSymbol
• CallHierarchyItem
• TypeHierarchyItem

@HighCommander4,
Could you please point us to the right places in clangd source code to achieve that?

[chouzz]

@travkin79
Are you using the latesed commit? It works in my local clangd with given example.

I[11:05:17.623] --> reply:textDocument/documentSymbol(11) 3 ms
V[11:05:17.624] >>> {"id":11,"jsonrpc":"2.0","result":[{"detail":"const int","kind":13,"name":"g_var","range":{"end":{"character":19,"line":0},"start":{"character":0,"line":0}},"selectionRange":{"end":{"character":15,"line":0},"start":{"character":10,"line":0}},"tags":[21,20]},{"children":[{"detail":"void ()","kind":6,"name":"pub","range":{"end":{"character":14,"line":3},"start":{"character":4,"line":3}},"selectionRange":{"end":{"character":12,"line":3},"start":{"character":9,"line":3}},"tags":[19,5]},{"detail":"void ()","kind":6,"name":"pri","range":{"end":{"character":14,"line":5},"start":{"character":4,"line":5}},"selectionRange":{"end":{"character":12,"line":5},"start":{"character":9,"line":5}},"tags":[19,2]},{"detail":"void ()","kind":6,"name":"pro","range":{"end":{"character":14,"line":7},"start":{"character":4,"line":7}},"selectionRange":{"end":{"character":12,"line":7},"start":{"character":9,"line":7}},"tags":[19,4]}],"detail":"class","kind":5,"name":"P","range":{"end":{"character":1,"line":8},"start":{"character":0,"line":1}},"selectionRange":{"end":{"character":7,"line":1},"start":{"character":6,"line":1}},"tags":[20]}]}

And I am going to update the branch to remove Constant.

Could you please check your implementation if the symbol tags are only created for diagnostics? If that's the case, could you please add symbol tags also for the following LSP methods and LSP types?

Currently, the implementation should only work in DocumentSymbol(outline). BTW, I tested in vscode, I am not sure why it also in textDocument/publishDiagnostics, could you please upload the full verbose logs of clangd? As for other features, I am going to add it after supported DocumentSymbol.

[travkin79]

Hi @chouzz,
I'm using commit 2d82242 from your branch support-symbolTags.

Your newest commit 0f6d25b only removes the Constant tag. Thus, It would only change the numbers returned as tags.

I'll check if I can provide a verbose LS log output.

Concerning textDocument/publishDiagnostics, the tags in this case seem to have always value 1 if existent, i.e. deprecation warnings. These seem to come from clang-tidy.

[travkin79]

Hi @chouzz,
I checked the verbose LS log output and it says it's using another clangd version than the one I built from your branch (it's telling me it is version 18.1.1).

I checked my config. It turns out there was some config file replacing the default clangd path with another one. Fixing that results in the expected behavior. Now, I can create an outline view like the following for your code snippet.

[travkin79]

Hi @chouzz,
I extended my prototype to create overlay icons for all Symbol Tags. I'm using another C++ code example for testing clangd's tags and my visualization. The results are promising, but I have a few questions.

• Could you please check if you can also set Final tags? It seems, clangd doesn't set these yet (see example).
• Is it correct that virtual ~AbstractClass() = default; is tagged as definition and not as declaration?

Example: demo.cpp

class AbstractClass // abstract
{
public:

  virtual ~AbstractClass() = default; // public, virtual, declaration

  virtual void f1() = 0; // public, (pure) virtual, declaration

  void f2() const; // public, const, declaration

protected:

  void f3() // protected, (declaration+)definition
  {
  }

private:

  static void f4() // private, static, (declaration+)definition
  {
  }
};

void AbstractClass::f2() const // public, const, definition
{
}

class ImplClass : public AbstractClass
{
public:

  void f1() final // public, override, final, (declaration+)definition
  {
  }
};

Outline prototype:

LS output:

{
  "id": "7",
  "jsonrpc": "2.0",
  "result": [
    {
      "children": [
        {
          "kind": 9,
          "name": "~AbstractClass",
          "range": {
            "end": {
              "character": 36,
              "line": 4
            },
            "start": {
              "character": 2,
              "line": 4
            }
          },
          "selectionRange": {
            "end": {
              "character": 11,
              "line": 4
            },
            "start": {
              "character": 10,
              "line": 4
            }
          },
          "tags": [
            15,
            19,
            5
          ]
        },
        {
          "detail": "void ()",
          "kind": 6,
          "name": "f1",
          "range": {
            "end": {
              "character": 23,
              "line": 6
            },
            "start": {
              "character": 2,
              "line": 6
            }
          },
          "selectionRange": {
            "end": {
              "character": 17,
              "line": 6
            },
            "start": {
              "character": 15,
              "line": 6
            }
          },
          "tags": [
            15,
            9,
            18,
            5
          ]
        },
        {
          "detail": "void () const",
          "kind": 6,
          "name": "f2",
          "range": {
            "end": {
              "character": 17,
              "line": 8
            },
            "start": {
              "character": 2,
              "line": 8
            }
          },
          "selectionRange": {
            "end": {
              "character": 9,
              "line": 8
            },
            "start": {
              "character": 7,
              "line": 8
            }
          },
          "tags": [
            20,
            18,
            5
          ]
        },
        {
          "detail": "void ()",
          "kind": 6,
          "name": "f3",
          "range": {
            "end": {
              "character": 3,
              "line": 14
            },
            "start": {
              "character": 2,
              "line": 12
            }
          },
          "selectionRange": {
            "end": {
              "character": 9,
              "line": 12
            },
            "start": {
              "character": 7,
              "line": 12
            }
          },
          "tags": [
            19,
            4
          ]
        },
        {
          "detail": "void ()",
          "kind": 6,
          "name": "f4",
          "range": {
            "end": {
              "character": 3,
              "line": 20
            },
            "start": {
              "character": 2,
              "line": 18
            }
          },
          "selectionRange": {
            "end": {
              "character": 16,
              "line": 18
            },
            "start": {
              "character": 14,
              "line": 18
            }
          },
          "tags": [
            8,
            19,
            2
          ]
        }
      ],
      "detail": "class",
      "kind": 5,
      "name": "AbstractClass",
      "range": {
        "end": {
          "character": 1,
          "line": 21
        },
        "start": {
          "character": 0,
          "line": 0
        }
      },
      "selectionRange": {
        "end": {
          "character": 19,
          "line": 0
        },
        "start": {
          "character": 6,
          "line": 0
        }
      },
      "tags": [
        9,
        19
      ]
    },
    {
      "detail": "void () const",
      "kind": 6,
      "name": "AbstractClass::f2",
      "range": {
        "end": {
          "character": 1,
          "line": 25
        },
        "start": {
          "character": 0,
          "line": 23
        }
      },
      "selectionRange": {
        "end": {
          "character": 22,
          "line": 23
        },
        "start": {
          "character": 20,
          "line": 23
        }
      },
      "tags": [
        20,
        19,
        5
      ]
    },
    {
      "children": [
        {
          "detail": "void ()",
          "kind": 6,
          "name": "f1",
          "range": {
            "end": {
              "character": 3,
              "line": 33
            },
            "start": {
              "character": 2,
              "line": 31
            }
          },
          "selectionRange": {
            "end": {
              "character": 9,
              "line": 31
            },
            "start": {
              "character": 7,
              "line": 31
            }
          },
          "tags": [
            15,
            19,
            5
          ]
        }
      ],
      "detail": "class",
      "kind": 5,
      "name": "ImplClass",
      "range": {
        "end": {
          "character": 1,
          "line": 34
        },
        "start": {
          "character": 0,
          "line": 27
        }
      },
      "selectionRange": {
        "end": {
          "character": 15,
          "line": 27
        },
        "start": {
          "character": 6,
          "line": 27
        }
      },
      "tags": [
        19
      ]
    }
  ]
}

[travkin79]

Hi @chouzz,
How is it going? Do you have time to go on working on this PR? It would be really great if we could finish it and get it merged. My team mates are looking forward to using the new features.