mirror of
https://github.com/oxc-project/oxc.git
synced 2026-11-04 10:29:58 +00:00
refactor(lsp): store the document version (#27926)
Split out of #26862, as discussed there. - `LSPFileSystem` stores the version reported by `textDocument/didOpen` and `textDocument/didChange` next to the content (`set_with_version`, `get_version`); `set`, used for `didSave`, keeps the stored version since that notification carries none. The API follows #23796, on `main`. - `TextDocument` carries the version (`None` for content read from disk) and `CodeActionParams` carries `document_version: Option<i32>` instead of `is_open_document`, which is `Some` exactly when the document is open. A `didSave` carrying text for a document which is not stored (a conforming client never sends one, since `didSave` only follows `didOpen`) still creates the entry as before, now with version 0. No behaviour change: nothing reads the version yet. The code-action cache keyed by it, and the `source.fixAll` fix of #26862, come separately on top of this. Tests: `LSPFileSystem` versions, and the backend wiring (`didOpen` 1 → `didChange` 2 → `didSave` keeps 2 → `didClose` none). Worth knowing: `CodeActionParams.is_open_document` becomes `document_version` and `TextDocument` gains a `version` field, which touches any external `Tool` implementor. AI disclosure: implemented with Claude Code under my review; every change was tested and reviewed by me. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> Co-authored-by: Alexander S. <sysix@sysix-coding.de>
This commit is contained in:
8 files changed
+151
-30
No files matched your search
@@ -575,9 +575,12 @@ impl Tool for ServerLinter {
|
||||
}
|
||||
|
||||
// We only run the lint process when the code action is explicitly invoked and the file is not open in the editor.
|
||||
let is_open = None;
|
||||
let actions =
|
||||
self.get_code_actions_for_uri(&uri, Some(CodeActionTriggerKind::Invoked), is_open);
|
||||
let document_version = None;
|
||||
let actions = self.get_code_actions_for_uri(
|
||||
&uri,
|
||||
Some(CodeActionTriggerKind::Invoked),
|
||||
document_version,
|
||||
);
|
||||
|
||||
let Some(actions) = actions else {
|
||||
return Ok(None);
|
||||
@@ -605,7 +608,7 @@ impl Tool for ServerLinter {
|
||||
let actions = self.get_code_actions_for_uri(
|
||||
¶ms.uri,
|
||||
params.context.trigger_kind,
|
||||
Some(params.is_open_document),
|
||||
params.document_version,
|
||||
);
|
||||
|
||||
let Some(actions) = actions else {
|
||||
@@ -755,7 +758,7 @@ impl ServerLinter {
|
||||
&self,
|
||||
uri: &Uri,
|
||||
trigger_kind: Option<CodeActionTriggerKind>,
|
||||
is_open: Option<bool>,
|
||||
document_version: Option<i32>,
|
||||
) -> Option<Vec<LinterCodeAction>> {
|
||||
if let Some(cached_code_actions) = self.code_actions.pin().get(uri) {
|
||||
Some(cached_code_actions.clone())
|
||||
@@ -764,7 +767,7 @@ impl ServerLinter {
|
||||
// otherwise it will be too heavy to run linting on every file open or cursor move, which will cause performance issues and a bad user experience.
|
||||
// It is most likely that the client already sent a request, where we run the lint process and cache the code actions.
|
||||
// So we only need to run the lint process when the code action is explicitly invoked and the file is not open in the editor.
|
||||
else if trigger_kind == Some(CodeActionTriggerKind::Invoked) && !is_open.unwrap_or(false)
|
||||
else if trigger_kind == Some(CodeActionTriggerKind::Invoked) && document_version.is_none()
|
||||
{
|
||||
let _ = self.run_file(uri, None);
|
||||
self.code_actions.pin().get(uri).cloned()
|
||||
@@ -1153,7 +1156,7 @@ mod test {
|
||||
range: Range,
|
||||
context: CodeActionContext,
|
||||
) -> CodeActionParams {
|
||||
CodeActionParams { uri: uri.clone(), range, context, is_open_document: false }
|
||||
CodeActionParams { uri: uri.clone(), range, context, document_version: None }
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1208,6 +1211,7 @@ mod test {
|
||||
let _ = linter
|
||||
.run_diagnostic(TextDocument {
|
||||
uri: &uri,
|
||||
version: None,
|
||||
language_id: LanguageId::default(),
|
||||
text: Some("let a = 1;".into()),
|
||||
})
|
||||
@@ -1249,6 +1253,7 @@ mod test {
|
||||
let _ = linter
|
||||
.run_diagnostic(TextDocument {
|
||||
uri: &uri,
|
||||
version: None,
|
||||
language_id: LanguageId::default(),
|
||||
text: Some("if (foo == NaN) {}".into()),
|
||||
})
|
||||
@@ -1333,6 +1338,7 @@ mod test {
|
||||
let _ = linter
|
||||
.run_diagnostic(TextDocument {
|
||||
uri: &uri,
|
||||
version: None,
|
||||
text: Some("debugger;".into()),
|
||||
language_id: LanguageId::default(),
|
||||
})
|
||||
|
||||
@@ -237,7 +237,7 @@ impl Tester<'_> {
|
||||
uri: uri.clone(),
|
||||
range,
|
||||
context: context.clone(),
|
||||
is_open_document: false,
|
||||
document_version: None,
|
||||
},
|
||||
),
|
||||
fix_all_action: linter
|
||||
@@ -245,7 +245,7 @@ impl Tester<'_> {
|
||||
uri: uri.clone(),
|
||||
range,
|
||||
context: fix_all_context.clone(),
|
||||
is_open_document: false,
|
||||
document_version: None,
|
||||
})
|
||||
.into_iter()
|
||||
.next(),
|
||||
|
||||
@@ -635,7 +635,7 @@ impl LanguageServer for Backend {
|
||||
}
|
||||
})
|
||||
{
|
||||
self.file_system.set(uri.clone(), content);
|
||||
self.file_system.set_with_version(uri.clone(), params.text_document.version, content);
|
||||
}
|
||||
|
||||
let document = self.file_system.get_document(&uri);
|
||||
@@ -703,6 +703,7 @@ impl LanguageServer for Backend {
|
||||
self.file_system.set_with_language(
|
||||
uri.clone(),
|
||||
LanguageId::new(params.text_document.language_id.to_string()),
|
||||
params.text_document.version,
|
||||
content,
|
||||
);
|
||||
|
||||
@@ -796,13 +797,13 @@ impl LanguageServer for Backend {
|
||||
return Ok(None);
|
||||
};
|
||||
|
||||
let is_open_document = self.file_system.is_open(&uri);
|
||||
let document_version = self.file_system.get_version(&uri);
|
||||
|
||||
let params = crate::CodeActionParams {
|
||||
uri,
|
||||
range: params.range,
|
||||
context: params.context,
|
||||
is_open_document,
|
||||
document_version,
|
||||
};
|
||||
|
||||
let code_actions = worker.get_code_actions_or_commands(params).await;
|
||||
|
||||
@@ -11,7 +11,14 @@ use crate::{ConcurrentHashMap, LanguageId, TextDocument, uri_utils::uri_to_file_
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
pub struct LSPFileSystem {
|
||||
files: ConcurrentHashMap<Uri, (LanguageId, Arc<str>)>,
|
||||
files: ConcurrentHashMap<Uri, OpenDocument>,
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
struct OpenDocument {
|
||||
language_id: LanguageId,
|
||||
version: i32,
|
||||
text: Arc<str>,
|
||||
}
|
||||
|
||||
/// Represents a resolved file path that can be used for file system operations.
|
||||
@@ -80,33 +87,47 @@ impl LSPFileSystem {
|
||||
}
|
||||
|
||||
pub fn set(&self, uri: Uri, content: String) {
|
||||
let language_id = self.get_language_id(&uri).unwrap_or_default();
|
||||
self.files.pin().insert(uri, (language_id, Arc::from(content)));
|
||||
let version = self.get_version(&uri).unwrap_or_default();
|
||||
self.set_with_version(uri, version, content);
|
||||
}
|
||||
|
||||
pub fn set_with_language(&self, uri: Uri, language_id: LanguageId, content: String) {
|
||||
self.files.pin().insert(uri, (language_id, Arc::from(content)));
|
||||
pub fn set_with_version(&self, uri: Uri, version: i32, content: String) {
|
||||
let language_id = self.get_language_id(&uri).unwrap_or_default();
|
||||
self.set_with_language(uri, language_id, version, content);
|
||||
}
|
||||
|
||||
pub fn set_with_language(
|
||||
&self,
|
||||
uri: Uri,
|
||||
language_id: LanguageId,
|
||||
version: i32,
|
||||
content: String,
|
||||
) {
|
||||
self.files
|
||||
.pin()
|
||||
.insert(uri, OpenDocument { language_id, version, text: Arc::from(content) });
|
||||
}
|
||||
|
||||
pub fn get_language_id(&self, uri: &Uri) -> Option<LanguageId> {
|
||||
self.files.pin().get(uri).map(|(lang, _)| lang.clone())
|
||||
self.files.pin().get(uri).map(|doc| doc.language_id.clone())
|
||||
}
|
||||
|
||||
pub fn get_version(&self, uri: &Uri) -> Option<i32> {
|
||||
self.files.pin().get(uri).map(|doc| doc.version)
|
||||
}
|
||||
|
||||
pub fn get_document<'a>(&self, uri: &'a Uri) -> TextDocument<'a> {
|
||||
self.files.pin().get(uri).map_or_else(
|
||||
|| TextDocument { uri, language_id: LanguageId::default(), text: None },
|
||||
|(language_id, content)| TextDocument {
|
||||
|| TextDocument { uri, language_id: LanguageId::default(), text: None, version: None },
|
||||
|doc| TextDocument {
|
||||
uri,
|
||||
language_id: language_id.clone(),
|
||||
text: Some(Arc::clone(content)),
|
||||
language_id: doc.language_id.clone(),
|
||||
text: Some(Arc::clone(&doc.text)),
|
||||
version: Some(doc.version),
|
||||
},
|
||||
)
|
||||
}
|
||||
|
||||
pub fn is_open(&self, uri: &Uri) -> bool {
|
||||
self.files.pin().contains_key(uri)
|
||||
}
|
||||
|
||||
pub fn remove(&self, uri: &Uri) {
|
||||
self.files.pin().remove(uri);
|
||||
}
|
||||
@@ -129,6 +150,10 @@ mod tests {
|
||||
PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("fixtures").join(fixture)
|
||||
}
|
||||
|
||||
fn uri_of_fixture(name: &str) -> Uri {
|
||||
file_path_to_uri(path_from_fixture("same_path_different_uri").join(name)).unwrap()
|
||||
}
|
||||
|
||||
fn path_to_lossy_string(path: &Path) -> Cow<'_, str> {
|
||||
path.as_os_str().to_string_lossy()
|
||||
}
|
||||
@@ -181,4 +206,46 @@ mod tests {
|
||||
.cow_replace("same_path_different_uri", "Same_Path_Different_URI")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_get_version() {
|
||||
let fs = LSPFileSystem::default();
|
||||
let uri = uri_of_fixture("test.txt");
|
||||
|
||||
assert_eq!(fs.get_version(&uri), None);
|
||||
assert_eq!(fs.get_document(&uri).version, None);
|
||||
|
||||
fs.set_with_language(uri.clone(), LanguageId::new("plaintext".to_string()), 4, "a".into());
|
||||
assert_eq!(fs.get_version(&uri), Some(4));
|
||||
assert_eq!(fs.get_document(&uri).version, Some(4));
|
||||
|
||||
fs.set_with_version(uri.clone(), 5, "b".into());
|
||||
assert_eq!(fs.get_version(&uri), Some(5));
|
||||
assert_eq!(fs.get_document(&uri).text.as_deref(), Some("b"));
|
||||
assert_eq!(fs.get_language_id(&uri), Some(LanguageId::new("plaintext".to_string())));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_set_keeps_the_version() {
|
||||
let fs = LSPFileSystem::default();
|
||||
let uri = uri_of_fixture("test.txt");
|
||||
|
||||
fs.set_with_language(uri.clone(), LanguageId::default(), 7, "a".into());
|
||||
fs.set(uri.clone(), "b".into());
|
||||
|
||||
assert_eq!(fs.get_version(&uri), Some(7));
|
||||
assert_eq!(fs.get_document(&uri).text.as_deref(), Some("b"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_remove_drops_the_version() {
|
||||
let fs = LSPFileSystem::default();
|
||||
let uri = uri_of_fixture("test.txt");
|
||||
|
||||
fs.set_with_version(uri.clone(), 2, "a".into());
|
||||
fs.remove(&uri);
|
||||
|
||||
assert_eq!(fs.get_version(&uri), None);
|
||||
assert_eq!(fs.get_document(&uri).version, None);
|
||||
}
|
||||
}
|
||||
@@ -41,11 +41,13 @@ pub struct TextDocument<'a> {
|
||||
pub uri: &'a Uri,
|
||||
pub language_id: LanguageId,
|
||||
pub text: Option<Arc<str>>,
|
||||
/// Version of `text` reported by the client, `None` when it is unknown, as for content read from disk.
|
||||
pub version: Option<i32>,
|
||||
}
|
||||
|
||||
impl<'a> TextDocument<'a> {
|
||||
pub fn new(uri: &'a Uri, language_id: LanguageId, text: Option<Arc<str>>) -> Self {
|
||||
Self { uri, language_id, text }
|
||||
Self { uri, language_id, text, version: None }
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -201,6 +201,15 @@ impl Tool for FakeTool {
|
||||
})];
|
||||
}
|
||||
|
||||
if params.uri.as_ref().ends_with("code_action_version.config") {
|
||||
return vec![CodeActionResponse::CodeAction(CodeAction {
|
||||
title: format!("Document version {:?}", params.document_version),
|
||||
kind: Some(CodeActionKind::QuickFix),
|
||||
edit: Some(WorkspaceEdit::default()),
|
||||
..Default::default()
|
||||
})];
|
||||
}
|
||||
|
||||
vec![]
|
||||
}
|
||||
|
||||
@@ -1909,6 +1918,41 @@ mod test_suite {
|
||||
server.shutdown(4).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_code_actions_receive_the_document_version() {
|
||||
let mut server = TestServer::new_initialized(
|
||||
|client| Backend::new(client, server_info(), create_workspace_manager()),
|
||||
initialize_request(InitializeRequestOptions::default()),
|
||||
)
|
||||
.await;
|
||||
|
||||
let file = format!("{WORKSPACE}/code_action_version.config");
|
||||
|
||||
// `did_open` reports version 1, `did_change` version 2 and `did_save` carries none.
|
||||
let mut id = 3;
|
||||
for (notification, expected) in [
|
||||
(None, "Document version None"),
|
||||
(Some(did_open(&file, "some text")), "Document version Some(1)"),
|
||||
(Some(did_change(&file, "changed text")), "Document version Some(2)"),
|
||||
(Some(did_save(&file, "saved text")), "Document version Some(2)"),
|
||||
(Some(did_close(&file)), "Document version None"),
|
||||
] {
|
||||
if let Some(notification) = notification {
|
||||
server.send_request(notification).await;
|
||||
}
|
||||
server.send_request(code_action(id, &file)).await;
|
||||
let response = server.recv_response().await;
|
||||
assert!(response.is_ok());
|
||||
assert_eq!(response.id(), &Id::Number(id));
|
||||
let actions: Vec<serde_json::Value> =
|
||||
serde_json::from_value(response.result().unwrap().clone()).unwrap();
|
||||
assert_eq!(actions[0]["title"], expected);
|
||||
id += 1;
|
||||
}
|
||||
|
||||
server.shutdown(id).await;
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_diagnostic_on_open() {
|
||||
let mut server = TestServer::new_initialized(
|
||||
|
||||
@@ -5,5 +5,6 @@ pub struct CodeActionParams {
|
||||
pub uri: Uri,
|
||||
pub range: Range,
|
||||
pub context: CodeActionContext,
|
||||
pub is_open_document: bool,
|
||||
/// Version of the open document, `None` when the document is not open.
|
||||
pub document_version: Option<i32>,
|
||||
}
|
||||
@@ -770,7 +770,7 @@ mod tests {
|
||||
uri: Uri::from_str("file:///root/file.js").unwrap(),
|
||||
range: Range::default(),
|
||||
context: CodeActionContext::default(),
|
||||
is_open_document: false,
|
||||
document_version: None,
|
||||
})
|
||||
.await;
|
||||
|
||||
@@ -781,7 +781,7 @@ mod tests {
|
||||
uri: Uri::from_str("file:///root/code_action.config").unwrap(),
|
||||
range: Range::default(),
|
||||
context: CodeActionContext::default(),
|
||||
is_open_document: false,
|
||||
document_version: None,
|
||||
})
|
||||
.await;
|
||||
|
||||
|
||||
Reference in new issue
Block a user