diff --git a/src/formatter/expr.rs b/src/formatter/expr.rs index 0c9a142..afa556d 100644 --- a/src/formatter/expr.rs +++ b/src/formatter/expr.rs @@ -413,6 +413,25 @@ impl<'a> Formatter<'a> { last.push_str(&formatted); continue; } + // Field selection (e.g. `.bar`) attaches to the preceding + // expression with no space. When that base was parenthesized + // (its parens stripped above), restore them so `(foo).bar` + // keeps its composite-field meaning rather than becoming the + // column reference `foo.bar`. + if formatted.starts_with('.') + && let Some(last) = target.last_mut() + { + // Re-wrap unless the base is already fully enclosed in + // outer parens. `contains('(')` would be too broad: a bare + // function call like `foo(x)` contains `(` yet still needs + // wrapping so `(foo(x)).bar` keeps its field-selection + // meaning instead of collapsing to `foo(x).bar`. + if !(last.starts_with('(') && last.ends_with(')')) { + *last = format!("({last})"); + } + last.push_str(&formatted); + continue; + } target.push(formatted); } else { let text = self.text(child).trim(); @@ -726,11 +745,12 @@ impl<'a> Formatter<'a> { && let Some(typename) = subexpr.find_child("Typename") { let formatted = self.format_expr(expr); - // Parenthesized expressions, function calls like fn(...), and - // expressions without spaces (simple identifiers/literals) are safe. - // Anything else (e.g. "a + b", "x IS NOT NULL") needs wrapping. - let needs_parens = - formatted.contains(' ') && !formatted.starts_with('(') && !formatted.contains('('); + // A compound operand carries a top-level operator (a space outside + // any parens/quotes), e.g. "a + b", "foo(x) + 1", "x IS NOT NULL". + // Because :: binds tighter than those operators, such operands must + // be wrapped. Bare identifiers, literals, and single function calls + // like foo(x) have no top-level space and are left alone. + let needs_parens = has_top_level_space(&formatted); return if needs_parens { format!("({formatted})::{}", self.format_typename(typename)) } else { @@ -1334,8 +1354,10 @@ impl<'a> Formatter<'a> { } // Join without extra spaces (dots already included). let result = parts.join(""); - // Clean up any double dots. - result.replace("..", ".") + // Clean up any double dots produced by joining a separator with an + // attr's own leading dot, but never touch dots inside quoted + // identifiers (e.g. "a..b" is a single, distinct object name). + collapse_double_dots_outside_quotes(&result) } pub(crate) fn format_relation_expr(&self, node: Node<'a>) -> String { @@ -1403,11 +1425,123 @@ impl<'a> Formatter<'a> { } } +/// Collapse consecutive dots (`..` → `.`) that arise from joining a qualified +/// name's separator with an attr's own leading dot, while leaving dots inside +/// double-quoted identifiers untouched. +fn collapse_double_dots_outside_quotes(s: &str) -> String { + let mut result = String::with_capacity(s.len()); + let mut in_quote = false; + let mut prev_dot = false; + let chars: Vec = s.chars().collect(); + let len = chars.len(); + let mut i = 0; + while i < len { + let ch = chars[i]; + if in_quote { + result.push(ch); + if ch == '"' { + if i + 1 < len && chars[i + 1] == '"' { + result.push('"'); + i += 2; + continue; + } + in_quote = false; + } + i += 1; + continue; + } + if ch == '"' { + in_quote = true; + prev_dot = false; + result.push(ch); + } else if ch == '.' { + if !prev_dot { + result.push('.'); + } + prev_dot = true; + } else { + prev_dot = false; + result.push(ch); + } + i += 1; + } + result +} + +/// Returns true when `s` contains whitespace at the top level (outside any +/// parentheses/brackets and outside quoted strings), indicating a compound +/// operand such as `a + b` or `foo(x) + 1` rather than a bare identifier, +/// literal, or single function call. +fn has_top_level_space(s: &str) -> bool { + let mut depth: i32 = 0; + let mut in_single = false; + // True when the current single-quoted string is a PostgreSQL escape string + // (E'...'), which uses backslash escaping rather than only '' doubling. + let mut escape_string = false; + let mut in_double = false; + let chars: Vec = s.chars().collect(); + let len = chars.len(); + let mut i = 0; + while i < len { + let ch = chars[i]; + if in_single { + // In an E'...' string a backslash escapes the next character, so a + // \' is not a terminator. + if escape_string && ch == '\\' && i + 1 < len { + i += 2; + continue; + } + if ch == '\'' { + // A doubled '' is an escaped quote, not a terminator. + if i + 1 < len && chars[i + 1] == '\'' { + i += 2; + continue; + } + in_single = false; + escape_string = false; + } + i += 1; + continue; + } + if in_double { + if ch == '"' { + if i + 1 < len && chars[i + 1] == '"' { + i += 2; + continue; + } + in_double = false; + } + i += 1; + continue; + } + match ch { + '\'' => { + in_single = true; + // An E/e immediately preceding the quote (and not part of a + // longer identifier) marks a backslash-escaped string literal. + escape_string = i >= 1 + && matches!(chars[i - 1], 'E' | 'e') + && (i < 2 || !(chars[i - 2].is_ascii_alphanumeric() || chars[i - 2] == '_')); + } + '"' => in_double = true, + '(' | '[' => depth += 1, + ')' | ']' => depth -= 1, + c if c.is_whitespace() && depth == 0 => return true, + _ => {} + } + i += 1; + } + false +} + /// Collapse runs of whitespace to single spaces, but preserve whitespace /// inside single-quoted or double-quoted strings. fn collapse_whitespace_outside_quotes(s: &str) -> String { let mut result = String::with_capacity(s.len()); let mut in_single_quote = false; + // True when the current single-quoted string is a PostgreSQL escape string + // (E'...'), which uses backslash escaping rather than only '' doubling. + let mut escape_string = false; let mut in_double_quote = false; let mut prev_was_space = false; @@ -1420,6 +1554,13 @@ fn collapse_whitespace_outside_quotes(s: &str) -> String { if in_single_quote { result.push(ch); + // In an E'...' string a backslash escapes the next character, so a + // \' is not a terminator and its following bytes stay untouched. + if escape_string && ch == '\\' && i + 1 < len { + result.push(chars[i + 1]); + i += 2; + continue; + } if ch == '\'' { // Check for escaped quote (''). if i + 1 < len && chars[i + 1] == '\'' { @@ -1428,6 +1569,7 @@ fn collapse_whitespace_outside_quotes(s: &str) -> String { continue; } in_single_quote = false; + escape_string = false; } i += 1; continue; @@ -1449,6 +1591,11 @@ fn collapse_whitespace_outside_quotes(s: &str) -> String { if ch == '\'' { in_single_quote = true; + // An E/e immediately preceding the quote (and not part of a longer + // identifier) marks a backslash-escaped string literal. + escape_string = i >= 1 + && matches!(chars[i - 1], 'E' | 'e') + && (i < 2 || !(chars[i - 2].is_ascii_alphanumeric() || chars[i - 2] == '_')); prev_was_space = false; result.push(ch); } else if ch == '"' { diff --git a/tests/fixtures/river/expr_cast_compound_func_parens.expected b/tests/fixtures/river/expr_cast_compound_func_parens.expected new file mode 100644 index 0000000..67b7657 --- /dev/null +++ b/tests/fixtures/river/expr_cast_compound_func_parens.expected @@ -0,0 +1 @@ +SELECT (foo(x) + 1)::INTEGER; diff --git a/tests/fixtures/river/expr_cast_compound_func_parens.sql b/tests/fixtures/river/expr_cast_compound_func_parens.sql new file mode 100644 index 0000000..bfd3072 --- /dev/null +++ b/tests/fixtures/river/expr_cast_compound_func_parens.sql @@ -0,0 +1 @@ +SELECT CAST(foo(x) + 1 AS integer); diff --git a/tests/fixtures/river/expr_escape_string_whitespace.expected b/tests/fixtures/river/expr_escape_string_whitespace.expected new file mode 100644 index 0000000..dafb605 --- /dev/null +++ b/tests/fixtures/river/expr_escape_string_whitespace.expected @@ -0,0 +1 @@ +SELECT E'it\'s here' AS x; diff --git a/tests/fixtures/river/expr_escape_string_whitespace.sql b/tests/fixtures/river/expr_escape_string_whitespace.sql new file mode 100644 index 0000000..dafb605 --- /dev/null +++ b/tests/fixtures/river/expr_escape_string_whitespace.sql @@ -0,0 +1 @@ +SELECT E'it\'s here' AS x; diff --git a/tests/fixtures/river/expr_paren_field_selection.expected b/tests/fixtures/river/expr_paren_field_selection.expected new file mode 100644 index 0000000..ffc7274 --- /dev/null +++ b/tests/fixtures/river/expr_paren_field_selection.expected @@ -0,0 +1,2 @@ +SELECT (foo).bar + FROM t; diff --git a/tests/fixtures/river/expr_paren_field_selection.sql b/tests/fixtures/river/expr_paren_field_selection.sql new file mode 100644 index 0000000..96c72a8 --- /dev/null +++ b/tests/fixtures/river/expr_paren_field_selection.sql @@ -0,0 +1 @@ +SELECT (foo).bar FROM t; diff --git a/tests/fixtures/river/expr_quoted_identifier_double_dots.expected b/tests/fixtures/river/expr_quoted_identifier_double_dots.expected new file mode 100644 index 0000000..ca20e24 --- /dev/null +++ b/tests/fixtures/river/expr_quoted_identifier_double_dots.expected @@ -0,0 +1,2 @@ +SELECT * + FROM "a..b"; diff --git a/tests/fixtures/river/expr_quoted_identifier_double_dots.sql b/tests/fixtures/river/expr_quoted_identifier_double_dots.sql new file mode 100644 index 0000000..df3df33 --- /dev/null +++ b/tests/fixtures/river/expr_quoted_identifier_double_dots.sql @@ -0,0 +1 @@ +SELECT * FROM "a..b"; diff --git a/tests/test_parens.rs b/tests/test_parens.rs index 0292a84..7fb88bf 100644 --- a/tests/test_parens.rs +++ b/tests/test_parens.rs @@ -27,6 +27,56 @@ fn preserve_adjacent_parens() { ); } +// https://github.com/gmr/pgfmt/issues/24 — rewriting CAST(x AS t) to x::t must +// keep parens around a compound operand, because :: binds tighter than +// arithmetic. Dropping them changes what gets cast. +#[test] +fn cast_wraps_compound_operand_with_function_call() { + let sql = "SELECT CAST(foo(x) + 1 AS integer);"; + let result = format(sql, Style::River).unwrap(); + assert!( + result.contains("(foo(x) + 1)::INTEGER"), + "CAST dropped required parens:\n{result}" + ); +} + +#[test] +fn cast_leaves_bare_function_call_unparenthesized() { + let sql = "SELECT CAST(foo(x) AS integer);"; + let result = format(sql, Style::River).unwrap(); + assert!( + result.contains("foo(x)::INTEGER") && !result.contains("(foo(x))::INTEGER"), + "CAST over-parenthesized a simple operand:\n{result}" + ); +} + +// A field selection on a function-call result needs the call parenthesized: +// `(foo(x)).bar` selects field `bar` from the composite row, whereas +// `foo(x).bar` is invalid. The base already contains `(`, so the re-wrap must +// key off outer enclosure, not the mere presence of a paren. +#[test] +fn field_selection_wraps_function_call_result() { + let sql = "SELECT (foo(x)).bar;"; + let result = format(sql, Style::River).unwrap(); + assert!( + result.contains("(foo(x)).bar"), + "Function-call field selection lost its parens:\n{result}" + ); +} + +// An E'...' escape string carries backslash escapes; a \' inside it is not a +// terminator. The CAST-to-:: rewrite must not misread the trailing bytes as a +// top-level compound operand and wrap the literal in needless parens. +#[test] +fn cast_leaves_escape_string_unparenthesized() { + let sql = "SELECT CAST(E'a\\'s value' AS text);"; + let result = format(sql, Style::River).unwrap(); + assert!( + !result.contains("(E'"), + "CAST over-parenthesized an escape-string operand:\n{result}" + ); +} + // Formats `sql` in the Aweber style and asserts that (1) `expected_substr` // survives, (2) reformatting the output is idempotent, and (3) no line carries // trailing whitespace. Shared by the column-level CHECK regression tests.