From 2c9062d4893c1e5fde51c40f64ece9e132f6d5e8 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sun, 25 Oct 2026 16:34:45 +0000 Subject: [PATCH 1/5] fix: normalise identifiers the same way everywhere Turn identifiers as written into names with one function, pgls_catalog::identifier_name, like Postgres' scanner: unquoted identifiers are downcased (ASCII only), quoted ones lose their quotes and unescape "". Hover, completions, the tree-sitter context, inlay hints and identifierTooLong use it, and Snapshot's find_* lookups now take names. Completions no longer lowercase the whole statement, so quoted names keep their case, and the grammar reads "a""b" as one identifier. --- .../lint/correctness/identifier_too_long.rs | 16 +--- .../identifierTooLong/quoted_and_columns.sql | 5 +- .../quoted_and_columns.sql.snap | 15 +++- crates/pgls_catalog/src/identifier.rs | 46 ++++++++++ crates/pgls_catalog/src/lib.rs | 3 + crates/pgls_catalog/src/snapshot/mod.rs | 67 +++----------- .../pgls_completions/src/providers/columns.rs | 42 ++++++++- .../src/relevance/filtering.rs | 5 +- .../pgls_completions/src/relevance/scoring.rs | 6 +- crates/pgls_completions/src/sanitization.rs | 9 +- ...test_helper__completes_quoted_columns.snap | 6 ++ ...er__suggests_columns_in_insert_clause.snap | 10 +-- crates/pgls_hover/src/hovered_node.rs | 44 ++++----- .../tests/hover_integration_tests.rs | 56 ++++++++++++ ...able_hover_quoted_name_keeps_its_case.snap | 22 +++++ ..._hover_quoted_name_with_escaped_quote.snap | 22 +++++ ...able_hover_unquoted_name_is_downcased.snap | 22 +++++ crates/pgls_inlay_hints/src/lib.rs | 15 +--- .../tests/inlay_hints_tests.rs | 14 +++ crates/pgls_treesitter/src/context/mod.rs | 89 ++++++++++--------- crates/pgls_treesitter_grammar/grammar.js | 2 +- .../tests/grammar_tests.rs | 7 ++ ..._quoted_identifier_with_escaped_quote.snap | 22 +++++ 23 files changed, 379 insertions(+), 166 deletions(-) create mode 100644 crates/pgls_catalog/src/identifier.rs create mode 100644 crates/pgls_hover/tests/snapshots/table_hover_quoted_name_keeps_its_case.snap create mode 100644 crates/pgls_hover/tests/snapshots/table_hover_quoted_name_with_escaped_quote.snap create mode 100644 crates/pgls_hover/tests/snapshots/table_hover_unquoted_name_is_downcased.snap create mode 100644 crates/pgls_treesitter_grammar/tests/snapshots/grammar_tests__quoted_identifier_with_escaped_quote.snap diff --git a/crates/pgls_analyser/src/lint/correctness/identifier_too_long.rs b/crates/pgls_analyser/src/lint/correctness/identifier_too_long.rs index 6e6134972..5046a0a51 100644 --- a/crates/pgls_analyser/src/lint/correctness/identifier_too_long.rs +++ b/crates/pgls_analyser/src/lint/correctness/identifier_too_long.rs @@ -1,5 +1,6 @@ use crate::{LinterDiagnostic, LinterRule, LinterRuleContext}; use pgls_analyse::{RuleSource, declare_lint_rule}; +use pgls_catalog::identifier_name; use pgls_console::markup; use pgls_diagnostics::Severity; use pgls_query::protobuf::Token; @@ -45,6 +46,7 @@ impl LinterRule for IdentifierTooLong { fn run(ctx: &LinterRuleContext) -> Vec { // The parser already truncated the names in the syntax tree, so we read the tokens. + // Downcasing an unquoted identifier doesn't change its length. let Some(sql) = ctx.sql() else { return vec![]; }; @@ -61,7 +63,7 @@ impl LinterRule for IdentifierTooLong { TextSize::from(u32::try_from(token.end).ok()?), ); let identifier = - identifier_value(sql.get(range.start().into()..range.end().into())?); + identifier_name(sql.get(range.start().into()..range.end().into())?); (identifier.len() > MAX_IDENTIFIER_BYTES).then_some((range, identifier)) }) .map(|(range, identifier)| { @@ -79,15 +81,3 @@ impl LinterRule for IdentifierTooLong { .collect() } } - -/// The name a quoted or unquoted identifier stands for. Downcasing an unquoted identifier -/// doesn't change its length. -fn identifier_value(text: &str) -> String { - match text - .strip_prefix('"') - .and_then(|text| text.strip_suffix('"')) - { - Some(quoted) => quoted.replace("\"\"", "\""), - None => text.to_owned(), - } -} diff --git a/crates/pgls_analyser/tests/specs/correctness/identifierTooLong/quoted_and_columns.sql b/crates/pgls_analyser/tests/specs/correctness/identifierTooLong/quoted_and_columns.sql index 0c37710d7..8fe906ebd 100644 --- a/crates/pgls_analyser/tests/specs/correctness/identifierTooLong/quoted_and_columns.sql +++ b/crates/pgls_analyser/tests/specs/correctness/identifierTooLong/quoted_and_columns.sql @@ -2,5 +2,8 @@ create table public.accounts ( -- expect_lint/identifierTooLong "Customer Subscription Billing Cycle Adjustments History Archive Id" bigint, -- expect_lint/identifierTooLong - customer_subscription_billing_cycle_adjustments_history_archive_note text + customer_subscription_billing_cycle_adjustments_history_archive_note text, + -- Unquoted names are downcased before they are truncated. + -- expect_lint/identifierTooLong + Customer_Subscription_Billing_Cycle_Adjustments_History_Archive_Ref text ); diff --git a/crates/pgls_analyser/tests/specs/correctness/identifierTooLong/quoted_and_columns.sql.snap b/crates/pgls_analyser/tests/specs/correctness/identifierTooLong/quoted_and_columns.sql.snap index 4e9f2a453..a6784ea9c 100644 --- a/crates/pgls_analyser/tests/specs/correctness/identifierTooLong/quoted_and_columns.sql.snap +++ b/crates/pgls_analyser/tests/specs/correctness/identifierTooLong/quoted_and_columns.sql.snap @@ -8,7 +8,10 @@ create table public.accounts ( -- expect_lint/identifierTooLong "Customer Subscription Billing Cycle Adjustments History Archive Id" bigint, -- expect_lint/identifierTooLong - customer_subscription_billing_cycle_adjustments_history_archive_note text + customer_subscription_billing_cycle_adjustments_history_archive_note text, + -- Unquoted names are downcased before they are truncated. + -- expect_lint/identifierTooLong + Customer_Subscription_Billing_Cycle_Adjustments_History_Archive_Ref text ); ``` @@ -31,3 +34,13 @@ lint/identifierTooLong ━━━━━━━━━━━━━━━━━━━ i Postgres truncates it to `customer_subscription_billing_cycle_adjustments_history_archive`. i Use a shorter name. + + + +lint/identifierTooLong ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + × Identifier customer_subscription_billing_cycle_adjustments_history_archive_ref is longer than 63 bytes. + + i Postgres truncates it to `customer_subscription_billing_cycle_adjustments_history_archive`. + + i Use a shorter name. diff --git a/crates/pgls_catalog/src/identifier.rs b/crates/pgls_catalog/src/identifier.rs new file mode 100644 index 000000000..ec72a8eb0 --- /dev/null +++ b/crates/pgls_catalog/src/identifier.rs @@ -0,0 +1,46 @@ +//! Identifiers in SQL text and the names they stand for. + +/// The name an identifier as written in SQL stands for, like Postgres' scanner reads it: a +/// quoted identifier without its quotes and with `""` unescaped, an unquoted identifier +/// downcased. Like [`downcase_identifier`] in a multibyte encoding such as UTF8, only ASCII +/// letters are downcased. The name is not truncated to 63 bytes. +/// +/// Text that is not a complete identifier, like the unterminated `"Foo` of a user who is still +/// typing, is treated as unquoted. +/// +/// [`downcase_identifier`]: https://github.com/postgres/postgres/blob/REL_18_6/src/backend/parser/scansup.c#L46 +pub fn identifier_name(text: &str) -> String { + match text + .strip_prefix('"') + .and_then(|text| text.strip_suffix('"')) + { + Some(quoted) => quoted.replace("\"\"", "\""), + None => text.to_ascii_lowercase(), + } +} + +#[cfg(test)] +mod tests { + use super::identifier_name; + + #[test] + fn unquoted_identifiers_are_downcased() { + assert_eq!(identifier_name("MyTable"), "mytable"); + assert_eq!(identifier_name("my_table$1"), "my_table$1"); + // Only ASCII letters, like Postgres in a UTF8 database. + assert_eq!(identifier_name("ÄBC"), "Äbc"); + } + + #[test] + fn quoted_identifiers_keep_their_case() { + assert_eq!(identifier_name(r#""MyTable""#), "MyTable"); + assert_eq!(identifier_name(r#""a""b""#), r#"a"b"#); + assert_eq!(identifier_name(r#""""#), ""); + } + + #[test] + fn incomplete_quoted_identifiers_are_unquoted() { + assert_eq!(identifier_name(r#""Foo"#), r#""foo"#); + assert_eq!(identifier_name(r#"""#), r#"""#); + } +} diff --git a/crates/pgls_catalog/src/lib.rs b/crates/pgls_catalog/src/lib.rs index 5373029c0..21901d992 100644 --- a/crates/pgls_catalog/src/lib.rs +++ b/crates/pgls_catalog/src/lib.rs @@ -11,6 +11,7 @@ //! - [`CatalogView`] (`lookup.rs`): the lookups the resolver makes. [`Catalog`] implements it. //! - [`resolve`] (`resolve/`): name resolution of a single statement against a [`CatalogView`], //! with one module per parse node in `resolve/nodes/`. +//! - [`identifier_name`] (`identifier.rs`): the name an identifier in SQL text stands for. //! //! The analyser walks a file statement by statement. For each statement it runs the rules, //! which resolve names against the current [`Catalog`] with the [`Session`]'s search path, and @@ -18,6 +19,7 @@ //! `session.apply(stmt)`. mod catalog; +mod identifier; mod lookup; pub mod resolve; mod session; @@ -25,6 +27,7 @@ pub mod snapshot; pub mod typing; pub use catalog::{Catalog, CatalogBase}; +pub use identifier::identifier_name; pub use lookup::{ Candidates, CastContext, CastInfo, CastMethod, CatalogView, ColumnInfo, FunctionArgument, FunctionArgumentMode, FunctionInfo, FunctionKind, FunctionSignature, Lookup, OperatorInfo, diff --git a/crates/pgls_catalog/src/snapshot/mod.rs b/crates/pgls_catalog/src/snapshot/mod.rs index a49f204a5..1862e78b7 100644 --- a/crates/pgls_catalog/src/snapshot/mod.rs +++ b/crates/pgls_catalog/src/snapshot/mod.rs @@ -125,34 +125,24 @@ impl Snapshot { }) } + // The `find_*` lookups take names, not identifiers as written: convert SQL text with + // [`crate::identifier_name`] first. + pub fn find_schema(&self, name: &str) -> Option<&Schema> { - let sanitized_name = Self::sanitize_identifier(name); - self.schemas.iter().find(|s| s.name == sanitized_name) + self.schemas.iter().find(|s| s.name == name) } pub fn find_tables(&self, name: &str, schema: Option<&str>) -> Vec<&Table> { - let sanitized_name = Self::sanitize_identifier(name); self.tables .iter() - .filter(|t| { - t.name == sanitized_name - && schema - .map(Self::sanitize_identifier) - .as_deref() - .is_none_or(|s| s == t.schema.as_str()) - }) + .filter(|t| t.name == name && schema.is_none_or(|s| s == t.schema)) .collect() } pub fn find_type(&self, name: &str, schema: Option<&str>) -> Option<&PostgresType> { - let sanitized_name = Self::sanitize_identifier(name); - self.types.iter().find(|t| { - t.name == sanitized_name - && schema - .map(Self::sanitize_identifier) - .as_deref() - .is_none_or(|s| s == t.schema.as_str()) - }) + self.types + .iter() + .find(|t| t.name == name && schema.is_none_or(|s| s == t.schema)) } pub fn find_type_by_id(&self, id: i64) -> Option<&PostgresType> { @@ -180,61 +170,32 @@ impl Snapshot { } pub fn find_cols(&self, name: &str, table: Option<&str>, schema: Option<&str>) -> Vec<&Column> { - let sanitized_name = Self::sanitize_identifier(name); self.columns .iter() .filter(|c| { - c.name.as_str() == sanitized_name - && table - .map(Self::sanitize_identifier) - .as_deref() - .is_none_or(|t| t == c.table_name.as_str()) - && schema - .map(Self::sanitize_identifier) - .as_deref() - .is_none_or(|s| s == c.schema_name.as_str()) + c.name.as_str() == name + && table.is_none_or(|t| t == c.table_name) + && schema.is_none_or(|s| s == c.schema_name) }) .collect() } pub fn find_types(&self, name: &str, schema: Option<&str>) -> Vec<&PostgresType> { - let sanitized_name = Self::sanitize_identifier(name); self.types .iter() - .filter(|t| { - t.name == sanitized_name - && schema - .map(Self::sanitize_identifier) - .as_deref() - .is_none_or(|s| s == t.schema.as_str()) - }) + .filter(|t| t.name == name && schema.is_none_or(|s| s == t.schema)) .collect() } pub fn find_functions(&self, name: &str, schema: Option<&str>) -> Vec<&Function> { - let sanitized_name = Self::sanitize_identifier(name); self.functions .iter() - .filter(|f| { - f.name == sanitized_name - && schema - .map(Self::sanitize_identifier) - .as_deref() - .is_none_or(|s| s == f.schema.as_str()) - }) + .filter(|f| f.name == name && schema.is_none_or(|s| s == f.schema)) .collect() } pub fn find_roles(&self, name: &str) -> Vec<&Role> { - let sanitized_name = Self::sanitize_identifier(name); - self.roles - .iter() - .filter(|r| r.name == sanitized_name) - .collect() - } - - fn sanitize_identifier(identifier: &str) -> String { - identifier.replace('"', "") + self.roles.iter().filter(|r| r.name == name).collect() } } diff --git a/crates/pgls_completions/src/providers/columns.rs b/crates/pgls_completions/src/providers/columns.rs index ddce40f18..e90d6deaf 100644 --- a/crates/pgls_completions/src/providers/columns.rs +++ b/crates/pgls_completions/src/providers/columns.rs @@ -51,9 +51,12 @@ fn get_completion_text(ctx: &TreesitterContext, col: &Column) -> CompletionText #[cfg(test)] mod tests { - use sqlx::PgPool; + use pgls_test_utils::QueryWithCursorPosition; + use sqlx::{Executor, PgPool}; - use crate::test_helper::{TestCompletionsCase, TestCompletionsSuite}; + use crate::test_helper::{ + CompletionAssertion, TestCompletionsCase, TestCompletionsSuite, assert_complete_results, + }; #[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] async fn handles_nested_queries(pool: PgPool) { @@ -597,4 +600,39 @@ mod tests { .snapshot("completes_quoted_columns_with_aliases") .await; } + + #[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] + async fn completes_the_columns_of_the_table_an_identifier_names(pool: PgPool) { + let setup = r#" + create table "MyTable" (quoted_id int); + create table mytable (folded_id int); + create table "a""b" (escaped_id int); + "#; + pool.execute(setup).await.unwrap(); + + // Unquoted names are downcased, quoted names keep their case and unescape `""`. + for (table, column) in [ + (r#""MyTable""#, "quoted_id"), + ("MyTable", "folded_id"), + (r#""a""b""#, "escaped_id"), + ] { + let others = ["quoted_id", "folded_id", "escaped_id"] + .into_iter() + .filter(|other| *other != column) + .map(|other| CompletionAssertion::LabelNotExists(other.into())); + assert_complete_results( + format!( + "select t.{} from {table} t", + QueryWithCursorPosition::cursor_marker() + ) + .as_str(), + std::iter::once(CompletionAssertion::Label(column.into())) + .chain(others) + .collect(), + None, + &pool, + ) + .await; + } + } } diff --git a/crates/pgls_completions/src/relevance/filtering.rs b/crates/pgls_completions/src/relevance/filtering.rs index 5dfc17905..a0c54e227 100644 --- a/crates/pgls_completions/src/relevance/filtering.rs +++ b/crates/pgls_completions/src/relevance/filtering.rs @@ -502,7 +502,10 @@ impl CompletionFilter<'_> { return None; } - if !kw.name.starts_with(content.chars().next().unwrap()) { + if !kw + .name + .starts_with(content.chars().next().unwrap().to_ascii_lowercase()) + { return None; } diff --git a/crates/pgls_completions/src/relevance/scoring.rs b/crates/pgls_completions/src/relevance/scoring.rs index f81bba727..ad5fc68c3 100644 --- a/crates/pgls_completions/src/relevance/scoring.rs +++ b/crates/pgls_completions/src/relevance/scoring.rs @@ -76,7 +76,9 @@ impl CompletionScore<'_> { (Some(_), Some(_)) => name.clone(), // If only one qualifier is written out, we might look at a schema, a table, or an alias. - (None, Some(qualifier)) => { + (None, Some(_)) => { + let qualifier = ctx.tail_qualifier_sanitized().unwrap_or_default(); + let qualifier = qualifier.as_str(); if self.get_schema_name().is_some_and(|s| s == qualifier) { self.get_table_name() .map(|t| format!("{t}.{name}")) @@ -443,7 +445,7 @@ impl CompletionScore<'_> { set.iter().any(|mentioned| match mentioned.alias.as_ref() { Some(als) => { let aliased_table = ctx.get_mentioned_table_for_alias(als.as_str()); - column.name == mentioned.column.replace('"', "") + column.name == mentioned.column && aliased_table.is_none_or(|t| t == &column.table_name) } None => mentioned.column == column.name, diff --git a/crates/pgls_completions/src/sanitization.rs b/crates/pgls_completions/src/sanitization.rs index 5cd1aa2b0..6c66342d8 100644 --- a/crates/pgls_completions/src/sanitization.rs +++ b/crates/pgls_completions/src/sanitization.rs @@ -48,8 +48,6 @@ where 'larger: 'smaller, { fn from(mut params: CompletionParams<'larger>) -> Self { - params.text = params.text.to_ascii_lowercase(); - while !params.text.is_char_boundary(params.position.into()) && params.position < TextSize::new(params.text.len().try_into().unwrap()) { @@ -353,8 +351,9 @@ mod tests { } #[test] - fn should_lowercase_everything_except_replaced_token() { - let input = "SELECT FROM users WHERE ts = NOW();"; + fn should_keep_the_case_of_the_sql() { + // Quoted identifiers like `"MyTable"` keep their case. + let input = r#"SELECT FROM "MyTable" WHERE ts = NOW();"#; let position = TextSize::new(7); let params = get_test_params(input, position); @@ -362,7 +361,7 @@ mod tests { assert_eq!( sanitized.text, - "select REPLACED_TOKEN from users where ts = now();" + r#"SELECT REPLACED_TOKEN FROM "MyTable" WHERE ts = NOW();"# ); } diff --git a/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns.snap b/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns.snap index 6953f6ddc..48a8b1d9a 100644 --- a/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns.snap +++ b/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns.snap @@ -142,6 +142,12 @@ users - private.users (Table) -------------- select "email" from "private".u| + +Results: +users - private.users (Table) + +-------------- + select "email" from "private".users; | Results: diff --git a/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__suggests_columns_in_insert_clause.snap b/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__suggests_columns_in_insert_clause.snap index 31b9186a6..1e7c82fca 100644 --- a/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__suggests_columns_in_insert_clause.snap +++ b/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__suggests_columns_in_insert_clause.snap @@ -257,9 +257,9 @@ identity_increment - information_schema.columns.identity_increment (Column) insert into instruments ("id", |) Results: -id - public.instruments.id (Column) name - public.instruments.name (Column) z - public.instruments.z (Column) +id - public.instruments.id (Column) a - public.others.a (Column) b - public.others.b (Column) @@ -268,9 +268,9 @@ b - public.others.b (Column) insert into instruments ("id", "|) Results: -id" - public.instruments.id (Column) name" - public.instruments.name (Column) z" - public.instruments.z (Column) +id" - public.instruments.id (Column) a" - public.others.a (Column) b" - public.others.b (Column) @@ -279,9 +279,9 @@ b" - public.others.b (Column) insert into instruments ("id", "|") Results: -id - public.instruments.id (Column) name - public.instruments.name (Column) z - public.instruments.z (Column) +id - public.instruments.id (Column) a - public.others.a (Column) b - public.others.b (Column) @@ -319,9 +319,9 @@ insert into instruments ("id", "name") values (| insert into instruments ("id", "name") values (|) Results: +z - public.instruments.z (Column) id - public.instruments.id (Column) name - public.instruments.name (Column) -z - public.instruments.z (Column) default - default (Keyword) a - public.others.a (Column) @@ -331,9 +331,9 @@ insert into instruments ("id", "name") values (1|) insert into instruments ("id", "name") values (1, |) Results: +z - public.instruments.z (Column) id - public.instruments.id (Column) name - public.instruments.name (Column) -z - public.instruments.z (Column) default - default (Keyword) a - public.others.a (Column) diff --git a/crates/pgls_hover/src/hovered_node.rs b/crates/pgls_hover/src/hovered_node.rs index b213bb5e2..f66f7a1a7 100644 --- a/crates/pgls_hover/src/hovered_node.rs +++ b/crates/pgls_hover/src/hovered_node.rs @@ -1,3 +1,4 @@ +use pgls_catalog::identifier_name; use pgls_treesitter::WrappingClause; #[derive(Debug)] @@ -23,35 +24,31 @@ impl HoveredNode { return None; } + // The catalog holds names, not identifiers as written. + let name = identifier_name(&node_content); + let under_cursor = &ctx.node_under_cursor; match under_cursor.kind() { "column_identifier" => Some(HoveredNode::Column(( ctx.head_qualifier_sanitized(), ctx.tail_qualifier_sanitized(), - node_content, + name, ))), "function_identifier" => Some(HoveredNode::Function(( ctx.tail_qualifier_sanitized(), - node_content, - ))), - "policy_identifier" => Some(HoveredNode::Policy(( - ctx.tail_qualifier_sanitized(), - node_content, - ))), - "table_identifier" => Some(HoveredNode::Table(( - ctx.tail_qualifier_sanitized(), - node_content, + name, ))), + "policy_identifier" => { + Some(HoveredNode::Policy((ctx.tail_qualifier_sanitized(), name))) + } + "table_identifier" => Some(HoveredNode::Table((ctx.tail_qualifier_sanitized(), name))), - "schema_identifier" => Some(HoveredNode::Schema(node_content)), - "role_identifier" => Some(HoveredNode::Role(node_content)), + "schema_identifier" => Some(HoveredNode::Schema(name)), + "role_identifier" => Some(HoveredNode::Role(name)), "any_identifier" if ctx.history_ends_with(&["table_reference", "any_identifier"]) => { - Some(HoveredNode::Table(( - ctx.tail_qualifier_sanitized(), - node_content, - ))) + Some(HoveredNode::Table((ctx.tail_qualifier_sanitized(), name))) } "any_identifier" @@ -65,10 +62,7 @@ impl HoveredNode { ) }) => { - Some(HoveredNode::Table(( - ctx.tail_qualifier_sanitized(), - node_content, - ))) + Some(HoveredNode::Table((ctx.tail_qualifier_sanitized(), name))) } "any_identifier" @@ -81,7 +75,7 @@ impl HoveredNode { Some(HoveredNode::Column(( ctx.head_qualifier_sanitized(), ctx.tail_qualifier_sanitized(), - node_content, + name, ))) } @@ -94,7 +88,7 @@ impl HoveredNode { { Some(HoveredNode::Function(( ctx.tail_qualifier_sanitized(), - node_content, + name, ))) } @@ -112,13 +106,13 @@ impl HoveredNode { // make sure we're not checking against an alias && ctx .get_mentioned_table_for_alias( - node_content.as_str(), + name.as_str(), ) .is_none() => { Some(HoveredNode::PostgresType(( ctx.tail_qualifier_sanitized(), - node_content, + name, ))) } @@ -127,7 +121,7 @@ impl HoveredNode { Some(HoveredNode::Column(( ctx.head_qualifier_sanitized(), ctx.tail_qualifier_sanitized(), - node_content, + name, ))) } diff --git a/crates/pgls_hover/tests/hover_integration_tests.rs b/crates/pgls_hover/tests/hover_integration_tests.rs index 74b6b9b9d..17730e028 100644 --- a/crates/pgls_hover/tests/hover_integration_tests.rs +++ b/crates/pgls_hover/tests/hover_integration_tests.rs @@ -781,3 +781,59 @@ async fn test_table_hover_resolved_by_search_path(test_db: PgPool) { ) .await; } + +const MIXED_CASE_TABLES: &str = r#" + create table "MyTable" (quoted_id int); + create table mytable (folded_id int); + create table "a""b" (escaped_id int); +"#; + +// The statements don't parse, so hover finds the tables by name instead of by reference. + +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn test_table_hover_unquoted_name_is_downcased(test_db: PgPool) { + let query = format!( + "select * from MyTa{}ble where", + QueryWithCursorPosition::cursor_marker() + ); + + test_hover_at_cursor( + "table_hover_unquoted_name_is_downcased", + query, + Some(MIXED_CASE_TABLES), + &test_db, + ) + .await; +} + +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn test_table_hover_quoted_name_keeps_its_case(test_db: PgPool) { + let query = format!( + r#"select * from "MyTa{}ble" where"#, + QueryWithCursorPosition::cursor_marker() + ); + + test_hover_at_cursor( + "table_hover_quoted_name_keeps_its_case", + query, + Some(MIXED_CASE_TABLES), + &test_db, + ) + .await; +} + +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn test_table_hover_quoted_name_with_escaped_quote(test_db: PgPool) { + let query = format!( + r#"select * from "a""{}b" where"#, + QueryWithCursorPosition::cursor_marker() + ); + + test_hover_at_cursor( + "table_hover_quoted_name_with_escaped_quote", + query, + Some(MIXED_CASE_TABLES), + &test_db, + ) + .await; +} diff --git a/crates/pgls_hover/tests/snapshots/table_hover_quoted_name_keeps_its_case.snap b/crates/pgls_hover/tests/snapshots/table_hover_quoted_name_keeps_its_case.snap new file mode 100644 index 000000000..7636d4510 --- /dev/null +++ b/crates/pgls_hover/tests/snapshots/table_hover_quoted_name_keeps_its_case.snap @@ -0,0 +1,22 @@ +--- +source: crates/pgls_hover/tests/hover_integration_tests.rs +expression: snapshot +--- +# Input +```sql +select * from "MyTable" where + ↑ hovered here +``` + +# Hover Results +### `public.MyTable` - 🔓 RLS disabled +```plain +Columns: +- quoted_id: int4 - nullable + +``` +--- +```plain + +~0 rows, ~0 dead rows, 0 B +``` diff --git a/crates/pgls_hover/tests/snapshots/table_hover_quoted_name_with_escaped_quote.snap b/crates/pgls_hover/tests/snapshots/table_hover_quoted_name_with_escaped_quote.snap new file mode 100644 index 000000000..632a19dd2 --- /dev/null +++ b/crates/pgls_hover/tests/snapshots/table_hover_quoted_name_with_escaped_quote.snap @@ -0,0 +1,22 @@ +--- +source: crates/pgls_hover/tests/hover_integration_tests.rs +expression: snapshot +--- +# Input +```sql +select * from "a""b" where + ↑ hovered here +``` + +# Hover Results +### `public.a"b` - 🔓 RLS disabled +```plain +Columns: +- escaped_id: int4 - nullable + +``` +--- +```plain + +~0 rows, ~0 dead rows, 0 B +``` diff --git a/crates/pgls_hover/tests/snapshots/table_hover_unquoted_name_is_downcased.snap b/crates/pgls_hover/tests/snapshots/table_hover_unquoted_name_is_downcased.snap new file mode 100644 index 000000000..98b339f8c --- /dev/null +++ b/crates/pgls_hover/tests/snapshots/table_hover_unquoted_name_is_downcased.snap @@ -0,0 +1,22 @@ +--- +source: crates/pgls_hover/tests/hover_integration_tests.rs +expression: snapshot +--- +# Input +```sql +select * from MyTable where + ↑ hovered here +``` + +# Hover Results +### `public.mytable` - 🔓 RLS disabled +```plain +Columns: +- folded_id: int4 - nullable + +``` +--- +```plain + +~0 rows, ~0 dead rows, 0 B +``` diff --git a/crates/pgls_inlay_hints/src/lib.rs b/crates/pgls_inlay_hints/src/lib.rs index 5125c13d8..a6fa9fbd3 100644 --- a/crates/pgls_inlay_hints/src/lib.rs +++ b/crates/pgls_inlay_hints/src/lib.rs @@ -5,8 +5,8 @@ //! resolution. Calls with unknown argument types, or that match no function or more than one, //! get none. -use pgls_catalog::CatalogView; use pgls_catalog::resolve::{FunctionContext, Reference, ResolveParams, resolve}; +use pgls_catalog::{CatalogView, identifier_name}; use pgls_query::NodeEnum; use pgls_query::protobuf::{ScanToken, Token}; use pgls_text_size::TextSize; @@ -175,7 +175,7 @@ fn is_reference_to(sql: &str, tokens: &[ScanToken], name: &str) -> bool { && tokens .last() .and_then(text) - .is_some_and(|last| normalize_identifier(last) == name) + .is_some_and(|last| identifier_name(last) == name) } fn is_identifier(text: &str) -> bool { @@ -184,14 +184,3 @@ fn is_identifier(text: &str) -> bool { .chars() .all(|c| c.is_alphanumeric() || c == '_' || c == '$') } - -/// The name an identifier refers to: unquoted identifiers are folded to lower case. -fn normalize_identifier(text: &str) -> String { - match text - .strip_prefix('"') - .and_then(|text| text.strip_suffix('"')) - { - Some(quoted) => quoted.replace("\"\"", "\""), - None => text.to_lowercase(), - } -} diff --git a/crates/pgls_inlay_hints/tests/inlay_hints_tests.rs b/crates/pgls_inlay_hints/tests/inlay_hints_tests.rs index 999b5678a..9dc749d55 100644 --- a/crates/pgls_inlay_hints/tests/inlay_hints_tests.rs +++ b/crates/pgls_inlay_hints/tests/inlay_hints_tests.rs @@ -159,3 +159,17 @@ async fn sql_syntax_gets_no_hints(test_db: PgPool) { "select extract(year from now())" ); } + +#[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] +async fn only_ascii_letters_of_unquoted_arguments_are_downcased(test_db: PgPool) { + test_db + .execute(r#"create function accent("Éa" int) returns int language sql as 'select 1'"#) + .await + .expect("Failed to create the function"); + let f = fixture(&test_db).await; + // Like Postgres in a UTF8 database, `ÉA` stands for `Éa`, not `éa`. + assert_eq!( + f.hinted(r#"select accent(t.ÉA) from (select 1 as "Éa") t"#), + r#"select accent(t.ÉA) from (select 1 as "Éa") t"# + ); +} diff --git a/crates/pgls_treesitter/src/context/mod.rs b/crates/pgls_treesitter/src/context/mod.rs index 8819ca78f..efef4930f 100644 --- a/crates/pgls_treesitter/src/context/mod.rs +++ b/crates/pgls_treesitter/src/context/mod.rs @@ -8,6 +8,7 @@ use crate::{ helper, parts_of_reference_query, queries::{self, QueryResult, TreeSitterQueriesExecutor}, }; +use pgls_catalog::identifier_name; use pgls_text_size::TextSize; use tree_sitter::Language; @@ -42,12 +43,22 @@ pub enum WrappingClause<'a> { CheckOrUsingClause, } +/// A column of the statement. Both are names, see [`identifier_name`]. #[derive(PartialEq, Eq, Hash, Debug, Clone)] pub struct MentionedColumn { pub column: String, pub alias: Option, } +/// A table alias of the statement. +#[derive(Debug)] +struct TableAlias { + /// The name of the table. + table: String, + /// The alias as written, like `"U"`. + text: String, +} + /// We can map a few nodes, such as the "update" node, to actual SQL clauses. /// That gives us a lot of insight for completions. /// Other nodes, such as the "relation" node, gives us less but still @@ -130,8 +141,10 @@ pub struct TreesitterContext<'a> { pub previous_clause: Option>, pub current_clause: Option>, + /// The names of the tables of the statement, by the name of their schema. mentioned_relations: HashMap, HashSet>, - mentioned_table_aliases: HashMap, + /// The table aliases of the statement, by the name of the alias. + mentioned_table_aliases: HashMap, mentioned_columns: HashMap>, HashSet>, scope_tracker: ScopeTracker, @@ -183,8 +196,8 @@ impl<'a> TreesitterContext<'a> { for relation_match in executor.get_iter(stmt_range) { match relation_match { QueryResult::Relation(r) => { - let schema_name = r.get_schema(sql); - let table_name = r.get_table(sql); + let schema_name = r.get_schema(sql).as_deref().map(identifier_name); + let table_name = identifier_name(&r.get_table(sql)); self.mentioned_relations .entry(schema_name) @@ -195,16 +208,20 @@ impl<'a> TreesitterContext<'a> { } QueryResult::TableAliases(table_alias_match) => { + let text = table_alias_match.get_alias(sql); self.mentioned_table_aliases.insert( - table_alias_match.get_alias(sql), - table_alias_match.get_table(sql), + identifier_name(&text), + TableAlias { + table: identifier_name(&table_alias_match.get_table(sql)), + text, + }, ); } QueryResult::SelectClauseColumns(c) => { let mentioned = MentionedColumn { - column: c.get_column(sql), - alias: c.get_alias(sql), + column: identifier_name(&c.get_column(sql)), + alias: c.get_alias(sql).as_deref().map(identifier_name), }; self.mentioned_columns @@ -217,8 +234,8 @@ impl<'a> TreesitterContext<'a> { QueryResult::WhereClauseColumns(c) => { let mentioned = MentionedColumn { - column: c.get_column(sql), - alias: c.get_alias(sql), + column: identifier_name(&c.get_column(sql)), + alias: c.get_alias(sql).as_deref().map(identifier_name), }; self.mentioned_columns @@ -231,7 +248,7 @@ impl<'a> TreesitterContext<'a> { QueryResult::InsertClauseColumns(c) => { let mentioned = MentionedColumn { - column: c.get_column(sql), + column: identifier_name(&c.get_column(sql)), alias: None, }; @@ -485,35 +502,25 @@ impl<'a> TreesitterContext<'a> { .is_within_one_of_fields(names) } - pub fn get_mentioned_relations(&self, key: &Option) -> Option<&HashSet> { - if let Some(key) = key.as_ref() { - let sanitized_key = key.replace('"', ""); - - self.mentioned_relations - .get(&Some(sanitized_key.clone())) - .or(self - .mentioned_relations - .get(&Some(format!(r#""{sanitized_key}""#)))) - } else { - self.mentioned_relations.get(&None) - } + /// The names of the tables of the statement in the schema with the name `schema`, or of + /// those without a schema for `None`. + pub fn get_mentioned_relations(&self, schema: &Option) -> Option<&HashSet> { + self.mentioned_relations.get(schema) } - pub fn get_mentioned_table_for_alias(&self, key: &str) -> Option<&String> { - let sanitized_key = key.replace('"', ""); - - self.mentioned_table_aliases.get(&sanitized_key).or(self - .mentioned_table_aliases - .get(&format!(r#""{sanitized_key}""#))) + /// The name of the table that the alias with the name `alias` stands for. + pub fn get_mentioned_table_for_alias(&self, alias: &str) -> Option<&String> { + self.mentioned_table_aliases + .get(alias) + .map(|alias| &alias.table) } + /// An alias of the table with the name `table_name`, as written. pub fn get_used_alias_for_table(&self, table_name: &str) -> Option { - for (alias, table) in self.mentioned_table_aliases.iter() { - if table == table_name { - return Some(alias.to_string()); - } - } - None + self.mentioned_table_aliases + .values() + .find(|alias| alias.table == table_name) + .map(|alias| alias.text.clone()) } pub fn get_mentioned_columns( @@ -535,24 +542,18 @@ impl<'a> TreesitterContext<'a> { !self.mentioned_columns.is_empty() } - /// Returns the head qualifier (leftmost), sanitized (quotes removed) + /// Returns the name of the head qualifier (leftmost), see [`identifier_name`]. /// For `schema.table.`: returns `Some("schema")` /// For `table.`: returns `None` pub fn head_qualifier_sanitized(&self) -> Option { - self.identifier_qualifiers - .0 - .as_ref() - .map(|s| s.replace('"', "")) + self.identifier_qualifiers.0.as_deref().map(identifier_name) } - /// Returns the tail qualifier (rightmost), sanitized (quotes removed) + /// Returns the name of the tail qualifier (rightmost), see [`identifier_name`]. /// For `schema.table.`: returns `Some("table")` /// For `table.`: returns `Some("table")` pub fn tail_qualifier_sanitized(&self) -> Option { - self.identifier_qualifiers - .1 - .as_ref() - .map(|s| s.replace('"', "")) + self.identifier_qualifiers.1.as_deref().map(identifier_name) } /// Returns true if there is at least one qualifier present diff --git a/crates/pgls_treesitter_grammar/grammar.js b/crates/pgls_treesitter_grammar/grammar.js index e1709fff5..955d7d2b2 100644 --- a/crates/pgls_treesitter_grammar/grammar.js +++ b/crates/pgls_treesitter_grammar/grammar.js @@ -3557,7 +3557,7 @@ module.exports = grammar({ _boolean: ($) => choice($.keyword_true, $.keyword_false), - _double_quote_string: (_) => /:?"[^"]*"/, + _double_quote_string: (_) => /:?"([^"]|"")*"/, // The norm specify that between two consecutive string must be a return, // but this is good enough. _single_quote_string: (_) => diff --git a/crates/pgls_treesitter_grammar/tests/grammar_tests.rs b/crates/pgls_treesitter_grammar/tests/grammar_tests.rs index 3b0d519b2..668722a30 100644 --- a/crates/pgls_treesitter_grammar/tests/grammar_tests.rs +++ b/crates/pgls_treesitter_grammar/tests/grammar_tests.rs @@ -87,3 +87,10 @@ fn test_6() { file_snapshot("test_6", sql); } + +#[test] +fn quoted_identifier_with_escaped_quote() { + let sql = r#"select "a""b".id from "a""b";"#; + + file_snapshot("quoted_identifier_with_escaped_quote", sql); +} diff --git a/crates/pgls_treesitter_grammar/tests/snapshots/grammar_tests__quoted_identifier_with_escaped_quote.snap b/crates/pgls_treesitter_grammar/tests/snapshots/grammar_tests__quoted_identifier_with_escaped_quote.snap new file mode 100644 index 000000000..0f3cf4ae2 --- /dev/null +++ b/crates/pgls_treesitter_grammar/tests/snapshots/grammar_tests__quoted_identifier_with_escaped_quote.snap @@ -0,0 +1,22 @@ +--- +source: crates/pgls_treesitter_grammar/tests/grammar_tests.rs +expression: writer +--- +select "a""b".id from "a""b"; +----------------------- +program [0..29] 'select "a""b".id from "a""b";' + statement [0..28] 'select "a""b".id from "a""b"' + select [0..16] 'select "a""b".id' + keyword_select [0..6] 'select' + select_expression [7..16] '"a""b".id' (@end) + term [7..16] '"a""b".id' (@end) + object_reference [7..16] '"a""b".id' (@end) + any_identifier [7..13] '"a""b"' (@object_reference_1of2) + . [13..14] '.' + any_identifier [14..16] 'id' (@object_reference_2of2) + from [17..28] 'from "a""b"' + keyword_from [17..21] 'from' + relation [22..28] '"a""b"' (@end) + table_reference [22..28] '"a""b"' (@end) + any_identifier [22..28] '"a""b"' (@table_reference_1of1) + ; [28..29] ';' From 0052ccea2221af1b4b6775cae6446dc1fb18717d Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sun, 25 Oct 2026 17:25:46 +0000 Subject: [PATCH 2/5] fix(completions): quote names that need it when inserting them Completions inserted table, column, function and schema names as they are stored, so MyTable, names with spaces or quotes, and reserved keywords were read by Postgres as another name, or not at all. They are now quoted, with " escaped as "", exactly where Postgres needs it: pgls_catalog::quote_identifier follows scan.l's identifier rules and the keywords that gram.y accepts at each position. --- Cargo.lock | 1 + crates/pgls_catalog/Cargo.toml | 1 + crates/pgls_catalog/src/identifier.rs | 92 ++++++++++++++- crates/pgls_catalog/src/lib.rs | 5 +- .../pgls_completions/src/providers/columns.rs | 16 ++- .../src/providers/functions.rs | 9 +- .../pgls_completions/src/providers/helper.rs | 108 +++++++++++++++--- .../pgls_completions/src/providers/schemas.rs | 12 +- .../pgls_completions/src/providers/tables.rs | 9 +- ...test_helper__completes_quoted_columns.snap | 10 +- ...completes_quoted_columns_with_aliases.snap | 60 +++++----- ...ests_relevant_columns_without_letters.snap | 2 +- crates/pgls_lexer/src/lib.rs | 5 + 13 files changed, 266 insertions(+), 64 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9860413af..70303daa6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2414,6 +2414,7 @@ dependencies = [ "futures-util", "globset", "insta", + "pgls_lexer", "pgls_query", "pgls_test_utils", "pgls_text_size", diff --git a/crates/pgls_catalog/Cargo.toml b/crates/pgls_catalog/Cargo.toml index 0eef5a7d4..264610496 100644 --- a/crates/pgls_catalog/Cargo.toml +++ b/crates/pgls_catalog/Cargo.toml @@ -19,6 +19,7 @@ schema = ["dep:schemars"] [dependencies] futures-util = { version = "0.3.31", optional = true } globset = "0.4.16" +pgls_lexer = { workspace = true } pgls_query = { workspace = true } pgls_text_size = { workspace = true } rustc-hash = { workspace = true } diff --git a/crates/pgls_catalog/src/identifier.rs b/crates/pgls_catalog/src/identifier.rs index ec72a8eb0..c60a8cac8 100644 --- a/crates/pgls_catalog/src/identifier.rs +++ b/crates/pgls_catalog/src/identifier.rs @@ -1,5 +1,9 @@ //! Identifiers in SQL text and the names they stand for. +use std::borrow::Cow; + +use pgls_lexer::{KeywordCategory, keyword_category}; + /// The name an identifier as written in SQL stands for, like Postgres' scanner reads it: a /// quoted identifier without its quotes and with `""` unescaped, an unquoted identifier /// downcased. Like [`downcase_identifier`] in a multibyte encoding such as UTF8, only ASCII @@ -19,9 +23,58 @@ pub fn identifier_name(text: &str) -> String { } } +/// Where a name is written, which decides the keywords it can be without quotes. See the name +/// classification hierarchy in [`gram.y`]. +/// +/// [`gram.y`]: https://github.com/postgres/postgres/blob/REL_18_6/src/backend/parser/gram.y#L17620 +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum NamePosition { + /// The first part of a column, table or schema name (`ColId`): any keyword except the + /// reserved ones and those that name types or functions. + ColId, + /// An unqualified function or type name (`type_function_name`): any keyword except the + /// reserved ones and those that name columns. + TypeFunctionName, + /// A name after a dot (`attr_name`, a `ColLabel`): any keyword. + ColLabel, +} + +/// The identifier to write in SQL for `name`: the name itself if Postgres reads it back as the +/// same name, else the name quoted, with `"` escaped as `""`. The inverse of +/// [`identifier_name`]. +pub fn quote_identifier(name: &str, position: NamePosition) -> Cow<'_, str> { + if needs_quotes(name, position) { + Cow::Owned(format!("\"{}\"", name.replace('"', "\"\""))) + } else { + Cow::Borrowed(name) + } +} + +/// Whether `name` must be quoted at `position`. Unquoted, it must be an identifier by +/// [`scan.l`]'s rules that downcasing leaves unchanged, and not a keyword that can't be used +/// there. +/// +/// [`scan.l`]: https://github.com/postgres/postgres/blob/REL_18_6/src/backend/parser/scan.l#L332 +pub fn needs_quotes(name: &str, position: NamePosition) -> bool { + let mut chars = name.chars(); + let Some(first) = chars.next() else { + return true; + }; + let is_start = |c: char| c.is_ascii_lowercase() || c == '_' || !c.is_ascii(); + if !is_start(first) || !chars.all(|c| is_start(c) || c.is_ascii_digit() || c == '$') { + return true; + } + match keyword_category(name) { + None | Some(KeywordCategory::Unreserved) => false, + Some(KeywordCategory::Reserved) => position != NamePosition::ColLabel, + Some(KeywordCategory::ColName) => position == NamePosition::TypeFunctionName, + Some(KeywordCategory::TypeFuncName) => position == NamePosition::ColId, + } +} + #[cfg(test)] mod tests { - use super::identifier_name; + use super::{NamePosition, identifier_name, needs_quotes, quote_identifier}; #[test] fn unquoted_identifiers_are_downcased() { @@ -43,4 +96,41 @@ mod tests { assert_eq!(identifier_name(r#""Foo"#), r#""foo"#); assert_eq!(identifier_name(r#"""#), r#"""#); } + + #[test] + fn names_are_quoted_when_postgres_would_read_another_name() { + let quoted = |name| quote_identifier(name, NamePosition::ColId).into_owned(); + assert_eq!(quoted("users"), "users"); + assert_eq!(quoted("_t1$x"), "_t1$x"); + assert_eq!(quoted("Äbc"), "Äbc"); + assert_eq!(quoted("MyTable"), r#""MyTable""#); + assert_eq!(quoted("my table"), r#""my table""#); + assert_eq!(quoted("1st"), r#""1st""#); + assert_eq!(quoted(r#"a"b"#), r#""a""b""#); + assert_eq!(quoted(""), r#""""#); + for name in ["users", "MyTable", "my table", r#"a"b"#] { + assert_eq!(identifier_name("ed(name)), name); + } + } + + #[test] + fn keywords_are_quoted_where_the_grammar_rejects_them() { + use NamePosition::*; + // Unreserved: `name`. Column name: `between`. Type or function name: `left`. + // Reserved: `select`. + for (name, col_id, type_function_name) in [ + ("name", false, false), + ("between", false, true), + ("left", true, false), + ("select", true, true), + ] { + assert_eq!(needs_quotes(name, ColId), col_id, "{name}"); + assert_eq!( + needs_quotes(name, TypeFunctionName), + type_function_name, + "{name}" + ); + assert!(!needs_quotes(name, ColLabel), "{name}"); + } + } } diff --git a/crates/pgls_catalog/src/lib.rs b/crates/pgls_catalog/src/lib.rs index 21901d992..a1bfc1f8f 100644 --- a/crates/pgls_catalog/src/lib.rs +++ b/crates/pgls_catalog/src/lib.rs @@ -11,7 +11,8 @@ //! - [`CatalogView`] (`lookup.rs`): the lookups the resolver makes. [`Catalog`] implements it. //! - [`resolve`] (`resolve/`): name resolution of a single statement against a [`CatalogView`], //! with one module per parse node in `resolve/nodes/`. -//! - [`identifier_name`] (`identifier.rs`): the name an identifier in SQL text stands for. +//! - [`identifier_name`] (`identifier.rs`): the name an identifier in SQL text stands for, and +//! [`quote_identifier`] the identifier to write for a name. //! //! The analyser walks a file statement by statement. For each statement it runs the rules, //! which resolve names against the current [`Catalog`] with the [`Session`]'s search path, and @@ -27,7 +28,7 @@ pub mod snapshot; pub mod typing; pub use catalog::{Catalog, CatalogBase}; -pub use identifier::identifier_name; +pub use identifier::{NamePosition, identifier_name, needs_quotes, quote_identifier}; pub use lookup::{ Candidates, CastContext, CastInfo, CastMethod, CatalogView, ColumnInfo, FunctionArgument, FunctionArgumentMode, FunctionInfo, FunctionKind, FunctionSignature, Lookup, OperatorInfo, diff --git a/crates/pgls_completions/src/providers/columns.rs b/crates/pgls_completions/src/providers/columns.rs index e90d6deaf..dd82094a9 100644 --- a/crates/pgls_completions/src/providers/columns.rs +++ b/crates/pgls_completions/src/providers/columns.rs @@ -1,4 +1,4 @@ -use pgls_catalog::{Column, Snapshot}; +use pgls_catalog::{Column, NamePosition, Snapshot, identifier_name}; use pgls_treesitter::TreesitterContext; use crate::{ @@ -35,9 +35,17 @@ pub fn complete_columns<'a>( } fn get_completion_text(ctx: &TreesitterContext, col: &Column) -> CompletionText { - let alias = ctx.get_used_alias_for_table(col.table_name.as_str()); - - let with_schema_or_alias = with_schema_or_alias(ctx, col.name.as_str(), alias.as_deref()); + // The alias as a name, which is quoted again if needed. + let alias = ctx + .get_used_alias_for_table(col.table_name.as_str()) + .map(|alias| identifier_name(&alias)); + + let with_schema_or_alias = with_schema_or_alias( + ctx, + col.name.as_str(), + alias.as_deref(), + NamePosition::ColId, + ); let range = get_range_to_replace(ctx); diff --git a/crates/pgls_completions/src/providers/functions.rs b/crates/pgls_completions/src/providers/functions.rs index 45f57d838..816b75bc0 100644 --- a/crates/pgls_completions/src/providers/functions.rs +++ b/crates/pgls_completions/src/providers/functions.rs @@ -1,4 +1,4 @@ -use pgls_catalog::{Function, Snapshot}; +use pgls_catalog::{Function, NamePosition, Snapshot}; use pgls_treesitter::TreesitterContext; use crate::{ @@ -35,7 +35,12 @@ pub fn complete_functions<'a>( } fn get_completion_text(ctx: &TreesitterContext, func: &Function) -> CompletionText { - let mut text = with_schema_or_alias(ctx, func.name.as_str(), Some(func.schema.as_str())); + let mut text = with_schema_or_alias( + ctx, + func.name.as_str(), + Some(func.schema.as_str()), + NamePosition::TypeFunctionName, + ); let mut range = get_range_to_replace(ctx); diff --git a/crates/pgls_completions/src/providers/helper.rs b/crates/pgls_completions/src/providers/helper.rs index 790e96f3c..14aeef7ec 100644 --- a/crates/pgls_completions/src/providers/helper.rs +++ b/crates/pgls_completions/src/providers/helper.rs @@ -1,3 +1,4 @@ +use pgls_catalog::{NamePosition, quote_identifier}; use pgls_text_size::TextRange; use pgls_treesitter::TreesitterContext; @@ -36,31 +37,110 @@ pub(crate) fn only_leading_quote(ctx: &TreesitterContext) -> bool { is_sanitized_token_with_quote(node_under_cursor_txt) } +/// The text to insert for `item_name`, prefixed with the schema or alias name unless it is +/// `public` or the user already wrote a qualifier. Names are quoted where Postgres would read +/// another name unquoted. `position` is where an unqualified `item_name` stands. pub(crate) fn with_schema_or_alias( ctx: &TreesitterContext, item_name: &str, schema_or_alias_name: Option<&str>, + position: NamePosition, ) -> String { let is_already_prefixed_with_schema_name = ctx.has_any_qualifier(); let with_quotes = node_text_surrounded_by_quotes(ctx); let single_leading_quote = only_leading_quote(ctx); - if schema_or_alias_name.is_none_or(|s| s == "public") || is_already_prefixed_with_schema_name { - if single_leading_quote { - format!(r#"{item_name}""#) - } else { - item_name.to_string() + // Inside quotes the user already typed, a name only needs its `"` escaped. + let escaped = |name: &str| name.replace('"', r#""""#); + + match schema_or_alias_name { + Some(prefix) if prefix != "public" && !is_already_prefixed_with_schema_name => { + if single_leading_quote { + format!(r#"{}"."{}""#, escaped(prefix), escaped(item_name)) + } else if with_quotes { + format!(r#"{}"."{}"#, escaped(prefix), escaped(item_name)) + } else { + format!( + "{}.{}", + quote_identifier(prefix, NamePosition::ColId), + quote_identifier(item_name, NamePosition::ColLabel) + ) + } + } + _ => { + if single_leading_quote { + format!(r#"{}""#, escaped(item_name)) + } else if with_quotes { + escaped(item_name) + } else if is_already_prefixed_with_schema_name { + quote_identifier(item_name, NamePosition::ColLabel).into_owned() + } else { + quote_identifier(item_name, position).into_owned() + } } - } else { - let schema_or_als = schema_or_alias_name.unwrap(); - - if single_leading_quote { - format!(r#"{}"."{}""#, schema_or_als.replace('"', ""), item_name) - } else if with_quotes { - format!(r#"{}"."{}"#, schema_or_als.replace('"', ""), item_name) - } else { - format!("{schema_or_als}.{item_name}") + } +} + +#[cfg(test)] +mod tests { + use pgls_test_utils::QueryWithCursorPosition; + use sqlx::{Executor, PgPool}; + + use crate::{ + complete, + test_helper::{get_test_deps, get_test_params}, + }; + + /// The text inserted for the completion item `label` at `{c}` in `query`. + async fn inserted(pool: &PgPool, query: &str, label: &str) -> Option { + let query = query.replace("{c}", &QueryWithCursorPosition::cursor_marker().to_string()); + let (tree, cache) = get_test_deps(None, query.as_str().into(), pool).await; + let params = get_test_params(&tree, &cache, query.as_str().into()); + complete(params) + .into_iter() + .find(|item| item.label == label) + .map(|item| item.completion_text.map_or(item.label, |text| text.text)) + } + + #[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] + async fn quotes_names_that_need_it(pool: PgPool) { + pool.execute( + r#" + create schema "My Schema"; + create table "MyTable" ("Id" int, "select" int, plain int); + create table "My Schema"."Other" (x int); + create function "MyFunc"() returns int language sql as 'select 1'; + create function "My Schema".helper() returns int language sql as 'select 1'; + "#, + ) + .await + .unwrap(); + + for (query, label, expected) in [ + // Tables and schemas. + ("select * from {c}", "MyTable", r#""MyTable""#), + ("select * from {c}", "My Schema", r#""My Schema""#), + ("select * from {c}", "Other", r#""My Schema"."Other""#), + (r#"select * from "My Schema".{c}"#, "Other", r#""Other""#), + // After an opening quote, only the closing one is missing. + (r#"select * from "{c}"#, "MyTable", r#"MyTable""#), + (r#"select * from "{c}"#, "Other", r#"My Schema"."Other""#), + // Columns. A keyword needs quotes on its own, but not after a dot. + (r#"select {c} from "MyTable""#, "Id", r#""Id""#), + (r#"select {c} from "MyTable""#, "select", r#""select""#), + (r#"select {c} from "MyTable""#, "plain", "plain"), + (r#"select t.{c} from "MyTable" t"#, "select", "select"), + (r#"select t.{c} from "MyTable" t"#, "Id", r#""Id""#), + // Functions. + ("select {c}", "MyFunc", r#""MyFunc"()"#), + ("select {c}", "helper", r#""My Schema".helper()"#), + ] { + assert_eq!( + inserted(&pool, query, label).await.as_deref(), + Some(expected), + "{query} → {label}" + ); } } } diff --git a/crates/pgls_completions/src/providers/schemas.rs b/crates/pgls_completions/src/providers/schemas.rs index a17946b05..aef029b32 100644 --- a/crates/pgls_completions/src/providers/schemas.rs +++ b/crates/pgls_completions/src/providers/schemas.rs @@ -1,12 +1,14 @@ +use super::helper::{get_range_to_replace, with_schema_or_alias}; use crate::{ + CompletionText, builder::{CompletionBuilder, PossibleCompletionItem}, relevance::{CompletionRelevanceData, filtering::CompletionFilter, scoring::CompletionScore}, }; -use pgls_catalog::Snapshot; +use pgls_catalog::{NamePosition, Snapshot}; use pgls_treesitter::TreesitterContext; pub fn complete_schemas<'a>( - _ctx: &'a TreesitterContext, + ctx: &'a TreesitterContext, snapshot: &'a Snapshot, builder: &mut CompletionBuilder<'a>, ) { @@ -22,7 +24,11 @@ pub fn complete_schemas<'a>( score: CompletionScore::from(relevance.clone()), filter: CompletionFilter::from(relevance), detail: None, - completion_text: None, + completion_text: Some(CompletionText { + is_snippet: false, + range: get_range_to_replace(ctx), + text: with_schema_or_alias(ctx, schema.name.as_str(), None, NamePosition::ColId), + }), }; builder.add_item(item); diff --git a/crates/pgls_completions/src/providers/tables.rs b/crates/pgls_completions/src/providers/tables.rs index 0b4b41185..937cbed0f 100644 --- a/crates/pgls_completions/src/providers/tables.rs +++ b/crates/pgls_completions/src/providers/tables.rs @@ -1,4 +1,4 @@ -use pgls_catalog::{Snapshot, Table}; +use pgls_catalog::{NamePosition, Snapshot, Table}; use pgls_treesitter::TreesitterContext; use crate::{ @@ -42,7 +42,12 @@ pub fn complete_tables<'a>( } fn get_completion_text(ctx: &TreesitterContext, table: &Table) -> CompletionText { - let text = with_schema_or_alias(ctx, table.name.as_str(), Some(table.schema.as_str())); + let text = with_schema_or_alias( + ctx, + table.name.as_str(), + Some(table.schema.as_str()), + NamePosition::ColId, + ); let range = get_range_to_replace(ctx); diff --git a/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns.snap b/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns.snap index 48a8b1d9a..fa9303a00 100644 --- a/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns.snap +++ b/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns.snap @@ -33,7 +33,7 @@ email - private.users.email (Column) id - private.users.id (Column) name - private.users.name (Column) quoted_column - private.users.quoted_column (Column) -pg_catalog.RI_FKey_cascade_del() - Schema: pg_catalog.RI_FKey_cascade_del (Function) +pg_catalog."RI_FKey_cascade_del"() - Schema: pg_catalog.RI_FKey_cascade_del (Function) -------------- @@ -104,10 +104,10 @@ private.users - private.users (Table) select "email" from "| Results: -private - private (Schema) -information_schema - information_schema (Schema) -pg_catalog - pg_catalog (Schema) -pg_toast - pg_toast (Schema) +private" - private (Schema) +information_schema" - information_schema (Schema) +pg_catalog" - pg_catalog (Schema) +pg_toast" - pg_toast (Schema) private"."users" - private.users (Table) -------------- diff --git a/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns_with_aliases.snap b/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns_with_aliases.snap index ba6ed1d84..3c35a7a03 100644 --- a/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns_with_aliases.snap +++ b/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__completes_quoted_columns_with_aliases.snap @@ -173,10 +173,10 @@ s| from private.users "pr" select | from private.users "pr" Results: -"pr".email - private.users.email (Column) -"pr".id - private.users.id (Column) -"pr".name - private.users.name (Column) -"pr".quoted_column - private.users.quoted_column (Column) +pr.email - private.users.email (Column) +pr.id - private.users.id (Column) +pr.name - private.users.name (Column) +pr.quoted_column - private.users.quoted_column (Column) name - public.names.name (Column) -------------- @@ -225,10 +225,10 @@ s| from private.users "pr" select | from private.users "pr" Results: -"pr".email - private.users.email (Column) -"pr".id - private.users.id (Column) -"pr".name - private.users.name (Column) -"pr".quoted_column - private.users.quoted_column (Column) +pr.email - private.users.email (Column) +pr.id - private.users.id (Column) +pr.name - private.users.name (Column) +pr.quoted_column - private.users.quoted_column (Column) name - public.names.name (Column) -------------- @@ -236,10 +236,10 @@ name - public.names.name (Column) select p| from private.users "pr" Results: -"pr".email - private.users.email (Column) -"pr".id - private.users.id (Column) -"pr".name - private.users.name (Column) -"pr".quoted_column - private.users.quoted_column (Column) +pr.email - private.users.email (Column) +pr.id - private.users.id (Column) +pr.name - private.users.name (Column) +pr.quoted_column - private.users.quoted_column (Column) pad_attribute - information_schema.collations.pad_attribute (Column) -------------- @@ -294,10 +294,10 @@ s| from private.users "pr" select | from private.users "pr" Results: -"pr".email - private.users.email (Column) -"pr".id - private.users.id (Column) -"pr".name - private.users.name (Column) -"pr".quoted_column - private.users.quoted_column (Column) +pr.email - private.users.email (Column) +pr.id - private.users.id (Column) +pr.name - private.users.name (Column) +pr.quoted_column - private.users.quoted_column (Column) name - public.names.name (Column) -------------- @@ -385,10 +385,10 @@ s| from private.users "pr" select | from private.users "pr" Results: -"pr".email - private.users.email (Column) -"pr".id - private.users.id (Column) -"pr".name - private.users.name (Column) -"pr".quoted_column - private.users.quoted_column (Column) +pr.email - private.users.email (Column) +pr.id - private.users.id (Column) +pr.name - private.users.name (Column) +pr.quoted_column - private.users.quoted_column (Column) name - public.names.name (Column) -------------- @@ -396,10 +396,10 @@ name - public.names.name (Column) select p| from private.users "pr" Results: -"pr".email - private.users.email (Column) -"pr".id - private.users.id (Column) -"pr".name - private.users.name (Column) -"pr".quoted_column - private.users.quoted_column (Column) +pr.email - private.users.email (Column) +pr.id - private.users.id (Column) +pr.name - private.users.name (Column) +pr.quoted_column - private.users.quoted_column (Column) pad_attribute - information_schema.collations.pad_attribute (Column) -------------- @@ -437,9 +437,9 @@ select | from private.users "pr" join public.names n on pr.id = n.uid; Results: n.name - public.names.name (Column) n.uid - public.names.uid (Column) -"pr".email - private.users.email (Column) -"pr".id - private.users.id (Column) -"pr".name - private.users.name (Column) +pr.email - private.users.email (Column) +pr.id - private.users.id (Column) +pr.name - private.users.name (Column) -------------- @@ -500,9 +500,9 @@ select "pr"."email", | from private.users "pr" join public.names n on pr.id = n. Results: n.name - public.names.name (Column) n.uid - public.names.uid (Column) -"pr".id - private.users.id (Column) -"pr".name - private.users.name (Column) -"pr".quoted_column - private.users.quoted_column (Column) +pr.id - private.users.id (Column) +pr.name - private.users.name (Column) +pr.quoted_column - private.users.quoted_column (Column) -------------- diff --git a/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__suggests_relevant_columns_without_letters.snap b/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__suggests_relevant_columns_without_letters.snap index 764f01ab6..e46596bff 100644 --- a/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__suggests_relevant_columns_without_letters.snap +++ b/crates/pgls_completions/src/snapshots/pgls_completions__test_helper__suggests_relevant_columns_without_letters.snap @@ -32,7 +32,7 @@ address - public.users.address (Column) email - public.users.email (Column) id - public.users.id (Column) name - public.users.name (Column) -pg_catalog.RI_FKey_cascade_del() - Schema: pg_catalog.RI_FKey_cascade_del (Function) +pg_catalog."RI_FKey_cascade_del"() - Schema: pg_catalog.RI_FKey_cascade_del (Function) -------------- diff --git a/crates/pgls_lexer/src/lib.rs b/crates/pgls_lexer/src/lib.rs index fdd46cd3e..7339600cf 100644 --- a/crates/pgls_lexer/src/lib.rs +++ b/crates/pgls_lexer/src/lib.rs @@ -11,6 +11,11 @@ pub use crate::params::{ convert_to_positional_params_with_metadata, }; +/// The category of the keyword `word`, compared case-insensitively. `None` if it's no keyword. +pub fn keyword_category(word: &str) -> Option { + SyntaxKind::from_keyword(word)?.keyword_category() +} + /// Lex the input string into tokens and diagnostics pub fn lex(input: &str) -> Lexed<'_> { Lexer::new(input).lex() From 6b242405a09e19911b6b08650f2ad09e09826172 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sat, 31 Oct 2026 09:08:28 +0000 Subject: [PATCH 3/5] fix(catalog): quote a function named operator operator( starts an operator like OPERATOR(pg_catalog.+), so completing a function named operator inserted operator(), which doesn't parse. --- crates/pgls_catalog/src/identifier.rs | 43 ++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/crates/pgls_catalog/src/identifier.rs b/crates/pgls_catalog/src/identifier.rs index c60a8cac8..87172d964 100644 --- a/crates/pgls_catalog/src/identifier.rs +++ b/crates/pgls_catalog/src/identifier.rs @@ -33,7 +33,10 @@ pub enum NamePosition { /// reserved ones and those that name types or functions. ColId, /// An unqualified function or type name (`type_function_name`): any keyword except the - /// reserved ones and those that name columns. + /// reserved ones and those that name columns, and except `operator`, which starts an + /// operator like `OPERATOR(pg_catalog.+)` when a `(` follows ([`qual_Op`]). + /// + /// [`qual_Op`]: https://github.com/postgres/postgres/blob/REL_18_6/src/backend/parser/gram.y#L16658 TypeFunctionName, /// A name after a dot (`attr_name`, a `ColLabel`): any keyword. ColLabel, @@ -64,6 +67,9 @@ pub fn needs_quotes(name: &str, position: NamePosition) -> bool { if !is_start(first) || !chars.all(|c| is_start(c) || c.is_ascii_digit() || c == '$') { return true; } + if position == NamePosition::TypeFunctionName && name == "operator" { + return true; + } match keyword_category(name) { None | Some(KeywordCategory::Unreserved) => false, Some(KeywordCategory::Reserved) => position != NamePosition::ColLabel, @@ -133,4 +139,39 @@ mod tests { assert!(!needs_quotes(name, ColLabel), "{name}"); } } + + /// Every keyword, written by [`quote_identifier`] at each position, parses, e.g. a function + /// named `operator` is called as `"operator"()`. + #[test] + fn keywords_round_trip_through_the_parser() { + use NamePosition::*; + let kwlist = include_str!( + "../../pgls_query/vendor/libpg_query/src/postgres/include/parser/kwlist.h" + ); + let keywords = kwlist + .lines() + .filter_map(|line| line.strip_prefix("PG_KEYWORD(\"")) + .filter_map(|line| line.split('"').next()); + let mut failures = Vec::new(); + for keyword in keywords { + let col_id = quote_identifier(keyword, ColId); + let function = quote_identifier(keyword, TypeFunctionName); + let label = quote_identifier(keyword, ColLabel); + for sql in [ + format!("select {col_id} from {col_id}.t {col_id} where {col_id} = 1"), + format!("select {col_id}.{label}, s.{label}() from {col_id}"), + format!("select {function}(), a + {function}(1)"), + ] { + if pgls_query::parse(&sql).is_err() { + failures.push(sql); + } + } + } + assert!(failures.is_empty(), "{failures:#?}"); + assert_eq!( + quote_identifier("operator", TypeFunctionName), + r#""operator""# + ); + assert_eq!(quote_identifier("operator", ColId), "operator"); + } } From e3cc5ce27dd4415c5da5c57014270459a2a09008 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sat, 31 Oct 2026 09:12:11 +0000 Subject: [PATCH 4/5] fix(completions): read the text around the cursor by bytes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cursor is a byte offset, but the sanitization indexed the statement by chars. After a multibyte character the token went in after the cursor, so the range to replace, which schemas now set too, didn't contain the cursor: select 'é' from | ; replaced 18..18 for a cursor at 17. The same change as in #966 by @ilia-maslakov, so the two merge cleanly. --- .../pgls_completions/src/providers/helper.rs | 36 ++++- crates/pgls_completions/src/sanitization.rs | 147 +++++++++++------- 2 files changed, 128 insertions(+), 55 deletions(-) diff --git a/crates/pgls_completions/src/providers/helper.rs b/crates/pgls_completions/src/providers/helper.rs index 14aeef7ec..c8ecadfff 100644 --- a/crates/pgls_completions/src/providers/helper.rs +++ b/crates/pgls_completions/src/providers/helper.rs @@ -85,21 +85,28 @@ pub(crate) fn with_schema_or_alias( #[cfg(test)] mod tests { use pgls_test_utils::QueryWithCursorPosition; + use pgls_text_size::TextRange; use sqlx::{Executor, PgPool}; use crate::{ - complete, + CompletionItem, complete, test_helper::{get_test_deps, get_test_params}, }; - /// The text inserted for the completion item `label` at `{c}` in `query`. - async fn inserted(pool: &PgPool, query: &str, label: &str) -> Option { + /// The completion item `label` at `{c}` in `query`. + async fn item(pool: &PgPool, query: &str, label: &str) -> Option { let query = query.replace("{c}", &QueryWithCursorPosition::cursor_marker().to_string()); let (tree, cache) = get_test_deps(None, query.as_str().into(), pool).await; let params = get_test_params(&tree, &cache, query.as_str().into()); complete(params) .into_iter() .find(|item| item.label == label) + } + + /// The text inserted for the completion item `label` at `{c}` in `query`. + async fn inserted(pool: &PgPool, query: &str, label: &str) -> Option { + item(pool, query, label) + .await .map(|item| item.completion_text.map_or(item.label, |text| text.text)) } @@ -143,4 +150,27 @@ mod tests { ); } } + + /// The range to replace is in bytes, like the cursor, also after multibyte characters. + #[sqlx::test(migrator = "pgls_test_utils::MIGRATIONS")] + async fn replaces_at_the_cursor_after_multibyte_characters(pool: PgPool) { + pool.execute("create schema private; create table people (name text);") + .await + .unwrap(); + + for (query, label) in [ + ("select 'é' from {c} ;", "private"), + ("select 'é' from {c}", "private"), + ("select 'é', {c} from people", "name"), + ] { + let cursor = query.find("{c}").unwrap() as u32; + let item = item(&pool, query, label).await; + assert_eq!( + item.and_then(|item| item.completion_text) + .map(|text| text.range), + Some(TextRange::empty(cursor.into())), + "{query} → {label}" + ); + } + } } diff --git a/crates/pgls_completions/src/sanitization.rs b/crates/pgls_completions/src/sanitization.rs index 6c66342d8..0cdbe4368 100644 --- a/crates/pgls_completions/src/sanitization.rs +++ b/crates/pgls_completions/src/sanitization.rs @@ -1,4 +1,4 @@ -use std::{borrow::Cow, cmp::max}; +use std::borrow::Cow; use pgls_text_size::TextSize; @@ -73,48 +73,44 @@ where 'larger: 'smaller, { fn with_adjusted_sql(params: CompletionParams<'larger>) -> Self { + // the position is a byte offset: the text is walked by its bytes, not by its chars, + // or a multi-byte char before the cursor puts the token after it let cursor_pos: usize = params.position.into(); let mut sql = String::new(); - let mut sql_iter = params.text.chars(); - - let max = max(cursor_pos + 1, params.text.len()); - let has_uneven_quotes = params.text.chars().filter(|c| *c == '"').count() % 2 != 0; let mut opened_quote = false; + let mut inserted = false; - for idx in 0..max { - match sql_iter.next() { - Some(c) => { - if c == '"' { - opened_quote = !opened_quote; - } - - if idx == cursor_pos { - if opened_quote && has_uneven_quotes { - sql.push_str(SANITIZED_TOKEN_WITH_QUOTE); - opened_quote = false; - } else { - sql.push_str(SANITIZED_TOKEN); - } - } - - sql.push(c); - } - None => { - // the cursor is outside the statement, - // we want to push spaces until we arrive at the cursor position. - // we'll then add the SANITIZED_TOKEN - if idx == cursor_pos { - if opened_quote && has_uneven_quotes { - sql.push_str(SANITIZED_TOKEN_WITH_QUOTE); - } else { - sql.push_str(SANITIZED_TOKEN); - } - } else { - sql.push(' '); - } + for (idx, c) in params.text.char_indices() { + if c == '"' { + opened_quote = !opened_quote; + } + + if idx == cursor_pos { + if opened_quote && has_uneven_quotes { + sql.push_str(SANITIZED_TOKEN_WITH_QUOTE); + opened_quote = false; + } else { + sql.push_str(SANITIZED_TOKEN); } + inserted = true; + } + + sql.push(c); + } + + if !inserted { + // the cursor is outside the statement, + // we want to push spaces until we arrive at the cursor position. + // we'll then add the SANITIZED_TOKEN + for _ in params.text.len()..cursor_pos { + sql.push(' '); + } + if opened_quote && has_uneven_quotes { + sql.push_str(SANITIZED_TOKEN_WITH_QUOTE); + } else { + sql.push_str(SANITIZED_TOKEN); } } @@ -142,6 +138,16 @@ where } } +/// The char that ends right at the byte offset `position`, if any. +fn char_before(sql: &str, position: usize) -> Option { + sql.get(..position).and_then(|s| s.chars().next_back()) +} + +/// The char that starts at the byte offset `position`, if any. +fn char_at(sql: &str, position: usize) -> Option { + sql.get(position..).and_then(|s| s.chars().next()) +} + /// Checks if the cursor is positioned inbetween two SQL nodes. /// /// ```sql @@ -151,13 +157,10 @@ where /// ``` fn cursor_inbetween_nodes(sql: &str, position: TextSize) -> bool { let position: usize = position.into(); - let mut chars = sql.chars(); - let previous_whitespace = chars - .nth(position.saturating_sub(1)) - .is_some_and(|c| c.is_ascii_whitespace()); + let previous_whitespace = char_before(sql, position).is_some_and(|c| c.is_ascii_whitespace()); - let current_whitespace = chars.next().is_some_and(|c| c.is_ascii_whitespace()); + let current_whitespace = char_at(sql, position).is_some_and(|c| c.is_ascii_whitespace()); previous_whitespace && current_whitespace } @@ -177,9 +180,7 @@ fn cursor_prepared_to_write_token_after_last_node(sql: &str, position: TextSize) fn cursor_on_a_dot(sql: &str, position: TextSize) -> bool { let position: usize = position.into(); - sql.chars() - .nth(position.saturating_sub(1)) - .is_some_and(|c| c == '.') + char_before(sql, position).is_some_and(|c| c == '.') } fn cursor_before_semicolon(tree: &tree_sitter::Tree, position: TextSize) -> bool { @@ -230,7 +231,7 @@ fn cursor_between_parentheses(sql: &str, position: TextSize) -> bool { let mut matching_open_idx = None; let mut matching_close_idx = None; - for (idx, char) in sql.chars().enumerate() { + for (idx, char) in sql.char_indices() { if char == '(' { tracking_open_idx = Some(idx); level += 1; @@ -253,11 +254,8 @@ fn cursor_between_parentheses(sql: &str, position: TextSize) -> bool { // early check: '(|)' // however, we want to check this after the level nesting. - let mut chars = sql.chars(); - if chars - .nth(position.saturating_sub(1)) - .is_some_and(|c| c == '(') - && chars.next().is_some_and(|c| c == ')') + if char_before(sql, position).is_some_and(|c| c == '(') + && char_at(sql, position).is_some_and(|c| c == ')') { return true; } @@ -276,8 +274,9 @@ fn cursor_between_parentheses(sql: &str, position: TextSize) -> bool { .unwrap_or_default(); let after = sql + .get(position..) + .unwrap_or_default() .chars() - .skip(position) .find(|c| !c.is_whitespace()) .unwrap_or_default(); @@ -678,11 +677,55 @@ mod tests { assert_eq!( sanitized.text, - r#"select * from "auth"."REPLACED_TOKEN_WITH_QUOTE"é; "# + r#"select * from "auth"."REPLACED_TOKEN_WITH_QUOTE"é;"# ); assert_eq!(sanitized.position, TextSize::new(22)); // leaves cursor position as is } + { + // multi-byte chars before the cursor: the position is a byte offset + // select 'щщ', u.id from users u where u.| + let input = "select 'щщ', u.id from users u where u."; + let position = TextSize::new(input.len() as u32); + + assert!(cursor_on_a_dot(input, position)); + + let params = get_test_params(input, position); + + let sanitized = SanitizedCompletionParams::from(params); + + assert_eq!( + sanitized.text, + "select 'щщ', u.id from users u where u.REPLACED_TOKEN" + ); + } + + { + // -- заказы + // select | from users; + let input = "-- заказы\nselect from users;"; + let position = TextSize::new("-- заказы\nselect ".len() as u32); + + assert!(cursor_inbetween_nodes(input, position)); + + let params = get_test_params(input, position); + + let sanitized = SanitizedCompletionParams::from(params); + + assert_eq!( + sanitized.text, + "-- заказы\nselect REPLACED_TOKEN from users;" + ); + } + + { + // select 'щ' in (|) + let input = "select 'щ' in ()"; + let position = TextSize::new("select 'щ' in (".len() as u32); + + assert!(cursor_between_parentheses(input, position)); + } + { let input = "insert into instruments (name, id, €, )"; let position = TextSize::new(36); From 0aff6ed5815d04a6f2676f3afd90daa5efa30332 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sat, 31 Oct 2026 09:12:11 +0000 Subject: [PATCH 5/5] fix(completions): qualify columns with an alias named public The alias is now normalised, so a "public" or PUBLIC alias became public, which with_schema_or_alias left out like the public schema. Only the public schema is left out now; an alias always qualifies its columns. --- .../pgls_completions/src/providers/columns.rs | 4 +- .../src/providers/functions.rs | 4 +- .../pgls_completions/src/providers/helper.rs | 38 ++++++++++++++++--- .../pgls_completions/src/providers/tables.rs | 4 +- 4 files changed, 38 insertions(+), 12 deletions(-) diff --git a/crates/pgls_completions/src/providers/columns.rs b/crates/pgls_completions/src/providers/columns.rs index dd82094a9..d17a3ea58 100644 --- a/crates/pgls_completions/src/providers/columns.rs +++ b/crates/pgls_completions/src/providers/columns.rs @@ -8,7 +8,7 @@ use crate::{ relevance::{CompletionRelevanceData, filtering::CompletionFilter, scoring::CompletionScore}, }; -use super::helper::with_schema_or_alias; +use super::helper::{Qualifier, with_schema_or_alias}; pub fn complete_columns<'a>( ctx: &TreesitterContext<'a>, @@ -43,7 +43,7 @@ fn get_completion_text(ctx: &TreesitterContext, col: &Column) -> CompletionText let with_schema_or_alias = with_schema_or_alias( ctx, col.name.as_str(), - alias.as_deref(), + alias.as_deref().map(Qualifier::Alias), NamePosition::ColId, ); diff --git a/crates/pgls_completions/src/providers/functions.rs b/crates/pgls_completions/src/providers/functions.rs index 816b75bc0..0a35a5a54 100644 --- a/crates/pgls_completions/src/providers/functions.rs +++ b/crates/pgls_completions/src/providers/functions.rs @@ -8,7 +8,7 @@ use crate::{ relevance::{CompletionRelevanceData, filtering::CompletionFilter, scoring::CompletionScore}, }; -use super::helper::with_schema_or_alias; +use super::helper::{Qualifier, with_schema_or_alias}; pub fn complete_functions<'a>( ctx: &'a TreesitterContext, @@ -38,7 +38,7 @@ fn get_completion_text(ctx: &TreesitterContext, func: &Function) -> CompletionTe let mut text = with_schema_or_alias( ctx, func.name.as_str(), - Some(func.schema.as_str()), + Some(Qualifier::Schema(func.schema.as_str())), NamePosition::TypeFunctionName, ); diff --git a/crates/pgls_completions/src/providers/helper.rs b/crates/pgls_completions/src/providers/helper.rs index c8ecadfff..5684855cd 100644 --- a/crates/pgls_completions/src/providers/helper.rs +++ b/crates/pgls_completions/src/providers/helper.rs @@ -37,25 +37,38 @@ pub(crate) fn only_leading_quote(ctx: &TreesitterContext) -> bool { is_sanitized_token_with_quote(node_under_cursor_txt) } -/// The text to insert for `item_name`, prefixed with the schema or alias name unless it is -/// `public` or the user already wrote a qualifier. Names are quoted where Postgres would read +/// The name that qualifies a completed item. +pub(crate) enum Qualifier<'a> { + /// The schema of a table or function, left out if it is `public`. + Schema(&'a str), + /// The alias of the table of a column, also if it is named `public`. + Alias(&'a str), +} + +/// The text to insert for `item_name`, prefixed with its qualifier unless that is the `public` +/// schema or the user already wrote a qualifier. Names are quoted where Postgres would read /// another name unquoted. `position` is where an unqualified `item_name` stands. pub(crate) fn with_schema_or_alias( ctx: &TreesitterContext, item_name: &str, - schema_or_alias_name: Option<&str>, + qualifier: Option, position: NamePosition, ) -> String { let is_already_prefixed_with_schema_name = ctx.has_any_qualifier(); + let prefix = match qualifier { + Some(Qualifier::Schema("public")) | None => None, + Some(Qualifier::Schema(name) | Qualifier::Alias(name)) => Some(name), + }; + let with_quotes = node_text_surrounded_by_quotes(ctx); let single_leading_quote = only_leading_quote(ctx); // Inside quotes the user already typed, a name only needs its `"` escaped. let escaped = |name: &str| name.replace('"', r#""""#); - match schema_or_alias_name { - Some(prefix) if prefix != "public" && !is_already_prefixed_with_schema_name => { + match prefix { + Some(prefix) if !is_already_prefixed_with_schema_name => { if single_leading_quote { format!(r#"{}"."{}""#, escaped(prefix), escaped(item_name)) } else if with_quotes { @@ -119,6 +132,7 @@ mod tests { create table "My Schema"."Other" (x int); create function "MyFunc"() returns int language sql as 'select 1'; create function "My Schema".helper() returns int language sql as 'select 1'; + create function "operator"() returns int language sql as 'select 1'; "#, ) .await @@ -139,9 +153,21 @@ mod tests { (r#"select {c} from "MyTable""#, "plain", "plain"), (r#"select t.{c} from "MyTable" t"#, "select", "select"), (r#"select t.{c} from "MyTable" t"#, "Id", r#""Id""#), - // Functions. + // An alias named `public` still qualifies its columns. + ( + r#"select {c} from "MyTable" "public""#, + "plain", + "public.plain", + ), + ( + r#"select {c} from "MyTable" PUBLIC"#, + "plain", + "public.plain", + ), + // Functions. `operator(` would start an operator. ("select {c}", "MyFunc", r#""MyFunc"()"#), ("select {c}", "helper", r#""My Schema".helper()"#), + ("select {c}", "operator", r#""operator"()"#), ] { assert_eq!( inserted(&pool, query, label).await.as_deref(), diff --git a/crates/pgls_completions/src/providers/tables.rs b/crates/pgls_completions/src/providers/tables.rs index 937cbed0f..bd59b929d 100644 --- a/crates/pgls_completions/src/providers/tables.rs +++ b/crates/pgls_completions/src/providers/tables.rs @@ -9,7 +9,7 @@ use crate::{ relevance::{CompletionRelevanceData, filtering::CompletionFilter, scoring::CompletionScore}, }; -use super::helper::with_schema_or_alias; +use super::helper::{Qualifier, with_schema_or_alias}; pub fn complete_tables<'a>( ctx: &'a TreesitterContext, @@ -45,7 +45,7 @@ fn get_completion_text(ctx: &TreesitterContext, table: &Table) -> CompletionText let text = with_schema_or_alias( ctx, table.name.as_str(), - Some(table.schema.as_str()), + Some(Qualifier::Schema(table.schema.as_str())), NamePosition::ColId, );