Skip to content

Fix LSP documentSymbol marking every symbol as deprecated#4531

Merged
doriable merged 3 commits into
bufbuild:mainfrom
garrettr:fix-lsp-document-symbol-deprecated-tag
May 16, 2026
Merged

Fix LSP documentSymbol marking every symbol as deprecated#4531
doriable merged 3 commits into
bufbuild:mainfrom
garrettr:fix-lsp-document-symbol-deprecated-tag

Conversation

@garrettr
Copy link
Copy Markdown
Contributor

@garrettr garrettr commented May 7, 2026

What

textDocument/documentSymbol responses unconditionally set Tags to [SymbolTagDeprecated] for every symbol. Per the LSP spec, clients prefer tags over the legacy deprecated boolean, so the unconditional tag won and every symbol in every .proto file rendered with a strikethrough in editors that honor symbol tags — in VS Code that's the Outline panel, breadcrumbs, sticky scroll, and Go to Symbol (Cmd+Shift+O).

This change only includes SymbolTagDeprecated in Tags when the symbol is actually deprecated.

It also fixes the deprecation check itself: it previously discarded the value returned by AsBool() and only checked the ok return, so an explicit option deprecated = false; would also be reported as deprecated. Use the returned boolean value directly.

Why

Introduced in #4311, first released in v1.65.0. Reproduction and full details in #4530.

Fixes #4530

DocumentSymbol responses unconditionally set Tags to
[SymbolTagDeprecated], so every symbol in every .proto file rendered
with a strikethrough in editors that honor symbol tags (VS Code's
Outline panel, breadcrumbs, sticky scroll, and Go to Symbol). Per the
LSP spec, clients prefer Tags over the legacy Deprecated boolean, so
the unconditional tag won even when Deprecated was false.

Only include SymbolTagDeprecated in Tags when the symbol is actually
deprecated.

Also fix the deprecation check itself: it previously discarded the
value returned by AsBool() and only checked the ok return, treating an
explicit `option deprecated = false;` as deprecated. Use the boolean
value directly instead.

Fixes bufbuild#4530
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 7, 2026

CLA assistant check
All committers have signed the CLA.

@doriable doriable self-requested a review May 7, 2026 22:16
@doriable
Copy link
Copy Markdown
Member

doriable commented May 7, 2026

This fix looks good, thank you for catching and submitting this!
A quick comment: would you mind adding a regression test case for this in diagnostic_test.go? As an aside, I'll look into the CVE's, it seems we'll need to upgrade our go version to address that, I can handle that from our end.

(EDIT: a PR has been opened to upgrade the Go version #4532)

Extend TestDocumentSymbol to assert on the Tags field of every returned
symbol, not just the legacy Deprecated boolean. Tags must only carry
SymbolTagDeprecated for symbols that are actually deprecated; the
existing test passed against the buggy unconditional-tag code because
it never inspected Tags.

Also add an ExplicitlyNotDeprecated message with
`option deprecated = false;` to the test fixture so both the unset and
explicitly-false cases are pinned down.
@garrettr
Copy link
Copy Markdown
Contributor Author

garrettr commented May 8, 2026

Thanks for the review! Added a regression test in document_symbol_test.go (the existing textDocument/documentSymbol test — there isn't a diagnostic_test.go and the bug is in the documentSymbol path rather than diagnostics, so I extended the test that was already exercising it).

The existing test already had a deprecated LegacyDocument case but only asserted on the legacy Deprecated boolean, not Tags, which is why it didn't catch this. The test now asserts Tags for every symbol, and I added an ExplicitlyNotDeprecated message with option deprecated = false; to pin down both the unset and explicitly-false cases.

Verified the new assertions fail against the pre-fix symbol.go ("symbol X has wrong tags" for every non-deprecated symbol) and pass with the fix.

@garrettr
Copy link
Copy Markdown
Contributor Author

garrettr commented May 8, 2026

I'm still waiting for my employer to approve the CLA. I will sign it (or close the PR without signing) when I hear back from them.

@garrettr
Copy link
Copy Markdown
Contributor Author

Just signed the CLA after getting permission from my company's legal department. @doriable Would you please take another look?

Copy link
Copy Markdown
Member

@doriable doriable left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for pushing up the fix! I'll merge for you and this will be available on the next release.

@doriable doriable merged commit 2dae477 into bufbuild:main May 16, 2026
9 checks passed
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.

LSP documentSymbol marks every symbol with SymbolTag.Deprecated, regardless of whether it is deprecated

3 participants