Skip to content

Fix LanguageClient.prepareRename() support for { defaultBehavior: true }#1748

Open
ndonfris wants to merge 1 commit into
microsoft:mainfrom
ndonfris:fix/prepareRename-defaultBehavior
Open

Fix LanguageClient.prepareRename() support for { defaultBehavior: true }#1748
ndonfris wants to merge 1 commit into
microsoft:mainfrom
ndonfris:fix/prepareRename-defaultBehavior

Conversation

@ndonfris
Copy link
Copy Markdown

  • /client behavior no longer converts { defaultBehavior: true } to null, to now match >= 3.16.0 specification

    • converts { defaultBehavior: true } into the editor's default word range, allowing textDocument/rename requests without widening the VS Code provider type
    • continues to reject { defaultBehavior: false }, preventing textDocument/rename requests
  • /client-node-tests new related prepareRename tests

    • adds test server coverage for a controlled { defaultBehavior: true } prepareRename response
    • adds a dedicated integration test asserting the client converts that server result into a vscode.Range while keeping the existing Rename test focused on the standard flow

…ue }`

* /client behavior no longer converts `{ defaultBehavior: true }` to `null`, to now match >= 3.16.0 specification

  - converts `{ defaultBehavior: true }` into the editor's default word range, allowing `textDocument/rename` requests without widening the VS Code provider type
  - continues to reject `{ defaultBehavior: false }`, preventing `textDocument/rename` requests

* /client-node-tests new related `prepareRename` tests

  - adds test server coverage for a controlled `{ defaultBehavior: true }` prepareRename response
  - adds a dedicated integration test asserting the client converts that server result into a vscode.Range while keeping the existing Rename test focused on the standard flow
@ndonfris
Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Copy link
Copy Markdown

Copilot AI left a 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 updates the language client’s prepareRename handling to support LSP { defaultBehavior: true } results by mapping them to a VS Code word range, and adds integration coverage to ensure the behavior is exercised end-to-end.

Changes:

  • Convert LSP { defaultBehavior: true } prepareRename responses into document.getWordRangeAtPosition(...) instead of null.
  • Extend the node test server to return { defaultBehavior: true } for a specific position.
  • Add an integration test asserting the client converts { defaultBehavior: true } into a vscode.Range.
Show a summary per file
File Description
client/src/common/rename.ts Maps LSP defaultBehavior prepareRename responses to a VS Code word range.
client-node-tests/src/servers/testServer.ts Makes the test server return { defaultBehavior: true } for a controlled cursor position.
client-node-tests/src/integration.test.ts Adds an integration test validating the defaultBehavior-to-Range conversion.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 3/3 changed files
  • Comments generated: 2

Comment on lines 99 to 103
} else if (this.isDefaultBehavior(result)) {
return result.defaultBehavior === true
? null
? document.getWordRangeAtPosition(position)
: Promise.reject(new Error(`The element can't be renamed.`));
} else if (result && Range.is(result.range)) {
const defaultBehaviorResult = await provider.prepareRename(document, defaultBehaviorPosition, tokenSource.token) as vscode.Range;

isInstanceOf(defaultBehaviorResult, vscode.Range);
assert.deepStrictEqual(defaultBehaviorResult, defaultBehaviorExpected);
@dbaeumer dbaeumer added the info-needed Issue requires more information from poster label May 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

info-needed Issue requires more information from poster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants