-
Notifications
You must be signed in to change notification settings - Fork 758
Implement support for Code Lenses #2145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thankfully this is opt-in, otherwise I'd be even more worried about merging this given this also uses the checker. |
| return ls.ProvideCodeLenses(ctx, params.TextDocument.Uri) | ||
| } | ||
|
|
||
| func (s *Server) handleCodeLensResolve(ctx context.Context, codeLens *lsproto.CodeLens, reqMsg *lsproto.RequestMessage) (*lsproto.CodeLens, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is likely to conflict with Sheetal's PR, but probably in a good way given the codelens needs to do cross-project too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm noticing that that PR (#1991) doesn't touch go-to-implementations, which in theory is also multi-project. I think that that should be a follow-up to this so the two have a common way to walk references across related projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that part as a separate PR but strada didn’t do it so I didn’t port it in my earlier pr
…ly namespaced command in the extension.
|
|
||
| type provideImplementationsOpts struct { | ||
| // Force the result to be Location objects. | ||
| requireLocationsResult bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of any options bag, I could just set up a new context for this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements comprehensive Code Lens support for the TypeScript language server, adding both "references" and "implementations" code lenses with user-configurable visibility settings. The implementation follows the LSP specification's two-phase approach: textDocument/codeLens provides unresolved lenses that are later populated via codeLens/resolve.
Key Changes:
- Server-side code lens provider with configurable user preferences for references and implementations
- Support for
includeDeclarationparameter in find-all-references to enable filtering declarations from reference counts - Client-side VS Code command integration to display locations in the editor's peek view
- Comprehensive test infrastructure with baseline snapshot testing for various code lens scenarios
Reviewed changes
Copilot reviewed 72 out of 72 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/ls/codelens.go |
New file implementing code lens provider with node traversal logic, resolution handling, and visibility rules |
internal/ls/findallreferences.go |
Extended with includeDeclarations support, declaration filtering logic, and provideImplementationsEx with options |
internal/ls/lsutil/userpreferences.go |
Added code lens preferences for enabled/disabled state and visibility controls |
internal/lsp/server.go |
Registered code lens handlers and wired up resolve handler with command name from initialization |
internal/lsp/lsproto/lsp_generated.go |
Generated types for CodeLensData structure and CodeLensKind enumeration |
internal/lsp/lsproto/_generate/generate.mts |
Added custom type definitions for code lens data and enumeration |
internal/ast/ast.go |
Changed getDeclarationFromName to exported GetDeclarationFromName for use in code lens logic |
internal/diagnostics/* |
Added localized message templates for references and implementations counts |
internal/fourslash/fourslash.go |
Extended test harness with code lens verification and includeDeclaration: true for find-all-refs baseline tests |
internal/fourslash/baselineutil.go |
Added codeLensesCmd baseline command type |
internal/fourslash/tests/*_test.go |
Six new test files covering various code lens scenarios with different preference configurations |
testdata/baselines/reference/fourslash/codeLenses/*.baseline.jsonc |
Six new baseline files capturing expected code lens outputs |
testdata/baselines/reference/fourslash/state/*.baseline |
39 updated baseline files reflecting includeDeclaration: true change |
_extension/src/client.ts |
Registered client-side command handler to convert LSP locations to VS Code peek view |
_extension/package.json |
Added command contribution for code lens show locations (disabled from command palette) |
internal/ls/completions.go |
Removed unused maps import after changing iteration pattern |
jakebailey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried this out and it works really well, even in checker.ts and when we do code lenses on all funcs.
This PR implements support for
textDocument/codeLensandcodeLens/resolveon the server side, alongside a custom client command calledtypescript.codeLens.showLocations.CodeLensobjects are produced bytextDocument/codeLensas indicators that an editor client can get more information "later on". WhencodeLens/resolveis called for a givenCodeLensobject, theTitle,Command, and commandArgumentsare populated appropriately so that a user can see some high-level information and potentially click on the UI decoration to trigger a command.We produce two types of
CodeLensobjects: "references" code lens, and "implementations" code lens. These act as markers/indicators so that when an editor like VS Code scrolls into the view of a code lens, it will call on the server to populate each lens with a title and list of locations to preview upon clicking. This amounts to a find-all-references or go-to-implementation call when scrolling into the view of a given code lens.The VS Code extension wires this up with a minimal client-side command which just converts the LSP objects into VS Code ones before redirecting the values to the editor's "peek" pane.
Much of the logic is roughly ported over from VS Code's Code Lens implementations, though it is not one-to-one. On an implementation level, instead of two separate providers/walks like the client did, this implementation does one walk and may produce up to two
CodeLensobjects per implementation, each which encodes how theCodeLensshould be resolved later. Since we have full syntactic and semantic information, we can make better judgments on what kinds of syntax should be decorated with aCodeLensif we ever want to modify the behavior.That said, we have some differences - some that I think are desirable, and some that are just bugs.
One thing I implemented here was support for
includeDeclarationintextDocument/references(aside: I find this behavior iffy for callers who "forget" to pass inincludeDeclaration). I used this as a shortcut to avoid filtering out declarations in find-all-references; however, as you can see above, this gets a different "References" count because it drops overrides/implementations. I actually think that this might not be the right move, and that maybe we should only discount references containing the source position.Current TODOs:
!!!for now)Datais handled given Jake's PR at Refine LSP with our own types, generate more stuff #2141codeLens/resolve.Fixes #1897