From e3bef21585bdc707723ff344722befee8ed4c7e1 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Fri, 18 Jul 2025 19:45:58 +0200 Subject: [PATCH 1/2] fix: splitter crash --- crates/pgt_lsp/src/handlers/text_document.rs | 14 +- crates/pgt_lsp/src/utils.rs | 12 +- crates/pgt_lsp/tests/server.rs | 487 ++++++++++++++++++ crates/pgt_statement_splitter/src/lib.rs | 8 + .../src/splitter/common.rs | 4 +- 5 files changed, 510 insertions(+), 15 deletions(-) diff --git a/crates/pgt_lsp/src/handlers/text_document.rs b/crates/pgt_lsp/src/handlers/text_document.rs index cc2efb4b..1c5a9a11 100644 --- a/crates/pgt_lsp/src/handlers/text_document.rs +++ b/crates/pgt_lsp/src/handlers/text_document.rs @@ -1,12 +1,10 @@ -use crate::{ - diagnostics::LspError, documents::Document, session::Session, utils::apply_document_changes, -}; +use crate::{documents::Document, session::Session, utils::apply_document_changes}; use anyhow::Result; use pgt_workspace::workspace::{ ChangeFileParams, CloseFileParams, GetFileContentParams, OpenFileParams, }; use tower_lsp::lsp_types; -use tracing::error; +use tracing::{error, field}; /// Handler for `textDocument/didOpen` LSP notification #[tracing::instrument(level = "debug", skip(session), err)] @@ -36,12 +34,12 @@ pub(crate) async fn did_open( Ok(()) } -// Handler for `textDocument/didChange` LSP notification -#[tracing::instrument(level = "debug", skip(session), err)] +/// 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)] pub(crate) async fn did_change( session: &Session, params: lsp_types::DidChangeTextDocumentParams, -) -> Result<(), LspError> { +) -> Result<()> { let url = params.text_document.uri; let version = params.text_document.version; @@ -56,7 +54,7 @@ pub(crate) async fn did_change( let text = apply_document_changes( session.position_encoding(), old_text, - ¶ms.content_changes, + params.content_changes, ); tracing::trace!("new document: {:?}", text); diff --git a/crates/pgt_lsp/src/utils.rs b/crates/pgt_lsp/src/utils.rs index 92059b66..b483e4b7 100644 --- a/crates/pgt_lsp/src/utils.rs +++ b/crates/pgt_lsp/src/utils.rs @@ -1,3 +1,4 @@ +use crate::adapters::from_lsp::text_range; use crate::adapters::line_index::LineIndex; use crate::adapters::{PositionEncoding, from_lsp, to_lsp}; use anyhow::{Context, Result, ensure}; @@ -10,8 +11,8 @@ use pgt_text_size::{TextRange, TextSize}; use std::any::Any; use std::borrow::Cow; use std::fmt::{Debug, Display}; -use std::io; use std::ops::{Add, Range}; +use std::{io, mem}; use tower_lsp::jsonrpc::Error as LspError; use tower_lsp::lsp_types; use tower_lsp::lsp_types::{self as lsp, CodeDescription, Url}; @@ -183,7 +184,7 @@ pub(crate) fn panic_to_lsp_error(err: Box) -> LspError { pub(crate) fn apply_document_changes( position_encoding: PositionEncoding, current_content: String, - content_changes: &[lsp_types::TextDocumentContentChangeEvent], + mut content_changes: Vec, ) -> String { // Skip to the last full document change, as it invalidates all previous changes anyways. let mut start = content_changes @@ -192,12 +193,12 @@ pub(crate) fn apply_document_changes( .position(|change| change.range.is_none()) .map_or(0, |idx| content_changes.len() - idx - 1); - let mut text: String = match content_changes.get(start) { + let mut text: String = match content_changes.get_mut(start) { // peek at the first content change as an optimization Some(lsp_types::TextDocumentContentChangeEvent { range: None, text, .. }) => { - let text = text.clone(); + let text = mem::take(text); start += 1; // The only change is a full document update @@ -225,12 +226,11 @@ pub(crate) fn apply_document_changes( line_index = LineIndex::new(&text); } index_valid = range.start.line; - if let Ok(range) = from_lsp::text_range(&line_index, range, position_encoding) { + if let Ok(range) = text_range(&line_index, range, position_encoding) { text.replace_range(Range::::from(range), &change.text); } } } - text } diff --git a/crates/pgt_lsp/tests/server.rs b/crates/pgt_lsp/tests/server.rs index 176868f5..63953590 100644 --- a/crates/pgt_lsp/tests/server.rs +++ b/crates/pgt_lsp/tests/server.rs @@ -1773,3 +1773,490 @@ $$; Ok(()) } + +#[tokio::test] +async fn test_crash_on_delete_character() -> Result<()> { + let factory = ServerFactory::default(); + let (service, client) = factory.create(None).into_inner(); + let (stream, sink) = client.split(); + let mut server = Server::new(service); + + let (sender, _) = channel(CHANNEL_BUFFER_SIZE); + let reader = tokio::spawn(client_handler(stream, sink, sender)); + + server.initialize().await?; + server.initialized().await?; + + // Open document with initial CREATE INDEX statement - exactly as in log + let initial_content = "\n\n\n\nCREATE INDEX \"idx_analytics_read_ratio\" ON \"public\".\"message\" USING \"btree\" (\"inbox_id\", \"timestamp\") INCLUDE (\"status\") WHERE (\"is_inbound\" = false);\n"; + + server.open_document(initial_content).await?; + + // Add a space after false (position 148 from the log) + server + .change_document( + 3, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 148, + }, + end: Position { + line: 4, + character: 148, + }, + }), + range_length: Some(0), + text: " ".to_string(), + }], + ) + .await?; + + // Follow the exact sequence from the logfile + // Type character by character in exact order + + // Version 4: "a" at 149 + server + .change_document( + 4, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 149, + }, + end: Position { + line: 4, + character: 149, + }, + }), + range_length: Some(0), + text: "a".to_string(), + }], + ) + .await?; + + // Version 5: "n" at 150 + server + .change_document( + 5, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 150, + }, + end: Position { + line: 4, + character: 150, + }, + }), + range_length: Some(0), + text: "n".to_string(), + }], + ) + .await?; + + // Version 6: "d" at 151 + server + .change_document( + 6, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 151, + }, + end: Position { + line: 4, + character: 151, + }, + }), + range_length: Some(0), + text: "d".to_string(), + }], + ) + .await?; + + // Version 7: " " at 152 + server + .change_document( + 7, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 152, + }, + end: Position { + line: 4, + character: 152, + }, + }), + range_length: Some(0), + text: " ".to_string(), + }], + ) + .await?; + + // Version 8: "c" at 153 + server + .change_document( + 8, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 153, + }, + end: Position { + line: 4, + character: 153, + }, + }), + range_length: Some(0), + text: "c".to_string(), + }], + ) + .await?; + + // Version 10: "h" at 154 and "a" at 155 (two changes in one version) + server + .change_document( + 10, + vec![ + TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 154, + }, + end: Position { + line: 4, + character: 154, + }, + }), + range_length: Some(0), + text: "h".to_string(), + }, + TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 155, + }, + end: Position { + line: 4, + character: 155, + }, + }), + range_length: Some(0), + text: "a".to_string(), + }, + ], + ) + .await?; + + // Version 11: "n" at 156 + server + .change_document( + 11, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 156, + }, + end: Position { + line: 4, + character: 156, + }, + }), + range_length: Some(0), + text: "n".to_string(), + }], + ) + .await?; + + // Version 12: "n" at 157 + server + .change_document( + 12, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 157, + }, + end: Position { + line: 4, + character: 157, + }, + }), + range_length: Some(0), + text: "n".to_string(), + }], + ) + .await?; + + // Version 13: "e" at 158 + server + .change_document( + 13, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 158, + }, + end: Position { + line: 4, + character: 158, + }, + }), + range_length: Some(0), + text: "e".to_string(), + }], + ) + .await?; + + // Version 14: "l" at 159 + server + .change_document( + 14, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 159, + }, + end: Position { + line: 4, + character: 159, + }, + }), + range_length: Some(0), + text: "l".to_string(), + }], + ) + .await?; + + // Version 15: "_" at 160 + server + .change_document( + 15, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 160, + }, + end: Position { + line: 4, + character: 160, + }, + }), + range_length: Some(0), + text: "_".to_string(), + }], + ) + .await?; + + // Version 16: "t" at 161 + server + .change_document( + 16, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 161, + }, + end: Position { + line: 4, + character: 161, + }, + }), + range_length: Some(0), + text: "t".to_string(), + }], + ) + .await?; + + // Version 17: "y" at 162 + server + .change_document( + 17, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 162, + }, + end: Position { + line: 4, + character: 162, + }, + }), + range_length: Some(0), + text: "y".to_string(), + }], + ) + .await?; + + // Version 18: "p" at 163 + server + .change_document( + 18, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 163, + }, + end: Position { + line: 4, + character: 163, + }, + }), + range_length: Some(0), + text: "p".to_string(), + }], + ) + .await?; + + // Version 19: "e" at 164 + server + .change_document( + 19, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 164, + }, + end: Position { + line: 4, + character: 164, + }, + }), + range_length: Some(0), + text: "e".to_string(), + }], + ) + .await?; + + // Version 20: " " at 165 + server + .change_document( + 20, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 165, + }, + end: Position { + line: 4, + character: 165, + }, + }), + range_length: Some(0), + text: " ".to_string(), + }], + ) + .await?; + + // Now we should have: "WHERE ("is_inbound" = false and channel_type )" + + // Version 21: Paste the problematic text with double single quotes + server + .change_document( + 21, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 166, + }, + end: Position { + line: 4, + character: 166, + }, + }), + range_length: Some(0), + text: "channel_type not in (''postal'', ''sms'')".to_string(), + }], + ) + .await?; + + // Delete "channel_type" + server + .change_document( + 22, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 166, + }, + end: Position { + line: 4, + character: 178, + }, + }), + range_length: Some(12), + text: "".to_string(), + }], + ) + .await?; + + // Delete one more character + server + .change_document( + 23, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 166, + }, + end: Position { + line: 4, + character: 167, + }, + }), + range_length: Some(1), + text: "".to_string(), + }], + ) + .await?; + + // This final delete should trigger the panic + let result = server + .change_document( + 24, + vec![TextDocumentContentChangeEvent { + range: Some(Range { + start: Position { + line: 4, + character: 175, + }, + end: Position { + line: 4, + character: 176, + }, + }), + range_length: Some(1), + text: "".to_string(), + }], + ) + .await; + + assert!(result.is_ok()); + + reader.abort(); + + Ok(()) +} diff --git a/crates/pgt_statement_splitter/src/lib.rs b/crates/pgt_statement_splitter/src/lib.rs index 19b2f230..02ca1b30 100644 --- a/crates/pgt_statement_splitter/src/lib.rs +++ b/crates/pgt_statement_splitter/src/lib.rs @@ -114,6 +114,14 @@ mod tests { ); } + #[test] + fn test_crash_eof() { + Tester::from("CREATE INDEX \"idx_analytics_read_ratio\" ON \"public\".\"message\" USING \"btree\" (\"inbox_id\", \"timestamp\") INCLUDE (\"status\") WHERE (\"is_inbound\" = false and channel_type not in ('postal'', 'sms'));") + .expect_statements(vec![ + "CREATE INDEX \"idx_analytics_read_ratio\" ON \"public\".\"message\" USING \"btree\" (\"inbox_id\", \"timestamp\") INCLUDE (\"status\") WHERE (\"is_inbound\" = false and channel_type not in ('postal'', 'sms'));", + ]); + } + #[test] #[timeout(1000)] fn basic() { diff --git a/crates/pgt_statement_splitter/src/splitter/common.rs b/crates/pgt_statement_splitter/src/splitter/common.rs index 9c3dea48..786c2478 100644 --- a/crates/pgt_statement_splitter/src/splitter/common.rs +++ b/crates/pgt_statement_splitter/src/splitter/common.rs @@ -70,7 +70,9 @@ pub(crate) fn parenthesis(p: &mut Splitter) { depth += 1; } SyntaxKind::R_PAREN | SyntaxKind::EOF => { - p.advance(); + if p.current() == SyntaxKind::R_PAREN { + p.advance(); + } depth -= 1; if depth == 0 { break; From 8dc7d134a127f7dd0c4d3dbbdbbfde6ee04450a3 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Fri, 18 Jul 2025 19:54:05 +0200 Subject: [PATCH 2/2] progress --- crates/pgt_lsp/src/utils.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/pgt_lsp/src/utils.rs b/crates/pgt_lsp/src/utils.rs index b483e4b7..8361cf08 100644 --- a/crates/pgt_lsp/src/utils.rs +++ b/crates/pgt_lsp/src/utils.rs @@ -1,6 +1,6 @@ use crate::adapters::from_lsp::text_range; use crate::adapters::line_index::LineIndex; -use crate::adapters::{PositionEncoding, from_lsp, to_lsp}; +use crate::adapters::{PositionEncoding, to_lsp}; use anyhow::{Context, Result, ensure}; use pgt_console::MarkupBuf; use pgt_console::fmt::Termcolor;