Skip to content

fix: whitespace-only change #230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 12, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions crates/pglt_lsp/src/handlers/text_document.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(&params.text_document.language_id),
)
)]
#[tracing::instrument(level = "debug", skip(session), err)]
pub(crate) async fn did_open(
session: &Session,
params: lsp_types::DidOpenTextDocumentParams,
Expand Down Expand Up @@ -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(&params.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,
Expand Down
10 changes: 6 additions & 4 deletions crates/pglt_workspace/src/workspace/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(&params) {
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);

Expand Down Expand Up @@ -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(),
Expand Down
132 changes: 102 additions & 30 deletions crates/pglt_workspace/src/workspace/server/change.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand All @@ -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;

Expand Down Expand Up @@ -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"
);
}

Expand Down Expand Up @@ -648,6 +643,83 @@ mod tests {
assert_document_integrity(&d);
}

#[test]
fn within_statements_2() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test was failing before my fix because of document integrity

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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,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))
}
Expand Down