From caf91a5d0918c9032c09b42d71d37c81e4afef7d Mon Sep 17 00:00:00 2001 From: psteinroe Date: Tue, 11 Mar 2025 14:03:44 +0100 Subject: [PATCH 1/3] fix: lock --- crates/pglt_lsp/src/handlers/text_document.rs | 11 ++--------- crates/pglt_workspace/src/workspace/server.rs | 2 ++ .../src/workspace/server/schema_cache_manager.rs | 14 +++++++++++--- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/crates/pglt_lsp/src/handlers/text_document.rs b/crates/pglt_lsp/src/handlers/text_document.rs index d777cf65d..78279cd11 100644 --- a/crates/pglt_lsp/src/handlers/text_document.rs +++ b/crates/pglt_lsp/src/handlers/text_document.rs @@ -10,14 +10,7 @@ use tower_lsp::lsp_types; use tracing::{error, field}; /// Handler for `textDocument/didOpen` LSP notification -#[tracing::instrument( - level = "debug", - skip_all, - fields( - text_document_uri = display(params.text_document.uri.as_ref()), - text_document_language_id = display(¶ms.text_document.language_id), - ) -)] +#[tracing::instrument(level = "debug", skip(session), err)] pub(crate) async fn did_open( session: &Session, params: lsp_types::DidOpenTextDocumentParams, @@ -45,7 +38,7 @@ pub(crate) async fn did_open( } // Handler for `textDocument/didChange` LSP notification -#[tracing::instrument(level = "debug", skip_all, fields(url = field::display(¶ms.text_document.uri), version = params.text_document.version), err)] +#[tracing::instrument(level = "debug", skip(session), err)] pub(crate) async fn did_change( session: &Session, params: lsp_types::DidChangeTextDocumentParams, diff --git a/crates/pglt_workspace/src/workspace/server.rs b/crates/pglt_workspace/src/workspace/server.rs index 4e15158c5..0db5023d5 100644 --- a/crates/pglt_workspace/src/workspace/server.rs +++ b/crates/pglt_workspace/src/workspace/server.rs @@ -456,6 +456,8 @@ impl Workspace for WorkspaceServer { let schema_cache = self.schema_cache.load(pool)?; + tracing::debug!("Loaded schema cache for completions"); + let result = pglt_completions::complete(pglt_completions::CompletionParams { position, schema: schema_cache.as_ref(), diff --git a/crates/pglt_workspace/src/workspace/server/schema_cache_manager.rs b/crates/pglt_workspace/src/workspace/server/schema_cache_manager.rs index b2dad8575..d5c83e02f 100644 --- a/crates/pglt_workspace/src/workspace/server/schema_cache_manager.rs +++ b/crates/pglt_workspace/src/workspace/server/schema_cache_manager.rs @@ -47,7 +47,9 @@ impl SchemaCacheManager { { // return early if the connection string is the same let inner = self.inner.read().unwrap(); + tracing::debug!("Current connection string: {}", inner.conn_str); if new_conn_str == inner.conn_str { + tracing::debug!("Connection string is the same, returning cached schema"); return Ok(SchemaCacheHandle::wrap(inner)); } } @@ -55,10 +57,16 @@ impl SchemaCacheManager { let maybe_refreshed = run_async(async move { SchemaCache::load(&pool).await })?; let refreshed = maybe_refreshed?; - let mut inner = self.inner.write().unwrap(); + { + // write lock must be dropped before we return the reference below, hence the block + let mut inner = self.inner.write().unwrap(); - inner.cache = refreshed; - inner.conn_str = new_conn_str; + // Double-check that we still need to refresh (another thread might have done it) + if new_conn_str != inner.conn_str { + inner.cache = refreshed; + inner.conn_str = new_conn_str; + } + } Ok(SchemaCacheHandle::new(&self.inner)) } From fc5da107c4a4b27ec8def901a19c410099968652 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Tue, 11 Mar 2025 14:19:16 +0100 Subject: [PATCH 2/3] cleanup --- .../pglt_workspace/src/workspace/server/schema_cache_manager.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/pglt_workspace/src/workspace/server/schema_cache_manager.rs b/crates/pglt_workspace/src/workspace/server/schema_cache_manager.rs index d5c83e02f..e1f10c1df 100644 --- a/crates/pglt_workspace/src/workspace/server/schema_cache_manager.rs +++ b/crates/pglt_workspace/src/workspace/server/schema_cache_manager.rs @@ -47,9 +47,7 @@ impl SchemaCacheManager { { // return early if the connection string is the same let inner = self.inner.read().unwrap(); - tracing::debug!("Current connection string: {}", inner.conn_str); if new_conn_str == inner.conn_str { - tracing::debug!("Connection string is the same, returning cached schema"); return Ok(SchemaCacheHandle::wrap(inner)); } } From 41f0df6e7af286ddcc789f72e87ce1c869d047b8 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Tue, 11 Mar 2025 14:58:36 +0100 Subject: [PATCH 3/3] fix: whitespace changes --- crates/pglt_workspace/src/workspace/server.rs | 8 +- .../src/workspace/server/change.rs | 132 ++++++++++++++---- 2 files changed, 106 insertions(+), 34 deletions(-) diff --git a/crates/pglt_workspace/src/workspace/server.rs b/crates/pglt_workspace/src/workspace/server.rs index 0db5023d5..4d3d7810f 100644 --- a/crates/pglt_workspace/src/workspace/server.rs +++ b/crates/pglt_workspace/src/workspace/server.rs @@ -216,26 +216,26 @@ impl Workspace for WorkspaceServer { params.version, )); - tracing::info!("Changing file: {:?}", params.path); + tracing::info!("Changing file: {:?}", params); for c in &doc.apply_file_change(¶ms) { match c { StatementChange::Added(added) => { - tracing::info!("Adding statement: {:?}", added); + tracing::debug!("Adding statement: {:?}", added); self.tree_sitter.add_statement(&added.stmt, &added.text); self.pg_query.add_statement(&added.stmt, &added.text); self.changed_stmts.insert(added.stmt.clone()); } StatementChange::Deleted(s) => { - tracing::info!("Deleting statement: {:?}", s); + tracing::debug!("Deleting statement: {:?}", s); self.tree_sitter.remove_statement(s); self.pg_query.remove_statement(s); self.changed_stmts.remove(s); } StatementChange::Modified(s) => { - tracing::info!("Modifying statement: {:?}", s); + tracing::debug!("Modifying statement: {:?}", s); self.tree_sitter.modify_statement(s); self.pg_query.modify_statement(s); diff --git a/crates/pglt_workspace/src/workspace/server/change.rs b/crates/pglt_workspace/src/workspace/server/change.rs index 1b7625cba..75915f853 100644 --- a/crates/pglt_workspace/src/workspace/server/change.rs +++ b/crates/pglt_workspace/src/workspace/server/change.rs @@ -268,18 +268,6 @@ impl Document { } if new_ranges.len() == 1 { - if change.is_whitespace() { - self.move_ranges( - affected_range.end(), - change.diff_size(), - change.is_addition(), - ); - - self.content = new_content; - - return changed; - } - let affected_idx = affected_indices[0]; let new_range = new_ranges[0].add(affected_range.start()); let (old_id, old_range) = self.positions[affected_idx]; @@ -290,22 +278,25 @@ impl Document { let new_id = self.id_generator.next(); self.positions[affected_idx] = (new_id, new_range); - changed.push(StatementChange::Modified(ModifiedStatement { - old_stmt: Statement { - id: old_id, - path: self.path.clone(), - }, - old_stmt_text: self.content[old_range].to_string(), - - new_stmt: Statement { - id: new_id, - path: self.path.clone(), - }, - new_stmt_text: changed_content[new_ranges[0]].to_string(), - // change must be relative to the statement - change_text: change.text.clone(), - change_range: change_range.sub(old_range.start()), - })); + if !change.is_whitespace() { + // whitespace-only changes should not invalidate the statement + changed.push(StatementChange::Modified(ModifiedStatement { + old_stmt: Statement { + id: old_id, + path: self.path.clone(), + }, + old_stmt_text: self.content[old_range].to_string(), + + new_stmt: Statement { + id: new_id, + path: self.path.clone(), + }, + new_stmt_text: changed_content[new_ranges[0]].to_string(), + // change must be relative to the statement + change_text: change.text.clone(), + change_range: change_range.sub(old_range.start()), + })); + } self.content = new_content; @@ -447,12 +438,16 @@ mod tests { .expect("Unexpected scan error") .ranges; - assert!(ranges.len() == d.positions.len()); + assert!( + ranges.len() == d.positions.len(), + "should have the correct amount of positions" + ); assert!( ranges .iter() - .all(|r| { d.positions.iter().any(|(_, stmt_range)| stmt_range == r) }) + .all(|r| { d.positions.iter().any(|(_, stmt_range)| stmt_range == r) }), + "all ranges should be in positions" ); } @@ -648,6 +643,83 @@ mod tests { assert_document_integrity(&d); } + #[test] + fn within_statements_2() { + let path = PgLTPath::new("test.sql"); + let input = "alter table deal alter column value drop not null;\n"; + let mut d = Document::new(path.clone(), input.to_string(), 0); + + assert_eq!(d.positions.len(), 1); + + let change1 = ChangeFileParams { + path: path.clone(), + version: 1, + changes: vec![ChangeParams { + text: " ".to_string(), + range: Some(TextRange::new(17.into(), 17.into())), + }], + }; + + let changed1 = d.apply_file_change(&change1); + assert_eq!(changed1.len(), 0, "should not emit change"); + assert_eq!( + d.content, + "alter table deal alter column value drop not null;\n" + ); + assert_document_integrity(&d); + + let change2 = ChangeFileParams { + path: path.clone(), + version: 2, + changes: vec![ChangeParams { + text: " ".to_string(), + range: Some(TextRange::new(18.into(), 18.into())), + }], + }; + + let changed2 = d.apply_file_change(&change2); + assert_eq!(changed2.len(), 0); + assert_eq!( + d.content, + "alter table deal alter column value drop not null;\n" + ); + assert_document_integrity(&d); + + let change3 = ChangeFileParams { + path: path.clone(), + version: 3, + changes: vec![ChangeParams { + text: " ".to_string(), + range: Some(TextRange::new(19.into(), 19.into())), + }], + }; + + let changed3 = d.apply_file_change(&change3); + assert_eq!(changed3.len(), 0); + assert_eq!( + d.content, + "alter table deal alter column value drop not null;\n" + ); + assert_document_integrity(&d); + + let change4 = ChangeFileParams { + path: path.clone(), + version: 4, + changes: vec![ChangeParams { + text: " ".to_string(), + range: Some(TextRange::new(20.into(), 20.into())), + }], + }; + + let changed4 = d.apply_file_change(&change4); + assert_eq!(changed4.len(), 0); + assert_eq!( + d.content, + "alter table deal alter column value drop not null;\n" + ); + assert_document_integrity(&d); + } + #[test] fn julians_sample() { let path = PgLTPath::new("test.sql");