Skip to content

Conversation

@travkin79
Copy link
Contributor

@travkin79 travkin79 commented Nov 20, 2024

Adds new symbol tags as proposed in my PR on the LSP specification, for example, adding private, package, protected, and public visibility tags as well as tags like static, final, abstract, read-only, nullable and non-null. Our motivation comes from our wish to add visibility and other symbol details to the outline view (see discussion eclipse-lsp4e/lsp4e#977), but maybe also to the call hierarchy and to the type hierarchy or to other related features / LSP operations.

The PR on the LSP specification requires at least one implementation in a language server and / or a client. We plan to finish a [first language server implementation in clangd](microsoft/language-server-protocol#2003
and llvm/llvm-project#113669) and a first client implementation in LSP4J, LSP4E, and CDT LSP (which is using clangd).

See microsoft/language-server-protocol#2003
and llvm/llvm-project#167536
and eclipse-lsp4e/lsp4e#1149

@jonahgraham
Copy link
Contributor

@travkin79 your use case is compelling. AFAIK LSP4J hasn't been the first implementation for an LSP feature before. LSP4J has had pre-released features though. It sounds like we have a bit of a chicken-and-egg situation here, and I don't want LSP4J to be what prevents moving us forward.

For pre-release we marked all elements that were not in the official spec with @Beta (com.google.common.annotations.Beta). Lets do that here too and then we can move towards a release with this change in it.

Nothing in the current HEAD has @Beta - but see an old example part of #133 if you want a reference:

Having the @Beta means we can change around those items easier without as much consideration for API consumers of LSP4J.

@travkin79
Copy link
Contributor Author

Hi @jonahgraham,
Thank you for the hint to the @Beta annotations. I think, I've seen them already somewhere. I'll add them and I think, they are very helpful for explicitly documenting API that is likely to change in near future.

Concerning the chicken-and-egg situation: I think, for getting my LSP specification proposal accepted, we need the following steps in the following order.

  1. finish the first clangd language server implementation using the new symbol tags ([clangd] Support symbolTags for document symbol llvm/llvm-project#113669)
  2. finish first language server client implementation that uses the new symbol tags
    i. Extend LSP4J to support the new symbol tags (Add symbol tags as proposed for LSP specification (e.g. visibility tags, static, abstract, etc.) #856)
    ii. Extend LSP4E to use the new symbol tags, e.g. in the outline view (Add symbol tags as proposed for LSP specification (e.g. visibility tags, static, abstract, etc.) #977 eclipse-lsp4e/lsp4e#1149 and Outline view icons are missing visibility / token modifier details - How can we change that? eclipse-lsp4e/lsp4e#977)
  3. Hope for the LSP specification PR to be accepted ([#98] Add SymbolTag values for access modifiers and other modifiers microsoft/language-server-protocol#2003)

@travkin79 travkin79 force-pushed the feature/new-symbol-tags branch from 91b187f to 62b1364 Compare December 6, 2024 09:51
@travkin79 travkin79 force-pushed the feature/new-symbol-tags branch 3 times, most recently from 6a09dc8 to e32a6a9 Compare November 24, 2025 13:14
@travkin79 travkin79 marked this pull request as ready for review November 24, 2025 13:20
@travkin79
Copy link
Contributor Author

Hi @jonahgraham,
It took some time, but now, we can go on with this PR and related PRs, since a first language server implementation for this change is now finished (implemented in clangd and the PR is ready to be merged). Could you please take a look at this PR?

I've used the @Beta annotations for the new symbol tags that are proposed to the LSP specification, but are not accepted yet. I've seen the @ProtocolDraft. Could that be a replacement for the @Beta annotations?

Reminder: For the LSP specification proposal to be accepted, we have to provide an exemplary implementation in one language server and one client. We chose clangd, LSP4J, LSP4E (and CDT LSP) for that.

Related work:

@github-actions
Copy link

github-actions bot commented Nov 24, 2025

Test Results

  500 files  ±0    500 suites  ±0   35s ⏱️ -3s
  359 tests ±0    359 ✅ ±0  0 💤 ±0  0 ❌ ±0 
4 052 runs  ±0  4 052 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit d66fd19. ± Comparison against base commit b5a8adf.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM - and is similar in scope to #893 (although more provisional than 3.18 of the spec, hopefully this can be done by the time 3.18 is complete)

Can you please add an entry to the Changelog and readme. Here is some suggested ideas.

Changelog:

diff --git a/CHANGELOG.md b/CHANGELOG.md
index e17bad6c..a5ffdd88 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,8 @@
 
 ### v1.0.0 (TBD)
 
+* Implemented [LSP proposal](https://github.com/microsoft/language-server-protocol/pull/2003) for `SymbolTag` [#856](https://github.com/eclipse-lsp4j/lsp4j/pull/856)
+
 Fixed issues: <https://github.com/eclipse-lsp4j/lsp4j/milestone/37?closed=1>
 
 * Removed `@Deprecated` annotations on members deprecated in the LSP/DAP protocol [#895](https://github.com/eclipse-lsp4j/lsp4j/issues/895)

Readme:

diff --git a/README.md b/README.md
index 8f229030..d712b5c4 100644
--- a/README.md
+++ b/README.md
@@ -35,7 +35,7 @@ The Maven Repositories, p2 Update Sites, and the Snapshots contain _signed jars_
 
 ### Supported LSP Versions
 
- * LSP4J 1.0.&ast; _(Next release)_ &rarr; LSP 3.17.0
+ * LSP4J 1.0.&ast; _(Next release)_ &rarr; LSP 3.17.0 (plus [SymbolTag proposal](https://github.com/microsoft/language-server-protocol/pull/2003))
  * LSP4J 0.24.&ast; &rarr; LSP 3.17.0
  * LSP4J 0.23.&ast; &rarr; LSP 3.17.0
  * LSP4J 0.22.&ast; &rarr; LSP 3.17.0

@travkin79 travkin79 force-pushed the feature/new-symbol-tags branch from e32a6a9 to d66fd19 Compare November 24, 2025 17:46
@jonahgraham jonahgraham added this to the 1.0.0 milestone Nov 24, 2025
@jonahgraham jonahgraham merged commit 91346fc into eclipse-lsp4j:main Nov 24, 2025
7 checks passed
@jonahgraham
Copy link
Contributor

@travkin79 This has been merged and is available in maven snapshots + nightly builds, the p2 repo is:

https://download.eclipse.org/lsp4j/builds/main/ (the heavy caching of download.e.o may mean this takes a little while to consistently show the updated version)

You should be able to update this line of your LSP4E PR to point to the above to get it to build. You'll also have to update the version ranges in the various MANIFEST.MFs:

As for the release date of 1.0.0, it is not set yet, but follow #871 and weigh in on your desires and needs.

@travkin79
Copy link
Contributor Author

Hi @jonahgraham,
Thank you very much for the review and your helpful hints. I'll update my LSP4E PR soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants