Support identifier location in inlay hints #2435
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for providing identifier locations in inlay hints, adapting the Strada approach to Corsa. Instead of attaching symbols directly to identifiers, the implementation uses a separate idToSymbol map that is provided to the node builder, which then tracks synthesized identifiers and their associated symbols. This enables inlay hints to provide location information for type identifiers, making them clickable in editors.
Key Changes
- Added infrastructure to track symbol-to-identifier mappings through the node builder API
- Enhanced inlay hints to include location information for identifiers in type annotations
- Added special handling for bundled file URIs to prevent crashes during tests
Reviewed changes
Copilot reviewed 52 out of 52 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/checker/nodebuilder.go |
Added NewNodeBuilderEx and getNodeBuilderEx functions to support optional idToSymbol map parameter |
internal/checker/nodebuilderimpl.go |
Added idToSymbol field, newIdentifier helper method, and logic to track symbol associations for synthesized identifiers |
internal/checker/printer.go |
Updated TypeToTypeNode and TypePredicateToTypePredicateNode signatures to accept idToSymbol map parameter |
internal/ls/inlay_hints.go |
Modified to create and pass idToSymbol map when generating type nodes, and updated to use map for location lookups |
internal/ls/completions.go |
Updated call site to pass nil for idToSymbol parameter; removed unused ptrIsTrue and ptrIsFalse helper functions |
internal/bundled/embed.go |
Added IsBundled function to detect bundled file paths |
internal/bundled/noembed.go |
Added IsBundled function stub returning false |
internal/lsp/lsproto/lsp.go |
Added check in FileName() to handle bundled URIs without conversion |
internal/ls/lsconv/converters.go |
Added check in FileNameToDocumentURI() to handle bundled file names |
internal/fourslash/fourslash.go |
Fixed baseline normalization to reset both start and end positions for lib file locations |
internal/fourslash/tests/inlayHintsIdentifierLocation_test.go |
Added new test for identifier location feature |
| Test baselines | Updated to reflect new location information in inlay hints |
| nodebuilder.FlagsUseAliasDefinedOutsideCurrentScope | ||
| typeNode := s.checker.TypePredicateToTypePredicateNode(typePredicate, nil /*enclosingDeclaration*/, flags) | ||
| idToSymbol := make(map[*ast.IdentifierNode]*ast.Symbol) | ||
| // !!! Avoid type node reuse so we collect identifier symbols. |
There was a problem hiding this comment.
q: If we reused a node, wouldn't a node.Symbol() suffice for that node, inside getInlayHintLabelParts, rather than only looking for the (potentially not) synthesized node's symbol?
There was a problem hiding this comment.
I think we don't store symbols on identifiers anymore in Corsa, so node.Symbol() for an AST identifier would always return nil.
There was a problem hiding this comment.
We could try to obtain the symbol for an AST identifier via checker methods here though. I might consider that solution once we get to it, I don't know which option would be faster.
In Strada, identifiers had symbols, and when we synthesized an identifier in a type node, we'd store a symbol in it, which we would later use in inlay hints to obtain a location for an identifier in the inlay hint label parts.
This PR adapts that for Corsa, but instead of attaching a symbol to an identifier, we now have to store this information in a separate map that is provided to the node builder implementation.
All inlay hint existing diffs are now good changes as far as I can tell. The newly accepted diffs are due to a few cases where Strada couldn't provide a symbol for an identifier but Corsa can.
I also had to add code to avoid converting bundled file names to URIs in the tests, as that would cause a crash, and I left a
!!!for possibly avoiding type node reuse in inlay hints once that's implemented, because I think otherwise we can't easily keep track of the mapping of identifiers to symbols.