Prevent renaming keywords by validating in handleRename #2231
Conversation
- Add nodeIsEligibleForRename function to check if a node can be renamed - Implement ProvidePrepareRename in language service - Add handlePrepareRename handler in LSP server - Update RenameProvider capability to indicate prepareProvider support - Handle keyword adjustments and eligibility checks Co-authored-by: jakebailey <[email protected]>
- Update ProvideSymbolsAndEntries to check node eligibility after adjustment - Add test for prepareRename on keywords - Format code Co-authored-by: jakebailey <[email protected]>
internal/ls/findallreferences.go
Outdated
|
|
||
| // ProvidePrepareRename provides information for rename preparation. | ||
| // This checks if the location is eligible for renaming and returns the range and placeholder. | ||
| func (l *LanguageService) ProvidePrepareRename(ctx context.Context, uri lsproto.DocumentUri, documentPosition lsproto.Position) (lsproto.PrepareRenameResponse, error) { |
There was a problem hiding this comment.
textDocument/prepareRename is an optional feature in the LSP.
Can we not just do this within handleRename by returning an error there?
There was a problem hiding this comment.
The issue shows a tooltip that appears when hovering/triggering rename, which comes from prepareRename. If we return an error from handleRename instead, users would only see the error AFTER attempting the rename, not as an early warning.
However, I can simplify the implementation to avoid the test regressions. The current test failures suggest that the validation logic needs to match TypeScript's more closely, which is complex. Would you prefer:
- Keep
prepareRenamebut simplify the validation logic - Return errors from
handleRenameand accept the different UX (error notification vs tooltip)
Let me know your preference and I'll adjust accordingly.
Remove prepareRename implementation and return error from handleRename instead. - Add nodeIsEligibleForRename function to check basic eligibility - Update ProvideSymbolsAndEntries to validate adjusted node - Return error from ProvideRenameFromSymbolAndEntries when no symbols found - Remove prepareRename handler and test Note: Awaiting feedback on approach - some tests need adjustment for the new validation logic. Co-authored-by: jakebailey <[email protected]>
internal/ls/findallreferences.go
Outdated
| // Check if ProvideSymbolsAndEntries returned ok=false or no symbols found | ||
| // Return an error to inform the user that rename is not available | ||
| if symbolsAndEntries == nil || len(symbolsAndEntries) == 0 { | ||
| return lsproto.WorkspaceEditOrNull{}, fmt.Errorf("%s", diagnostics.You_cannot_rename_this_element) |
There was a problem hiding this comment.
@copilot you need to localize this diagnostic, not just print it.
There was a problem hiding this comment.
Fixed in d8a16fb - now using diagnostics.You_cannot_rename_this_element.Localize(locale.FromContext(ctx)) to properly localize the error message.
Use locale.FromContext(ctx) and .Localize() to properly localize the "You cannot rename this element" error message instead of just converting to string. Co-authored-by: jakebailey <[email protected]>
internal/ls/findallreferences.go
Outdated
| // Return an error to inform the user that rename is not available | ||
| if symbolsAndEntries == nil || len(symbolsAndEntries) == 0 { | ||
| return lsproto.WorkspaceEditOrNull{}, fmt.Errorf("%s", diagnostics.You_cannot_rename_this_element) | ||
| return lsproto.WorkspaceEditOrNull{}, fmt.Errorf(diagnostics.You_cannot_rename_this_element.Localize(locale.FromContext(ctx))) |
There was a problem hiding this comment.
@copilot This is a bug; you are passing a message into the format string of a *f func
There was a problem hiding this comment.
Fixed in 3b02540 - now using errors.New() instead of fmt.Errorf() to avoid treating the localized message as a format string.
Use errors.New() instead of fmt.Errorf() when creating error from localized message to avoid treating the message as a format string. Co-authored-by: jakebailey <[email protected]>
Current Status
Implemented validation in
handleRenamethat returns a properly localized error when renaming invalid targets.Changes:
nodeIsEligibleForRename()for basic node type validationProvideSymbolsAndEntries()to check eligibility of adjusted nodesProvideRenameFromSymbolAndEntries()to return localized error for ineligible renameslocale.FromContext(ctx)and.Localize()for proper localizationerrors.New()instead offmt.Errorf()to avoid treating localized message as format stringNote:
Some fourslash tests are failing because they expect empty results (no changes) but now receive errors. Awaiting clarification on whether to:
prepareRenamefor better UX (tooltip vs error notification)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.