Fix Pylance settings handling on Dev18 - #8585
Fix Pylance settings handling on Dev18#8585Stella Huang (StellaHuang95) wants to merge 3 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b6a821e-0a88-49fa-807e-874551b5a398
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
🔒 Automated review in progress — Heejae Chang (@heejaechang) is auto-reviewing this PR. |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This pull request fixes Dev18 (System.Text.Json-based) configuration delivery to Pylance so that VS options (notably Automatically add brackets to functions) are correctly serialized/bound and reflected in Pylance completion behavior.
Changes:
- Updated Dev18 signature-help interception to use a System.Text.Json-friendly contract (
JsonDocument) while preserving the Dev17JTokenpath. - Enabled whole-object parameter binding for object-shaped custom JSON-RPC messages and added a cross-serializer deserialization helper.
- Updated settings payload shape/serialization (DTO fields → properties; fallback
didChangeConfigurationnow sends settings under thepythonsection key).
Show a summary per file
| File | Description |
|---|---|
| Python/Product/PythonTools/PythonTools/LanguageServerClient/PythonSignatureHelpMiddleLayer.cs | Switches Dev18 middle-layer generic to JsonDocument to avoid Newtonsoft selecting the wrong formatter. |
| Python/Product/PythonTools/PythonTools/LanguageServerClient/PythonLanguageClientCustomTarget.cs | Adjusts JSON-RPC handlers to use single-object parameter binding and serializer-neutral deserialization. |
| Python/Product/PythonTools/PythonTools/LanguageServerClient/PythonLanguageClient.cs | Updates fallback didChangeConfiguration payload to be section-keyed under python. |
| Python/Product/PythonTools/PythonTools/LanguageServerClient/LanguageServerSettings.cs | Converts outbound settings DTO public fields to properties so STJ and Newtonsoft serialize consistently. |
Review details
Comments suppressed due to low confidence (2)
Python/Product/PythonTools/PythonTools/LanguageServerClient/PythonLanguageClientCustomTarget.cs:175
OnWorkspaceConfigurationno longer catches exceptions. IfDeserialize(...)or theWorkspaceConfigurationevent handler throws, the exception can bubble out of the JSON-RPC target and potentially disrupt the language-client RPC loop; the previous implementation returnednullon failure.
public async Task<object> OnWorkspaceConfiguration(object arg) {
var reqParams = Deserialize<WorkspaceConfiguration.ConfigurationParams>(arg);
if (this.WorkspaceConfiguration != null && reqParams != null) {
var eventArgs = new ConfigurationArgs { requestParams = reqParams, requestResult = null };
await this.WorkspaceConfiguration.InvokeAsync(this, eventArgs);
Python/Product/PythonTools/PythonTools/LanguageServerClient/PythonLanguageClientCustomTarget.cs:190
Deserialize<T>relies onarg.ToString()to produce JSON for non-JTokenvalues. ForSystem.Text.Jsonpayloads this is not guaranteed (e.g.,JsonDocument.ToString()is not JSON), which can cause deserialization failures and break configuration/registration handling.
// Dev18 supplies System.Text.Json values; older formatters supply JToken.
return arg is JToken token
? token.ToObject<T>()
: JsonConvert.DeserializeObject<T>(arg.ToString());
}
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Low
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Heejae Chang (heejaechang)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (2)
Python/Product/PythonTools/PythonTools/LanguageServerClient/PythonLanguageClientCustomTarget.cs:190
- Deserialize falls back to JsonConvert.DeserializeObject(arg.ToString()) for System.Text.Json payloads. If StreamJsonRpc supplies a JsonDocument (or other STJ container that doesn’t override ToString()), arg.ToString() won’t be valid JSON, causing deserialization to fail (and silently break telemetry/config handling). Prefer extracting raw JSON via GetRawText() for JsonElement/JsonDocument before passing to JsonConvert.
// Dev18 supplies System.Text.Json values; older formatters supply JToken.
return arg is JToken token
? token.ToObject<T>()
: JsonConvert.DeserializeObject<T>(arg.ToString());
}
Python/Product/PythonTools/PythonTools/LanguageServerClient/PythonLanguageClientCustomTarget.cs:116
- OnTelemetryEvent previously swallowed all exceptions during logging/notification handling. The new implementation can let exceptions from Trace/_logger/PylanceException (or event handlers) bubble out of the JsonRpc dispatch, which can destabilize the LSP connection. Consider restoring a defensive try/catch around the logging + AnalysisComplete notification (and fix the current mis-indentation on the invoke line).
Trace.WriteLine(arg);
if (telemetry.Exception == null) {
_logger?.LogEvent(telemetry.EventName, telemetry.Properties, telemetry.Measurements);
} else {
_logger?.LogFault(new PylanceException(telemetry.EventName, telemetry.Exception.stack), telemetry.EventName, false);
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Low
| public async Task<object> OnWorkspaceConfiguration(object arg) { | ||
| var reqParams = Deserialize<WorkspaceConfiguration.ConfigurationParams>(arg); | ||
| if (this.WorkspaceConfiguration != null && reqParams != null) { | ||
| var eventArgs = new ConfigurationArgs { requestParams = reqParams, requestResult = null }; | ||
| await this.WorkspaceConfiguration.InvokeAsync(this, eventArgs); | ||
| return eventArgs.requestResult; | ||
| } | ||
| return null; | ||
| } |
Heejae Chang (heejaechang)
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b6a821e-0a88-49fa-807e-874551b5a398
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Review details
Comments suppressed due to low confidence (3)
Python/Product/PythonTools/PythonTools/LanguageServerClient/PythonLanguageClientCustomTarget.cs:116
- OnTelemetryEvent no longer guards the logging/event-invocation block with a catch. If a logger implementation or an AnalysisComplete subscriber throws, the exception can escape the JSON-RPC target and potentially tear down the RPC connection. Wrap the processing block in a try/catch (matching the prior behavior) so malformed/unexpected telemetry can’t destabilize the client.
Trace.WriteLine(arg);
if (telemetry.Exception == null) {
_logger?.LogEvent(telemetry.EventName, telemetry.Properties, telemetry.Measurements);
} else {
_logger?.LogFault(new PylanceException(telemetry.EventName, telemetry.Exception.stack), telemetry.EventName, false);
Python/Product/PythonTools/PythonTools/LanguageServerClient/PythonLanguageClientCustomTarget.cs:144
- Changing python/endProgress to a parameterless method can cause JSON-RPC binding failures (-32602) if the server sends a params object (even an empty one). Consider taking a single optional parameter to remain compatible with both shapes.
[JsonRpcMethod("python/endProgress")]
public async Task OnEndProgressAsync() {
await _joinableTaskContext.Factory.SwitchToMainThreadAsync();
Python/Product/PythonTools/PythonTools/LanguageServerClient/PythonLanguageClientCustomTarget.cs:175
- OnWorkspaceConfiguration previously swallowed deserialization/handler exceptions and returned null. The new implementation can now throw (e.g., JsonException from Deserialize or exceptions from WorkspaceConfiguration handlers), which risks breaking the JSON-RPC connection. Reintroduce a catch and return null on failure, similar to OnWorkspaceFolders.
[JsonRpcMethod("workspace/configuration", UseSingleObjectParameterDeserialization = true)]
public async Task<object> OnWorkspaceConfiguration(object arg) {
var reqParams = Deserialize<WorkspaceConfiguration.ConfigurationParams>(arg);
if (this.WorkspaceConfiguration != null && reqParams != null) {
var eventArgs = new ConfigurationArgs { requestParams = reqParams, requestResult = null };
await this.WorkspaceConfiguration.InvokeAsync(this, eventArgs);
- Files reviewed: 6/6 changed files
- Comments generated: 3
- Review effort level: Low
| [JsonRpcMethod("python/beginProgress")] | ||
| #pragma warning disable IDE0060 // Remove unused parameter | ||
| public void OnBeginProgressAsync(JToken arg) { | ||
| #pragma warning restore IDE0060 // Remove unused parameter | ||
| public void OnBeginProgressAsync() { | ||
| } |
| // Dev18 supplies System.Text.Json values; older formatters supply JToken. | ||
| return arg is JToken token | ||
| ? token.ToObject<T>() | ||
| : JsonConvert.DeserializeObject<T>(arg.ToString()); | ||
| } |
| <Reference Include="System.Text.Json, Version=8.0.0.5, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" Condition="'$(VSTarget)' == '18.0'"> | ||
| <HintPath>$(PackagesPath)\System.Text.Json.8.0.5\lib\net462\System.Text.Json.dll</HintPath> | ||
| <Private>false</Private> | ||
| <SpecificVersion>true</SpecificVersion> | ||
| </Reference> |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Heejae Chang (heejaechang)
left a comment
There was a problem hiding this comment.
Approved via Review Center.



Summary
Fixes #8578.
Checking Automatically add brackets to functions persisted correctly in Visual Studio, but Dev18 did not deliver the enabled setting to Pylance. As a result, Pylance continued returning plain completion text such as
getcwdinstead of a snippet such asgetcwd($0).Root cause
Dev18 uses System.Text.Json for eligible language clients, exposing three serialization and binding mismatches in the PTVS/Pylance configuration path:
workspace/configurationparameters as one object ({ "items": [...] }), but the custom StreamJsonRpc handler did not opt into whole-object parameter binding, so the request was rejected with-32602.{}even when the option was enabled.workspace/didChangeConfigurationpayload placed analysis settings directly undersettings.analysis, while Pylance expects the section undersettings.python.analysis.Latest
mainalso introduced aJTokensignature-help middle layer. On Dev18, that type selects the Newtonsoft formatter for the whole language client, which conflicts with the STJ-compatible path needed for PTVS's initialize interception. The Dev18 implementation therefore usesJsonDocument, while the existing Dev17JTokenimplementation remains unchanged.Fix
workspace/configuration.pythonsection.Risk
Risk is low because the changes are limited to the Pylance language-client serialization and custom RPC boundary:
Validation
The fix was manually verified on Dev18: after enabling the option, function completions included parentheses as expected. No tests were added for this change.