[M] Change in dart/sdk[main]: [analysis_server] Support String IDs in log replay + run some LSP int...
0 views
Skip to first unread message
Danny Tuppeny (Gerrit)
Jan 15, 2026, 6:28:58 AM (7 days ago) Jan 15
to Brian Wilkerson, Jake Macdonald, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Brian Wilkerson and Jake Macdonald
Danny Tuppeny added 2 comments![]( "Open in Gerrit")
File pkg/analysis_server/lib/src/session_logger/log_entry.dart
Line 44, Patchset 1:
// The id in the JSON could be either an int or String (LSP is JSON-RPC 2
// which allows either).Danny Tuppeny . unresolved
I'm not sure you how you feel about using this `Either2` class from the LSP files, but I think it's either that or `Object?` since we need to support both, and we can't just `toString()` the int, because `1` and `"1"` are both valid and distinct IDs.
File pkg/analysis_server/tool/log_player/log_player.dart
Line 21, Patchset 2 (Latest):
bool _shouldSkip(Message message) =>Danny Tuppeny . unresolved
The tests all seem to pass locally, but I don't know if they cover the log player or if I should be testing it manually. If the latter, are there any notes on how to capture and replay one to verify it?
Related details
Attention is currently required from:
- Brian Wilkerson
- Jake Macdonald
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Jan 15, 2026, 5:32:50 PM (6 days ago) Jan 15
to Danny Tuppeny, Brian Wilkerson, Jake Macdonald, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Danny Tuppeny and Jake Macdonald
Brian Wilkerson voted and added 3 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
| Commit-Queue | +1 |
3 comments
Patchset-level comments
File-level comment, Patchset 2 (Latest):
Brian Wilkerson . resolved
Sorry for the delay. I thought I had reviewed it earlier but is appear I didn't.
File pkg/analysis_server/lib/src/session_logger/log_entry.dart
Line 44, Patchset 1:
// The id in the JSON could be either an int or String (LSP is JSON-RPC 2
// which allows either).Danny Tuppeny . resolved
I'm not sure you how you feel about using this `Either2` class from the LSP files, but I think it's either that or `Object?` since we need to support both, and we can't just `toString()` the int, because `1` and `"1"` are both valid and distinct IDs.
Brian Wilkerson
I'm fine with using it.
File pkg/analysis_server/tool/log_player/log_player.dart
Line 21, Patchset 2 (Latest):
bool _shouldSkip(Message message) =>Danny Tuppeny . resolved
The tests all seem to pass locally, but I don't know if they cover the log player or if I should be testing it manually. If the latter, are there any notes on how to capture and replay one to verify it?
Brian Wilkerson
Jake is going to land a `.md` file to explain how to use the tool.
Related details
Attention is currently required from:
- Danny Tuppeny
- Jake Macdonald
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Jan 19, 2026, 7:23:36 AM (3 days ago) Jan 19
to Commit Queue, Brian Wilkerson, Jake Macdonald, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Jake Macdonald
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Danny Tuppeny . resolved
The issue this will fix came up at https://github.com/dart-lang/sdk/issues/62442
I'm not yet sure how it was triggered - I expected the bug would only affect LSP clients using String IDs, and that would not include VS Code (I haven't seen it myself, despite being on an SDK that includes the original change).
Related details
Attention is currently required from:
- Jake Macdonald
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jake Macdonald (Gerrit)
Jan 21, 2026, 1:56:39 PM (10 hours ago) Jan 21
to Danny Tuppeny, Felipe Morschel, Commit Queue, Brian Wilkerson, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Danny Tuppeny
Jake Macdonald voted Code-Review+1![]( "Open in Gerrit")
Attention is currently required from:
- Danny Tuppeny
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Jan 21, 2026, 2:37:50 PM (10 hours ago) Jan 21
to Danny Tuppeny, Brian Wilkerson, Jake Macdonald, Felipe Morschel, Commit Queue, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Danny Tuppeny
Brian Wilkerson voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention is currently required from:
- Danny Tuppeny
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Jan 21, 2026, 2:47:58 PM (10 hours ago) Jan 21
to Danny Tuppeny, Brian Wilkerson, Jake Macdonald, Felipe Morschel, Commit Queue, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Danny Tuppeny
Brian Wilkerson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
1 comment
Patchset-level comments
File-level comment, Patchset 3 (Latest):
Brian Wilkerson . resolved
Sigh. Looks like it needs to be rebased.
Related details
Attention is currently required from:
- Danny Tuppeny
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Jan 21, 2026, 2:49:42 PM (9 hours ago) Jan 21
to Brian Wilkerson, Jake Macdonald, Felipe Morschel, Commit Queue, dart-analys...@google.com, rev...@dartlang.org
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
Patchset-level comments
Brian Wilkerson . resolved
Sigh. Looks like it needs to be rebased.
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Jan 21, 2026, 2:55:13 PM (9 hours ago) Jan 21
to Danny Tuppeny, Brian Wilkerson, Jake Macdonald, Felipe Morschel, Commit Queue, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Danny Tuppeny
Brian Wilkerson voted Commit-Queue+2![]( "Open in Gerrit")
Attention is currently required from:
- Danny Tuppeny
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Jan 21, 2026, 3:24:33 PM (9 hours ago) Jan 21
to Danny Tuppeny, Brian Wilkerson, Jake Macdonald, Felipe Morschel, dart-analys...@google.com, rev...@dartlang.org
Commit Queue submitted the change![]( "Open in Gerrit")
Unreviewed changes
3 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
Commit message:
[analysis_server] Support String IDs in log replay + run some LSP integration tests with string IDs
My previous CL made some LSP tests run with String IDs instead of ints, but those tests don't appear to go through the session logger so did not fail with the casts here.
This change adds a base integration test that also uses string IDs, which did fail on the cast, so I've updated the session logger to use `Either2<int, String>` for IDs instead.
Fixes https://github.com/dart-lang/sdk/issues/62442
Change-Id: Iee582e9ce2b8b5a1127120c987670a679d2ca76c
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/473260
Commit-Queue: Brian Wilkerson <brianwi...@google.com>
Reviewed-by: Jake Macdonald <jak...@google.com>
Reviewed-by: Brian Wilkerson <brianwi...@google.com>
Files:
- M pkg/analysis_server/integration_test/lsp_server/initialization_test.dart
- M pkg/analysis_server/integration_test/lsp_server/integration_tests.dart
- M pkg/analysis_server/lib/src/session_logger/log_entry.dart
- M pkg/analysis_server/lib/src/session_logger/session_logger_sink.dart
- M pkg/analysis_server/tool/log_player/log_player.dart
- M third_party/pkg/language_server_protocol/lib/protocol_special.dart
Change size: M
Delta: 6 files changed, 58 insertions(+), 20 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Jake Macdonald, +1 by Brian Wilkerson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages