From 079d8ef9701f8bc087515660a38b334505075567 Mon Sep 17 00:00:00 2001 From: edjubert Date: Tue, 8 Sep 2026 18:14:21 +0200 Subject: [PATCH 1/7] fix(lexer): classify named parameters by SQL context --- crates/pgls_lexer/src/params.rs | 180 ++++++++++++++++++++++++++------ 1 file changed, 147 insertions(+), 33 deletions(-) diff --git a/crates/pgls_lexer/src/params.rs b/crates/pgls_lexer/src/params.rs index b2924ac16..3d0a48f08 100644 --- a/crates/pgls_lexer/src/params.rs +++ b/crates/pgls_lexer/src/params.rs @@ -23,18 +23,18 @@ const IDENTIFIER_CONTEXT: [SyntaxKind; 15] = [ SyntaxKind::DOT, ]; -/// Converts named parameters in a SQL query string to positional parameters. +/// Converts named parameters in a SQL query string to parser-compatible placeholders. /// -/// This function scans the input SQL string for named parameters (e.g., `@param`, `:param`, `:'param'`) -/// and replaces them with positional parameters (e.g., `$1`, `$2`, etc.). -/// -/// It maintains the original spacing of the named parameters in the output string. -/// -/// Useful for preparing SQL queries for parsing or execution where named paramters are not supported. +/// Replacements preserve the source byte length so parser diagnostics can be mapped directly +/// back to the original query. A raw colon inside brackets is left untouched because it may be +/// PostgreSQL array-slice syntax rather than a named parameter. pub fn convert_to_positional_params(text: &str) -> String { let mut result = String::with_capacity(text.len()); - let mut param_mapping: HashMap<&str, usize> = HashMap::new(); - let mut param_index = 1; + let mut value_params: HashMap<&str, usize> = HashMap::new(); + let mut identifier_params: HashMap<&str, usize> = HashMap::new(); + let mut value_index = 1; + let mut identifier_index = 0; + let mut bracket_depth = 0_usize; let lexed = lex(text); for (token_idx, kind) in lexed.tokens().enumerate() { @@ -45,43 +45,105 @@ pub fn convert_to_positional_params(text: &str) -> String { let token_text = lexed.text(token_idx); if matches!(kind, SyntaxKind::NAMED_PARAM) { - let idx = match param_mapping.get(token_text) { - Some(&index) => index, - None => { - let index = param_index; - param_mapping.insert(token_text, index); - param_index += 1; - index + let flavor = NamedParamFlavor::from_text(token_text); + let previous = previous_non_trivia_kind(&lexed, token_idx); + let next = next_non_trivia_kind(&lexed, token_idx); + let is_identifier = match flavor { + Some(NamedParamFlavor::ColonRaw) if bracket_depth > 0 => { + result.push_str(token_text); + continue; } + Some(NamedParamFlavor::ColonIdentifier) => true, + _ if next == Some(SyntaxKind::DOT) => true, + _ if previous.is_some_and(|kind| IDENTIFIER_CONTEXT.contains(&kind)) => true, + _ => false, }; - // find previous non-trivia token - let prev_token = (0..token_idx) - .rev() - .map(|i| lexed.kind(i)) - .find(|kind| !kind.is_trivia()); - - let replacement = match prev_token { - Some(k) if IDENTIFIER_CONTEXT.contains(&k) => deterministic_identifier(idx - 1), - _ => format!("${idx}"), + let replacement = if is_identifier { + let index = *identifier_params.entry(token_text).or_insert_with(|| { + let index = identifier_index; + identifier_index += 1; + index + }); + identifier_replacement(index, token_text.len()) + } else { + let index = *value_params.entry(token_text).or_insert_with(|| { + let index = value_index; + value_index += 1; + index + }); + format!("${index}") }; - let original_len = token_text.len(); - let replacement_len = replacement.len(); - - result.push_str(&replacement); - // maintain original spacing - if replacement_len < original_len { - result.push_str(&" ".repeat(original_len - replacement_len)); - } + push_padded_replacement(&mut result, &replacement, token_text.len()); } else { result.push_str(token_text); } + + match kind { + SyntaxKind::L_BRACK => bracket_depth += 1, + SyntaxKind::R_BRACK => bracket_depth = bracket_depth.saturating_sub(1), + _ => {} + } } + debug_assert_eq!(result.len(), text.len()); result } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum NamedParamFlavor { + AtPrefix, + DollarRaw, + ColonRaw, + ColonString, + ColonIdentifier, +} + +impl NamedParamFlavor { + fn from_text(text: &str) -> Option { + match text.as_bytes().first()? { + b'@' => Some(Self::AtPrefix), + b'$' => Some(Self::DollarRaw), + b':' if text.starts_with(":'") => Some(Self::ColonString), + b':' if text.starts_with(":\"") => Some(Self::ColonIdentifier), + b':' => Some(Self::ColonRaw), + _ => None, + } + } +} + +fn previous_non_trivia_kind(lexed: &crate::Lexed<'_>, token_idx: usize) -> Option { + (0..token_idx) + .rev() + .map(|idx| lexed.kind(idx)) + .find(|kind| !kind.is_trivia()) +} + +fn next_non_trivia_kind(lexed: &crate::Lexed<'_>, token_idx: usize) -> Option { + (token_idx + 1..lexed.len()) + .map(|idx| lexed.kind(idx)) + .find(|kind| !kind.is_trivia() && *kind != SyntaxKind::EOF) +} + +fn push_padded_replacement(result: &mut String, replacement: &str, original_len: usize) { + assert!( + replacement.len() <= original_len, + "named parameter replacement must preserve source length" + ); + result.push_str(replacement); + result.push_str(&" ".repeat(original_len - replacement.len())); +} + +fn identifier_replacement(index: usize, max_len: usize) -> String { + let identifier = deterministic_identifier(index); + if identifier.len() <= max_len { + identifier + } else { + "a".to_string() + } +} + const ALPHABET: [char; 26] = [ 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z', @@ -135,4 +197,56 @@ mod tests { "select * from users where first_name = $1 and starts_with(email, $1 ) and created_at > $2 ;" ); } + + #[test] + fn classifies_named_parameter_contexts() { + let cases = [ + ( + "select * from :schema.items where id = :id", + "select * from a .items where id = $1 ", + ), + ( + "select * from @schema.items where id = $id", + "select * from a .items where id = $1 ", + ), + ( + "select * from $schema.items where id = @id", + "select * from a .items where id = $1 ", + ), + ( + "select * from :\"schema\".items", + "select * from a .items", + ), + ( + "cross join :raw_data.migration_infos", + "cross join a .migration_infos", + ), + ]; + + for (input, expected) in cases { + let normalized = convert_to_positional_params(input); + assert_eq!(normalized, expected); + assert_eq!(normalized.len(), input.len()); + } + } + + #[test] + fn preserves_array_slices_and_keeps_parameter_indexes_contiguous() { + let input = "select arr[3:array_upper(arr, 1)], :value from :schema.items where id = :id"; + let normalized = convert_to_positional_params(input); + + assert!(normalized.contains("arr[3:array_upper(arr, 1)]")); + assert!(normalized.contains("$1 ")); + assert!(normalized.contains("$2 ")); + assert_eq!(normalized.len(), input.len()); + } + + #[test] + fn preserves_quoted_and_at_parameters_in_brackets() { + let input = "select arr[:'value'], arr[@value], arr[$value]"; + let normalized = convert_to_positional_params(input); + + assert_eq!(normalized, "select arr[$1 ], arr[$2 ], arr[$3 ]"); + assert_eq!(normalized.len(), input.len()); + } } From d0d9dff43514ec1458adcf76a8c04293e351a9ad Mon Sep 17 00:00:00 2001 From: edjubert Date: Tue, 8 Sep 2026 18:14:22 +0200 Subject: [PATCH 2/7] test(workspace): cover named qualifier and array slice parsing --- .../src/workspace/server/pg_query.rs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/crates/pgls_workspace/src/workspace/server/pg_query.rs b/crates/pgls_workspace/src/workspace/server/pg_query.rs index 09545e7b8..d27bd8996 100644 --- a/crates/pgls_workspace/src/workspace/server/pg_query.rs +++ b/crates/pgls_workspace/src/workspace/server/pg_query.rs @@ -125,6 +125,40 @@ mod tests { assert!(res.is_ok()); } + #[test] + fn parses_named_schema_qualifiers() { + let cases = [ + r#" +SELECT + customers.id +FROM staging.customers +CROSS JOIN :raw_data.migration_infos; +"#, + "SELECT * FROM @schema.items;", + "SELECT * FROM $schema.items;", + r#"SELECT * FROM :"schema".items;"#, + "SELECT * FROM :table;", + ]; + + let store = PgQueryStore::new(); + for input in cases { + let result = store.get_or_cache_ast(&StatementId::new(input)); + assert!(result.is_ok(), "failed to parse {input:?}: {result:?}"); + } + } + + #[test] + fn preserves_array_slice_syntax_when_normalizing_parameters() { + let input = "SELECT array_to_string(arr[3:array_upper(arr, 1)], ',') FROM t;"; + let normalized = convert_to_positional_params(input); + + assert_eq!(normalized, input); + + let store = PgQueryStore::new(); + let result = store.get_or_cache_ast(&StatementId::new(input)); + assert!(result.is_ok(), "failed to parse {input:?}: {result:?}"); + } + #[test] fn test_plpgsql_syntax_error() { let input = " From 8bf4c4301d91dbf84903cfcd767424df25c7e97a Mon Sep 17 00:00:00 2001 From: edjubert Date: Tue, 8 Sep 2026 18:20:50 +0200 Subject: [PATCH 3/7] test(workspace): preserve positions after named parameter normalization --- .../src/workspace/server.tests.rs | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/crates/pgls_workspace/src/workspace/server.tests.rs b/crates/pgls_workspace/src/workspace/server.tests.rs index 9e903c5fb..d44e3bbc2 100644 --- a/crates/pgls_workspace/src/workspace/server.tests.rs +++ b/crates/pgls_workspace/src/workspace/server.tests.rs @@ -265,6 +265,57 @@ async fn test_syntax_error(test_db: PgPool) { ); } +#[tokio::test] +async fn named_parameter_normalization_preserves_syntax_diagnostic_offsets() { + let workspace = get_test_workspace(None).expect("Unable to create test workspace"); + let path = PgLSPath::new("named-parameter.sql"); + let content = "SELECT * FROM :very_long_schema.existing_table AS t seect 1;"; + + workspace + .open_file(OpenFileParams { + path: path.clone(), + content: content.into(), + version: 1, + }) + .expect("Unable to open test file"); + + let diagnostics = workspace + .pull_file_diagnostics(crate::workspace::PullFileDiagnosticsParams { + path, + categories: RuleCategories::all(), + max_diagnostics: 100, + only: vec![], + skip: vec![], + }) + .expect("Unable to pull diagnostics") + .diagnostics; + + let syntax_diagnostic = diagnostics + .iter() + .find(|diagnostic| { + diagnostic + .category() + .is_some_and(|category| category.name() == "syntax") + }) + .expect("Expected one syntax diagnostic"); + + let expected_span = TextRange::new(0.into(), u32::try_from(content.len()).unwrap().into()); + assert_eq!(syntax_diagnostic.location().span, Some(expected_span)); + + let qualifier_start = content + .find(":very_long_schema") + .expect("query contains the named qualifier"); + assert_ne!( + syntax_diagnostic.location().span, + Some(TextRange::new( + u32::try_from(qualifier_start).unwrap().into(), + u32::try_from(qualifier_start + ":very_long_schema".len()) + .unwrap() + .into() + )) + ); +} + #[tokio::test] async fn correctly_ignores_files() { let mut conf = PartialConfiguration::init(); From 60cfa063224498de97b2e4a2175492f9d57759d4 Mon Sep 17 00:00:00 2001 From: edjubert Date: Thu, 10 Sep 2026 17:37:47 +0200 Subject: [PATCH 4/7] refactor(lexer): expose named parameter conversion metadata --- crates/pgls_lexer/src/lib.rs | 5 +++- crates/pgls_lexer/src/params.rs | 52 +++++++++++++++++++++++++++++++-- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/crates/pgls_lexer/src/lib.rs b/crates/pgls_lexer/src/lib.rs index 03ceec4e8..f367907bc 100644 --- a/crates/pgls_lexer/src/lib.rs +++ b/crates/pgls_lexer/src/lib.rs @@ -6,7 +6,10 @@ mod params; pub use crate::codegen::syntax_kind::SyntaxKind; pub use crate::lexed::{LexDiagnostic, Lexed}; pub use crate::lexer::Lexer; -pub use crate::params::convert_to_positional_params; +pub use crate::params::{ + NamedParameterConversion, convert_to_positional_params, + convert_to_positional_params_with_metadata, +}; /// Lex the input string into tokens and diagnostics pub fn lex(input: &str) -> Lexed<'_> { diff --git a/crates/pgls_lexer/src/params.rs b/crates/pgls_lexer/src/params.rs index 3d0a48f08..162e40e0c 100644 --- a/crates/pgls_lexer/src/params.rs +++ b/crates/pgls_lexer/src/params.rs @@ -23,18 +23,25 @@ const IDENTIFIER_CONTEXT: [SyntaxKind; 15] = [ SyntaxKind::DOT, ]; +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct NamedParameterConversion { + pub sql: String, + pub has_identifier_parameters: bool, +} + /// Converts named parameters in a SQL query string to parser-compatible placeholders. /// /// Replacements preserve the source byte length so parser diagnostics can be mapped directly /// back to the original query. A raw colon inside brackets is left untouched because it may be /// PostgreSQL array-slice syntax rather than a named parameter. -pub fn convert_to_positional_params(text: &str) -> String { +pub fn convert_to_positional_params_with_metadata(text: &str) -> NamedParameterConversion { let mut result = String::with_capacity(text.len()); let mut value_params: HashMap<&str, usize> = HashMap::new(); let mut identifier_params: HashMap<&str, usize> = HashMap::new(); let mut value_index = 1; let mut identifier_index = 0; let mut bracket_depth = 0_usize; + let mut has_identifier_parameters = false; let lexed = lex(text); for (token_idx, kind) in lexed.tokens().enumerate() { @@ -59,6 +66,10 @@ pub fn convert_to_positional_params(text: &str) -> String { _ => false, }; + if is_identifier { + has_identifier_parameters = true; + } + let replacement = if is_identifier { let index = *identifier_params.entry(token_text).or_insert_with(|| { let index = identifier_index; @@ -88,7 +99,14 @@ pub fn convert_to_positional_params(text: &str) -> String { } debug_assert_eq!(result.len(), text.len()); - result + NamedParameterConversion { + sql: result, + has_identifier_parameters, + } +} + +pub fn convert_to_positional_params(text: &str) -> String { + convert_to_positional_params_with_metadata(text).sql } #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -249,4 +267,34 @@ mod tests { assert_eq!(normalized, "select arr[$1 ], arr[$2 ], arr[$3 ]"); assert_eq!(normalized.len(), input.len()); } + + #[test] + fn reports_identifier_parameter_conversion_metadata() { + let input = "select * from :raw_data.documents where id = :id"; + let conversion = convert_to_positional_params_with_metadata(input); + + assert_eq!( + conversion.sql, + "select * from a .documents where id = $1 " + ); + assert!(conversion.has_identifier_parameters); + assert_eq!(conversion.sql.len(), input.len()); + assert_eq!(convert_to_positional_params(input), conversion.sql); + } + + #[test] + fn value_parameters_and_array_slices_do_not_report_identifier_metadata() { + let value_input = "select :id, @name, $email, :'status'"; + let value_conversion = convert_to_positional_params_with_metadata(value_input); + + assert!(!value_conversion.has_identifier_parameters); + assert_eq!(value_conversion.sql.len(), value_input.len()); + + let slice_input = "select arr[3:array_upper(arr, 1)]"; + let slice_conversion = convert_to_positional_params_with_metadata(slice_input); + + assert!(!slice_conversion.has_identifier_parameters); + assert_eq!(slice_conversion.sql, slice_input); + assert_eq!(slice_conversion.sql.len(), slice_input.len()); + } } From e6f9a01fefa22131f34053eb11ef1e77bfbe287e Mon Sep 17 00:00:00 2001 From: edjubert Date: Thu, 10 Sep 2026 18:23:25 +0200 Subject: [PATCH 5/7] fix(workspace): skip typecheck for named identifiers --- crates/pgls_workspace/src/workspace/server.rs | 100 ++++++++++-------- .../src/workspace/server.tests.rs | 80 ++++++++++++++ .../src/workspace/server/pg_query.rs | 2 +- 3 files changed, 138 insertions(+), 44 deletions(-) diff --git a/crates/pgls_workspace/src/workspace/server.rs b/crates/pgls_workspace/src/workspace/server.rs index 3e5a6d520..bd95bc377 100644 --- a/crates/pgls_workspace/src/workspace/server.rs +++ b/crates/pgls_workspace/src/workspace/server.rs @@ -17,7 +17,7 @@ use document::{ExecuteStatementMapper, TypecheckDiagnosticsMapper}; #[cfg(feature = "db")] use futures::{StreamExt, TryStreamExt, stream}; #[cfg(feature = "db")] -use pg_query::convert_to_positional_params; +use pg_query::convert_to_positional_params_with_metadata; use pgls_analyse::AnalysisFilter; use pgls_analyser::{Analyser, AnalyserConfig, AnalyserParams, LinterOptions}; @@ -610,49 +610,63 @@ impl Workspace for WorkspaceServer { if let Some(ast) = ast { // Type checking if typecheck_enabled { - let typecheck_result = - pgls_typecheck::check_sql(TypecheckParams { - conn: &pool, - sql: convert_to_positional_params(id.content()) - .as_str(), - ast: &ast, - tree: &cst, - schema_cache: schema_cache.as_ref(), - search_path_patterns, - identifiers: fn_sig - .map(|s| { - s.args - .iter() - .map(|a| TypedIdentifier { - path: s.name.clone(), - name: a.name.clone(), - type_: IdentifierType { - schema: a.type_.schema.clone(), - name: a.type_.name.clone(), - is_array: a.type_.is_array, - }, - }) - .collect::>() - }) - .unwrap_or_default(), - }) - .await; - - match typecheck_result { - Ok(Some(diag)) => { - let r = diag - .location() - .span - .map(|span| span + range.start()); - diagnostics.push( - diag.with_file_path( - path.as_path().display().to_string(), - ) - .with_file_span(r.unwrap_or(range)), - ); + let conversion = + convert_to_positional_params_with_metadata( + id.content(), + ); + + if !conversion.has_identifier_parameters { + let typecheck_result = + pgls_typecheck::check_sql(TypecheckParams { + conn: &pool, + sql: conversion.sql.as_str(), + ast: &ast, + tree: &cst, + schema_cache: schema_cache.as_ref(), + search_path_patterns, + identifiers: fn_sig + .map(|s| { + s.args + .iter() + .map(|a| TypedIdentifier { + path: s.name.clone(), + name: a.name.clone(), + type_: IdentifierType { + schema: a + .type_ + .schema + .clone(), + name: a + .type_ + .name + .clone(), + is_array: a + .type_ + .is_array, + }, + }) + .collect::>() + }) + .unwrap_or_default(), + }) + .await; + + match typecheck_result { + Ok(Some(diag)) => { + let r = diag + .location() + .span + .map(|span| span + range.start()); + diagnostics.push( + diag.with_file_path( + path.as_path().display().to_string(), + ) + .with_file_span(r.unwrap_or(range)), + ); + } + Ok(None) => {} + Err(err) => return Err(err), } - Ok(None) => {} - Err(err) => return Err(err), } } diff --git a/crates/pgls_workspace/src/workspace/server.tests.rs b/crates/pgls_workspace/src/workspace/server.tests.rs index d44e3bbc2..df1bc239c 100644 --- a/crates/pgls_workspace/src/workspace/server.tests.rs +++ b/crates/pgls_workspace/src/workspace/server.tests.rs @@ -807,6 +807,86 @@ async fn test_create_as_typecheck_diagnostic_offsets(test_db: PgPool) { ); } +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn named_identifier_params_skip_only_affected_typecheck(test_db: PgPool) { + let connect_options = test_db.connect_options(); + let host = connect_options.get_host().to_string(); + let port = connect_options.get_port(); + let database = connect_options + .get_database() + .expect("test database must have a name") + .to_string(); + + let mut conf = PartialConfiguration::init(); + conf.merge_with(PartialConfiguration { + db: Some(PartialDatabaseConfiguration { + host: Some(host), + port: Some(port), + database: Some(database), + ..Default::default() + }), + ..Default::default() + }); + + test_db + .execute("CREATE TABLE named_parameter_typecheck_users (id integer PRIMARY KEY);") + .await + .expect("test table setup must succeed"); + + let workspace = get_test_workspace(Some(conf)).expect("Unable to create test workspace"); + let path = PgLSPath::new("named-identifier-parameter.sql"); + let content = r#" +SELECT * FROM :raw_data.documents; +SELECT missing_column FROM named_parameter_typecheck_users; +"#; + + workspace + .open_file(OpenFileParams { + path: path.clone(), + content: content.into(), + version: 1, + }) + .expect("Unable to open test file"); + + let diagnostics = workspace + .pull_file_diagnostics(crate::workspace::PullFileDiagnosticsParams { + path, + categories: RuleCategories::all(), + max_diagnostics: 100, + only: vec![], + skip: vec![], + }) + .expect("Unable to pull diagnostics") + .diagnostics; + + let typecheck_diagnostics = diagnostics + .iter() + .filter(|diagnostic| { + diagnostic + .category() + .is_some_and(|category| category.name() == "typecheck") + }) + .collect::>(); + + assert_eq!( + typecheck_diagnostics.len(), + 1, + "only the second statement should produce a typecheck diagnostic: {diagnostics:#?}" + ); + + let missing_column_start = content + .find("missing_column") + .expect("test SQL contains the invalid column"); + let missing_column_end = missing_column_start + "missing_column".len(); + assert_eq!( + typecheck_diagnostics[0].location().span, + Some(TextRange::new( + u32::try_from(missing_column_start).unwrap().into(), + u32::try_from(missing_column_end).unwrap().into(), + )) + ); +} + #[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] async fn test_named_params(_test_db: PgPool) { let conf = PartialConfiguration::init(); diff --git a/crates/pgls_workspace/src/workspace/server/pg_query.rs b/crates/pgls_workspace/src/workspace/server/pg_query.rs index d27bd8996..7ba6df755 100644 --- a/crates/pgls_workspace/src/workspace/server/pg_query.rs +++ b/crates/pgls_workspace/src/workspace/server/pg_query.rs @@ -2,7 +2,7 @@ use std::num::NonZeroUsize; use std::sync::{Arc, LazyLock, Mutex}; use lru::LruCache; -pub use pgls_lexer::convert_to_positional_params; +pub use pgls_lexer::{convert_to_positional_params, convert_to_positional_params_with_metadata}; use pgls_query_ext::diagnostics::*; use pgls_text_size::TextRange; use regex::Regex; From e074d3e605b40a9eec597eced698f79bb22ac75e Mon Sep 17 00:00:00 2001 From: edjubert Date: Mon, 14 Sep 2026 10:14:55 +0200 Subject: [PATCH 6/7] refactor(lexer): derive named parameter placeholders from the parameter name --- crates/pgls_lexer/src/params.rs | 188 +++++++++++++----- .../src/workspace/server/pg_query.rs | 2 +- 2 files changed, 140 insertions(+), 50 deletions(-) diff --git a/crates/pgls_lexer/src/params.rs b/crates/pgls_lexer/src/params.rs index 162e40e0c..fc0b97132 100644 --- a/crates/pgls_lexer/src/params.rs +++ b/crates/pgls_lexer/src/params.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use crate::{SyntaxKind, lex}; @@ -27,6 +27,14 @@ const IDENTIFIER_CONTEXT: [SyntaxKind; 15] = [ pub struct NamedParameterConversion { pub sql: String, pub has_identifier_parameters: bool, + /// Placeholder identifier to the source text it replaced, for instance + /// `raw_data` to `:raw_data`. + pub identifier_replacements: HashMap, + /// Positional index to the source text it replaced, for instance `1` to `:'agen_code'`. + pub value_replacements: HashMap, + /// False when a placeholder collides with a real identifier of the same statement, in which + /// case the substitution cannot be undone and the caller must not rewrite the statement. + pub restorable: bool, } /// Converts named parameters in a SQL query string to parser-compatible placeholders. @@ -37,13 +45,25 @@ pub struct NamedParameterConversion { pub fn convert_to_positional_params_with_metadata(text: &str) -> NamedParameterConversion { let mut result = String::with_capacity(text.len()); let mut value_params: HashMap<&str, usize> = HashMap::new(); - let mut identifier_params: HashMap<&str, usize> = HashMap::new(); let mut value_index = 1; - let mut identifier_index = 0; let mut bracket_depth = 0_usize; let mut has_identifier_parameters = false; let lexed = lex(text); + + // Identifiers written as such in the source. A placeholder equal to one of them cannot be + // told apart from it after formatting, so the conversion is flagged as not restorable. + let plain_identifiers: HashSet<&str> = lexed + .tokens() + .enumerate() + .filter(|(idx, kind)| *kind == SyntaxKind::IDENT && !lexed.text(*idx).is_empty()) + .map(|(idx, _)| lexed.text(idx)) + .collect(); + + let mut identifier_replacements: HashMap = HashMap::new(); + let mut value_replacements: HashMap = HashMap::new(); + let mut restorable = true; + for (token_idx, kind) in lexed.tokens().enumerate() { if kind == SyntaxKind::EOF { break; @@ -71,18 +91,28 @@ pub fn convert_to_positional_params_with_metadata(text: &str) -> NamedParameterC } let replacement = if is_identifier { - let index = *identifier_params.entry(token_text).or_insert_with(|| { - let index = identifier_index; - identifier_index += 1; - index - }); - identifier_replacement(index, token_text.len()) + let replacement = identifier_replacement(token_text, token_text.len()); + + // Two different parameters landing on the same placeholder, which happens when a + // name yields no usable identifier and both fall back, cannot be told apart on the + // way back. + if plain_identifiers.contains(replacement.as_str()) + || identifier_replacements + .get(&replacement) + .is_some_and(|existing| existing != token_text) + { + restorable = false; + } + + identifier_replacements.insert(replacement.clone(), token_text.to_string()); + replacement } else { let index = *value_params.entry(token_text).or_insert_with(|| { let index = value_index; value_index += 1; index }); + value_replacements.insert(index, token_text.to_string()); format!("${index}") }; @@ -102,6 +132,9 @@ pub fn convert_to_positional_params_with_metadata(text: &str) -> NamedParameterC NamedParameterConversion { sql: result, has_identifier_parameters, + identifier_replacements, + value_replacements, + restorable, } } @@ -153,49 +186,46 @@ fn push_padded_replacement(result: &mut String, replacement: &str, original_len: result.push_str(&" ".repeat(original_len - replacement.len())); } -fn identifier_replacement(index: usize, max_len: usize) -> String { - let identifier = deterministic_identifier(index); - if identifier.len() <= max_len { - identifier - } else { - "a".to_string() +/// Builds the identifier that stands in for a named parameter used in identifier position. +/// +/// Deriving it from the parameter name is what makes the substitution reversible: `:raw_data` +/// becomes `raw_data`, which the formatter can map back. Dropping the leading colon always frees +/// at least one byte, so the replacement fits in the source span, underscore included. +/// +/// The name is lowercased because PostgreSQL folds unquoted identifiers, so that is the spelling +/// the printer will emit and the one the reverse map has to be keyed on. A name that happens to be +/// a SQL keyword gets an underscore: `:table` would otherwise yield `table`, which does not parse +/// in identifier position. +fn identifier_replacement(token_text: &str, max_len: usize) -> String { + let name: String = token_text + .trim_start_matches([':', '@', '$']) + .trim_matches('"') + .chars() + .filter(|c| c.is_ascii_alphanumeric() || *c == '_') + .collect::() + .to_ascii_lowercase(); + + if name.is_empty() || name.starts_with(|c: char| c.is_ascii_digit()) { + return "a".to_string(); } -} - -const ALPHABET: [char; 26] = [ - 'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', - 't', 'u', 'v', 'w', 'x', 'y', 'z', -]; -/// Generates a deterministic identifier based on the given index. -fn deterministic_identifier(idx: usize) -> String { - let iteration = idx / ALPHABET.len(); - let pos = idx % ALPHABET.len(); + let candidate = if SyntaxKind::from_keyword(&name).is_some() { + format!("{name}_") + } else { + name + }; - format!( - "{}{}", - ALPHABET[pos], - if iteration > 0 { - deterministic_identifier(iteration - 1) - } else { - "".to_string() - } - ) + if candidate.len() <= max_len { + candidate + } else { + "a".to_string() + } } #[cfg(test)] mod tests { use super::*; - #[test] - fn test_deterministic_identifier() { - assert_eq!(deterministic_identifier(0), "a"); - assert_eq!(deterministic_identifier(25), "z"); - assert_eq!(deterministic_identifier(26), "aa"); - assert_eq!(deterministic_identifier(27), "ba"); - assert_eq!(deterministic_identifier(51), "za"); - } - #[test] fn test_convert_to_positional_params() { let input = "select * from users where id = @one and name = :two and email = :'three';"; @@ -221,23 +251,23 @@ mod tests { let cases = [ ( "select * from :schema.items where id = :id", - "select * from a .items where id = $1 ", + "select * from schema_.items where id = $1 ", ), ( "select * from @schema.items where id = $id", - "select * from a .items where id = $1 ", + "select * from schema_.items where id = $1 ", ), ( "select * from $schema.items where id = @id", - "select * from a .items where id = $1 ", + "select * from schema_.items where id = $1 ", ), ( "select * from :\"schema\".items", - "select * from a .items", + "select * from schema_ .items", ), ( "cross join :raw_data.migration_infos", - "cross join a .migration_infos", + "cross join raw_data .migration_infos", ), ]; @@ -275,7 +305,7 @@ mod tests { assert_eq!( conversion.sql, - "select * from a .documents where id = $1 " + "select * from raw_data .documents where id = $1 " ); assert!(conversion.has_identifier_parameters); assert_eq!(conversion.sql.len(), input.len()); @@ -297,4 +327,64 @@ mod tests { assert_eq!(slice_conversion.sql, slice_input); assert_eq!(slice_conversion.sql.len(), slice_input.len()); } + + #[test] + fn identifier_placeholder_is_derived_from_the_name() { + let conversion = convert_to_positional_params_with_metadata("SELECT x FROM :raw_data.t"); + + assert_eq!(conversion.sql, "SELECT x FROM raw_data .t"); + assert_eq!(conversion.sql.len(), "SELECT x FROM :raw_data.t".len()); + assert!(conversion.restorable); + assert_eq!( + conversion.identifier_replacements.get("raw_data"), + Some(&":raw_data".to_string()) + ); + } + + #[test] + fn value_placeholders_are_mapped_back_to_their_source_text() { + let conversion = + convert_to_positional_params_with_metadata("SELECT 1 WHERE a = :'agen_code'"); + + assert_eq!( + conversion.value_replacements.get(&1), + Some(&":'agen_code'".to_string()) + ); + assert!(conversion.restorable); + } + + #[test] + fn a_placeholder_colliding_with_a_real_identifier_is_not_restorable() { + let conversion = + convert_to_positional_params_with_metadata("SELECT x FROM raw_data.t, :raw_data.u"); + + assert!(!conversion.restorable); + } + + #[test] + fn a_name_that_is_a_keyword_gets_an_underscore() { + let conversion = convert_to_positional_params_with_metadata("SELECT x FROM :table.t"); + + // `table` alone does not parse in identifier position, `table_` does, and the underscore + // fits because the colon was dropped. + assert_eq!(conversion.sql, "SELECT x FROM table_.t"); + assert_eq!(conversion.sql.len(), "SELECT x FROM :table.t".len()); + assert_eq!( + conversion.identifier_replacements.get("table_"), + Some(&":table".to_string()) + ); + } + + #[test] + fn a_derived_name_is_lowercased() { + let conversion = + convert_to_positional_params_with_metadata("GRANT usage ON SCHEMA public TO :DB_ROLE"); + + // PostgreSQL folds unquoted identifiers, so `db_role` is what the printer will emit and + // therefore the only key the reverse map can use. + assert_eq!( + conversion.identifier_replacements.get("db_role"), + Some(&":DB_ROLE".to_string()) + ); + } } diff --git a/crates/pgls_workspace/src/workspace/server/pg_query.rs b/crates/pgls_workspace/src/workspace/server/pg_query.rs index 7ba6df755..a17f3eaba 100644 --- a/crates/pgls_workspace/src/workspace/server/pg_query.rs +++ b/crates/pgls_workspace/src/workspace/server/pg_query.rs @@ -115,7 +115,7 @@ mod tests { assert_eq!( result, - "grant usage on schema public, app_public, app_hidden to a ;" + "grant usage on schema public, app_public, app_hidden to db_role ;" ); let store = PgQueryStore::new(); From 05a0b07d44d2596d612b19b44234ba86a120dce3 Mon Sep 17 00:00:00 2001 From: edjubert Date: Mon, 14 Sep 2026 10:29:08 +0200 Subject: [PATCH 7/7] fix(workspace): restore named parameters in formatted output --- crates/pgls_workspace/src/workspace/server.rs | 102 ++++++++++++++++-- .../src/workspace/server.tests.rs | 33 ++++++ 2 files changed, 127 insertions(+), 8 deletions(-) diff --git a/crates/pgls_workspace/src/workspace/server.rs b/crates/pgls_workspace/src/workspace/server.rs index bd95bc377..0fd0fa8de 100644 --- a/crates/pgls_workspace/src/workspace/server.rs +++ b/crates/pgls_workspace/src/workspace/server.rs @@ -980,7 +980,11 @@ impl Workspace for WorkspaceServer { continue; }; - formatted_sql_fn_bodies.insert(parent_id, result.formatted); + let Some(formatted) = restore_named_parameters(text, &result.formatted, ast) else { + continue; + }; + + formatted_sql_fn_bodies.insert(parent_id, formatted); } } @@ -1021,14 +1025,33 @@ impl Workspace for WorkspaceServer { match pgls_pretty_print::format_statement(&ast, &text, &config) { Ok(result) => { - if text != result.formatted { - statements.push(StatementFormatResult { - original: text.clone(), - formatted: result.formatted.clone(), - range: stmt_range, - }); + match restore_named_parameters(&text, &result.formatted, &ast) { + Some(formatted) => { + if text != formatted { + statements.push(StatementFormatResult { + original: text.clone(), + formatted: formatted.clone(), + range: stmt_range, + }); + } + formatted_output.push_str(&formatted); + } + None => { + diagnostics.push(SDiagnostic::new( + pgls_diagnostics::Error::from( + WorkspaceError::format_error( + "Named parameters could not be restored after \ + formatting, the statement was left untouched" + .to_string(), + ), + ) + .with_file_path(&path_str) + .with_file_span(stmt_range), + )); + + formatted_output.push_str(&text); + } } - formatted_output.push_str(&result.formatted); } Err(err) => { diagnostics.push(SDiagnostic::new( @@ -1190,6 +1213,69 @@ fn is_dir(path: &Path) -> bool { path.is_dir() || (path.is_symlink() && fs::read_link(path).is_ok_and(|path| path.is_dir())) } +/// Put psql named parameters back where the formatter printed their placeholders. +/// +/// The parser only ever sees placeholders, so the printed statement carries `$1` or a bare +/// identifier instead of `:'agen_code'` or `:raw_data`. Substitution happens on the token stream +/// so that a `$1` inside a string literal is left alone, and the result is proven by converting it +/// back and comparing the normalized ASTs: a statement that does not survive that round trip is +/// reported as unrestorable and kept as it was written. +fn restore_named_parameters( + original: &str, + formatted: &str, + original_ast: &pgls_query::NodeEnum, +) -> Option { + use pgls_lexer::{SyntaxKind, lex}; + + let conversion = pgls_lexer::convert_to_positional_params_with_metadata(original); + + if conversion.identifier_replacements.is_empty() && conversion.value_replacements.is_empty() { + return Some(formatted.to_string()); + } + + if !conversion.restorable { + return None; + } + + let lexed = lex(formatted); + let mut restored = String::with_capacity(formatted.len()); + + for (idx, kind) in lexed.tokens().enumerate() { + if kind == SyntaxKind::EOF { + break; + } + + let text = lexed.text(idx); + + let replacement = match kind { + SyntaxKind::IDENT => conversion.identifier_replacements.get(text), + SyntaxKind::POSITIONAL_PARAM => text + .trim_start_matches('$') + .parse::() + .ok() + .and_then(|index| conversion.value_replacements.get(&index)), + _ => None, + }; + + match replacement { + Some(source_text) => restored.push_str(source_text), + None => restored.push_str(text), + } + } + + let reparsed = pgls_query::parse(&pgls_lexer::convert_to_positional_params(&restored)).ok()?; + let mut restored_ast = reparsed.into_root()?; + let mut expected_ast = original_ast.clone(); + pgls_pretty_print::normalize_ast(&mut restored_ast); + pgls_pretty_print::normalize_ast(&mut expected_ast); + + if restored_ast == expected_ast { + Some(restored) + } else { + None + } +} + #[cfg(all(test, feature = "db"))] #[path = "server.tests.rs"] mod tests; diff --git a/crates/pgls_workspace/src/workspace/server.tests.rs b/crates/pgls_workspace/src/workspace/server.tests.rs index df1bc239c..941ab83c5 100644 --- a/crates/pgls_workspace/src/workspace/server.tests.rs +++ b/crates/pgls_workspace/src/workspace/server.tests.rs @@ -316,6 +316,39 @@ async fn named_parameter_normalization_preserves_syntax_diagnostic_offsets() { ); } +#[tokio::test] +async fn format_preserves_named_parameters() { + let mut conf = PartialConfiguration::init(); + conf.merge_with(PartialConfiguration { + format: Some(PartialFormatConfiguration { + enabled: Some(true), + ..Default::default() + }), + ..Default::default() + }); + + let workspace = get_test_workspace(Some(conf)).expect("Unable to create test workspace"); + let path = PgLSPath::new("named-parameter-format.sql"); + let content = "SELECT x FROM :raw_data.t WHERE t.a = :'agen_code' AND t.b = :var_date;"; + + workspace + .open_file(OpenFileParams { + path: path.clone(), + content: content.into(), + version: 1, + }) + .expect("Unable to open test file"); + + let result = workspace + .pull_file_formatting(PullFileFormattingParams { path, range: None }) + .expect("Unable to format file"); + + assert!(result.formatted.contains(":raw_data.t")); + assert!(result.formatted.contains(":'agen_code'")); + assert!(result.formatted.contains(":var_date")); + assert!(!result.formatted.contains("$1")); +} + #[tokio::test] async fn correctly_ignores_files() { let mut conf = PartialConfiguration::init();