[M] Change in dart/sdk[main]: DAS plugins: Only use fix edits which apply to each file with a match...
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
Let me know if this direction makes sense. I have to do some manual testing before I land.
Related details
- Brian Wilkerson
- Danny Tuppeny
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 1 comment![]( "Open in Gerrit")
.modificationStamp;The logic here seems right. The question is whether the `modificationStamp` and the `editFileStamp` are the consistent. The plugin isolate produced the `editFileStamp`, but I don't know where that came from. I also don't know where the `modificationStamp` came from.
If they are both from the file system then they are consistent and I think we're good to go. But if either of them is a fake modification stamp from the client, then was it passed along in such a way as to ensure that both isolates are seeing the same value?
Related details
- Danny Tuppeny
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 1 comment![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Commit-Queue | +1 |
1 comment
The logic here seems right. The question is whether the `modificationStamp` and the `editFileStamp` are the consistent. The plugin isolate produced the `editFileStamp`, but I don't know where that came from. I also don't know where the `modificationStamp` came from.
If they are both from the file system then they are consistent and I think we're good to go. But if either of them is a fake modification stamp from the client, then was it passed along in such a way as to ensure that both isolates are seeing the same value?
Good point. I checked with where the `timeStamp` field is created in the "change builder" code and sure enough, all of the code there uses `0` or `-1` (for a non-existent file).
I then updated the code in change_builder_core.dart to use real modification time stamps. But this does not work for OverlayResourceProvider. :(
When I send a request for edits for a file which is unchanged in the IDE (but which was previously changed, so I think an Overlay was added and then removed), I get these timestamps:
Request time stamp: 1758812993075, time stamp on edit: 0
`ResourceProvider.setOverlay` is always called with `modificationStamp: _overlayModificationStamp++`, in legacy_analysis_server.dart, lsp_analysis_server.dart, and plugin_server.dart. I obviously copy/pasted the impl into plugin_server.dart. I think that this code is used because we otherwise don't have a timestamp to use. LspAnalysisServer and LegacyAnalysisServer could use
`DateTime.now`... but that probably wouldn't work: If the client (VS Code) sent
an overlay notification with one timestamp, and asked for edits, and LspAnalysisServer set it's own overlay time to `DateTime.now`, then several milliseconds would have elapsed.
(Indeed, AnalysisServer starts with
`int overlayModificationStamp = DateTime.now().millisecondsSinceEpoch;` and PluginServer starts with
`int _overlayModificationStamp = 0;` but I don't think we can try to synchronize these either.)
I see that the `AddContentOverlay` and the `ChangeContentOverlay` classes have a `version` on them, "Version numbers allow the server to tag edits with the version of the document they apply to which can avoid applying edits to documents that have already been updated since the edits were computed." Well that sounds great! I've started using these in this CL. Let me know what you think...
Related details
- Brian Wilkerson
- Danny Tuppeny
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 1 comment![]( "Open in Gerrit")
int _overlayModificationStamp = DateTime.now().millisecondsSinceEpoch;It's my understanding that LSP requires the 'version' fields the client sends to be monotonically increasing but not necessarily sequential. That means that they might be timestamps, or they might be just the next integer after the previous 'version' number.
As a result, I don't know that we can / want to use a real timestamp. I'll defer to you and Danny on this one.
I don't know whether the legacy protocol sends us a version number or whether it just always ends up being some constant value. If they don't send us one then a fake 'version' similar to LSP's would be best when communicating with the plugin isolate.
Related details
- Danny Tuppeny
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
int _overlayModificationStamp = DateTime.now().millisecondsSinceEpoch;It's my understanding that LSP requires the 'version' fields the client sends to be monotonically increasing but not necessarily sequential. That means that they might be timestamps, or they might be just the next integer after the previous 'version' number.
As a result, I don't know that we can / want to use a real timestamp. I'll defer to you and Danny on this one.
I don't know whether the legacy protocol sends us a version number or whether it just always ends up being some constant value. If they don't send us one then a fake 'version' similar to LSP's would be best when communicating with the plugin isolate.
This change just brings the plugin server in line with `AnalysisServer`: https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/lib/src/analysis_server.dart#L237
There is a theory there about reducing collisions. I can add a similar comment here.
Otherwise, this CL doesn't have anything to do with what LSP clients or legacy clients send to DAS. This is purely between DAS and the plugins isolate. In lsp_analysis_server.dart, we start passing an overlay version number in each call to AddContentOverlay and ChangeContentOverlay. And then plugin_server.dart consumes them here.
Related details
- Brian Wilkerson
- Danny Tuppeny
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |