Fix getReferencedSymbolsForSymbol, implement getReferencesAtExportSpecifier #2304
Conversation
jakebailey
requested review from
DanielRosenwasser,
andrewbranch,
Copilot,
gabritto and
iisaduan
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #2204 by implementing the missing getReferencesAtExportSpecifier function and correcting a boolean logic error in getReferencedSymbolsForSymbol. The changes enable proper handling of references in export specifiers, particularly for re-exports and aliased exports.
Key Changes
- Implemented
getReferencesAtExportSpecifierfunction that was previously marked as "not implemented" - Fixed backwards condition in
getReferencedSymbolsForSymbol(changed!isForRenameWithPrefixAndSuffixTexttoisForRenameWithPrefixAndSuffixText) - Enhanced fourslash test error reporting to display actual error messages instead of "nil response"
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/ls/findallreferences.go | Core implementation: added getReferencesAtExportSpecifier function, fixed boolean condition, added seenReExportRHS field to track seen re-export right-hand sides, and enabled calls to the new function |
| internal/fourslash/fourslash.go | Improved error handling to report actual LSP errors instead of generic "nil response" messages |
| internal/fourslash/tests/statefindallrefs_test.go | Added comprehensive test for re-exports in multi-project solutions to verify the fix |
| internal/fourslash/tests/gen/renameForAliasingExport02_test.go | Removed t.Skip() to enable previously failing test |
| internal/fourslash/_scripts/failingTests.txt | Removed TestRenameForAliasingExport02 from failing tests list |
| testdata/baselines/reference/submodule/fourslash/findRenameLocations/.baseline.jsonc | Updated baselines showing convergence with TypeScript behavior - export specifiers now correctly show rename locations |
| testdata/baselines/reference/fourslash/findAllReferences/*.baseline.jsonc | Updated baselines showing export specifiers are now correctly identified as references |
| testdata/baselines/reference/fourslash/documentHighlights/*.baseline.jsonc | Updated baselines showing export specifiers are now correctly highlighted |
| testdata/baselines/reference/fourslash/state/*.baseline | Updated state baselines reflecting correct reference tracking for export specifiers |
|
|
||
| var exportSpecifier *ast.Node | ||
| if !isForRenameWithPrefixAndSuffixText(options) || len(symbol.Declarations) == 0 { | ||
| if isForRenameWithPrefixAndSuffixText(options) && len(symbol.Declarations) != 0 { |
There was a problem hiding this comment.
Not specific to this PR, but maybe we want to rename this function since the actual user preference is useAliasesForRenames?
There was a problem hiding this comment.
It was called isForRenameWithPrefixAndSuffixText in Strada, we are just seemingly inconsistent all around...
There was a problem hiding this comment.
I know, that's what I meant. Strada used providePrefixAndSuffixTextForRename as the preference name and related functions, but it was called useAliasesForRenames in vscode, and now in Corsa we confusingly have both terms.
There was a problem hiding this comment.
Yeah. Probably better fixed in another PR. #2272 at least lets us choose whichever name we want.
gabritto
left a comment
gabritto
left a comment
There was a problem hiding this comment.
This looks good, thanks! I think the remaining rename diffs may just be because we haven't set up rename user preferences yet I think.
Fixes #2204
getReferencesAtExportSpecifierhad not been ported.getReferencedSymbolsForSymbolhad a condition backwards.Finding the latter involved debugging a panic; in this PR is also a fix to fourslash to actually print out an error message instead of "nil response".