sql: parenthesize postfix-access receivers (. and []) by self-delimitation

The receiver helpers for the postfix `.field`/`.*` and `[…]` operators
whitelisted variants that are only ever produced by the parser wrapped in
`Expr::Nested`, so a `Nested`-stripped AST (an implicit rewrite, a fuzzed
round trip) printed them bare and the trailing token re-bound:

* `write_dot_receiver` treated a bare `Identifier`/`QualifiedWildcard`
  receiver as safe, but `a` followed by `.b`/`.*` prints `a.b`/`a.*`, which
  reparses as the qualified identifier `Identifier([a, b])` /
  `QualifiedWildcard([a])` instead of a field/wildcard access. The parser
  only builds those accesses over a parenthesized receiver, so it always
  wraps the name in `Nested`. Drop both from the safe set. `FieldAccess` /
  `WildcardAccess` receivers stay safe: their own printing parenthesizes a
  bare-name base (`(a).b.c`), so the chain remains self-delimiting and
  parser-shaped `(x).a.b` does not gain spurious parens.

* `write_subscript_receiver` only parenthesized keyword identifiers. A `Cast`
  receiver is unsafe because the type parser eats the following `[…]` as an
  array suffix (`(a::int4)[1]` -> `a::int4[1]` is `a` cast to `int4[]`), and a
  nested `Subscript` receiver is unsafe because consecutive `[…]` flatten into
  one node (`(a[1])[2]` -> `a[1][2]` is a single two-position subscript).
  Replace the keyword check with an explicit safe whitelist that
  parenthesizes `Cast`, nested `Subscript`, and the operator/comparison
  constructs by default.

Both are churn-free: the parser never emits these receivers bare (it eats the
`[…]` into the cast type, flattens subscripts, and wraps dotted names in
`Nested`), so the parser and pretty-printer datadriven suites are unchanged.
Found by extending the round-trip grammar fuzzer to cover subscripts, field
access, `COLLATE`, `AT TIME ZONE`, and the `position`/`extract`/`substring`
special forms.

Tests: adds `postfix_access_receiver_reparenthesized_after_nested_stripped`
to `sqlparser_common.rs`.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: def-

src/sql-parser/src/ast/defs/expr.rs

@@ -561,14 +561,21 @@ impl_display_t!(Expr);
 /// the lexer and parser greedily extend adjacent tokens: `1.x` tokenizes the
 /// number `1.` and leaves `x` as an alias, and `'a'::T.x` consumes `T.x` as a
 /// qualified type name. The whitelist below covers receivers that print as
-/// self-terminating syntax (identifiers, parenthesized exprs, function calls,
-/// bracketed collections, etc.); anything else gets explicit parens.
+/// self-terminating syntax (parenthesized exprs, function calls, bracketed
+/// collections, etc.); anything else gets explicit parens.
+///
+/// A bare `Identifier`/`QualifiedWildcard` receiver is *not* safe: `a` then
+/// `.b`/`.*` prints as `a.b`/`a.*`, which reparses as the qualified identifier
+/// `Identifier([a, b])` / `QualifiedWildcard([a])` rather than a field/wildcard
+/// access. The parser only ever builds those accesses over a parenthesized
+/// receiver (`(a).b`), so it wraps the name in `Expr::Nested`; a bare name here
+/// is a `Nested`-stripped AST and must be re-parenthesized. A `FieldAccess` /
+/// `WildcardAccess` receiver *is* safe, because its own printing already
+/// parenthesizes a bare-name base (`(a).b.c`), so the chain stays self-delimiting.
 fn write_dot_receiver<W: fmt::Write, T: AstInfo>(f: &mut AstFormatter<W>, expr: &Expr<T>) {
     let safe = matches!(
         expr,
-        Expr::Identifier(_)
-            | Expr::QualifiedWildcard(_)
-            | Expr::FieldAccess { .. }
+        Expr::FieldAccess { .. }
             | Expr::WildcardAccess(_)
             | Expr::Parameter(_)
             | Expr::Nested(_)
@@ -879,14 +886,45 @@ fn prefix_operand_needs_parens<T: AstInfo>(operand: &Expr<T>) -> bool {
 /// regular subscript. Parenthesize identifiers whose last component is a
 /// context-sensitive keyword so the round trip stays an identifier subscript.
 fn write_subscript_receiver<W: fmt::Write, T: AstInfo>(f: &mut AstFormatter<W>, expr: &Expr<T>) {
-    let needs_parens = if let Expr::Identifier(idents) = expr {
-        idents
+    let needs_parens = match expr {
+        // A bare keyword identifier (`map`, `list`, …) dispatches to the
+        // map/list-literal grammar before `[`, so it needs parens even though
+        // identifiers are otherwise safe receivers.
+        Expr::Identifier(idents) => idents
             .last()
             .and_then(|id| id.as_keyword())
             .map(|kw| kw.is_context_sensitive_keyword())
-            .unwrap_or(false)
-    } else {
-        false
+            .unwrap_or(false),
+        // Self-delimiting primaries, the bracketed collections, and the postfix
+        // forms that end in an identifier or `)` are safe: a following `[…]`
+        // attaches to the whole receiver as a fresh subscript.
+        Expr::QualifiedWildcard(_)
+        | Expr::Parameter(_)
+        | Expr::Value(_)
+        | Expr::Function(_)
+        | Expr::HomogenizingFunction { .. }
+        | Expr::NullIf { .. }
+        | Expr::Nested(_)
+        | Expr::Subquery(_)
+        | Expr::Exists(_)
+        | Expr::Case { .. }
+        | Expr::Row { .. }
+        | Expr::Array(_)
+        | Expr::ArraySubquery(_)
+        | Expr::List(_)
+        | Expr::ListSubquery(_)
+        | Expr::Map(_)
+        | Expr::MapSubquery(_)
+        | Expr::FieldAccess { .. }
+        | Expr::WildcardAccess(_)
+        | Expr::Collate { .. } => false,
+        // `Cast`: the type parser swallows a following `[…]` as an array suffix
+        // (`a::int4[1]` is `a` cast to `int4[]`, not a subscript of `a::int4`).
+        // `Subscript`: consecutive `[…]` flatten into one node (`a[1][2]` is a
+        // single subscript), so a nested subscript receiver must be parenthesized
+        // to stay nested. Everything else (operators, `IS`/`LIKE`/… constructs)
+        // binds looser than `[` and would re-associate, so parenthesize by default.
+        _ => true,
     };
     if needs_parens {
         f.write_str("(");

src/sql-parser/tests/sqlparser_common.rs

@@ -990,3 +990,64 @@ fn like_pattern_reparenthesized_after_nested_stripped() {
         );
     }
 }
+
+#[mz_ore::test]
+fn postfix_access_receiver_reparenthesized_after_nested_stripped() {
+    use mz_sql_parser::ast::display::AstDisplay;
+    use mz_sql_parser::ast::visit_mut::{self, VisitMut};
+    use mz_sql_parser::ast::{AstInfo, Expr};
+
+    struct StripNested;
+    impl<'a, T: AstInfo> VisitMut<'a, T> for StripNested {
+        fn visit_expr_mut(&mut self, e: &'a mut Expr<T>) {
+            visit_mut::visit_expr_mut(self, e);
+            if let Expr::Nested(inner) = e {
+                *e = (**inner).clone();
+            }
+        }
+    }
+
+    // The receivers of the postfix `.field`/`.*` and `[…]` operators must stay
+    // self-delimiting once `Expr::Nested` is stripped, or the trailing token
+    // re-binds. A bare-name `FieldAccess`/`WildcardAccess` receiver prints as a
+    // dotted name and collides with a qualified identifier (`(a).b` -> `a.b` is
+    // `Identifier([a, b])`); a `Cast` subscript receiver lets the type parser eat
+    // the `[…]` as an array suffix (`(a::int4)[1]` -> `a::int4[1]` is `a::int4[]`);
+    // and a nested `Subscript` receiver flattens (`(a[1])[2]` -> `a[1][2]` is one
+    // two-position subscript).
+    for sql in [
+        // dot receiver
+        "SELECT (a).b",
+        "SELECT (a).*",
+        "SELECT ((a).b).c",
+        "SELECT ((a).*).*",
+        "SELECT (a).b[1]",
+        // subscript receiver
+        "SELECT (a::int4)[1]",
+        "SELECT (CAST(a AS int4))[1]",
+        "SELECT (a[1])[2]",
+        "SELECT (a + b)[1]",
+        // these are parser-shaped and must remain stable (no spurious parens)
+        "SELECT (x).a.b",
+        "SELECT a.b[1]",
+        "SELECT a[1][2]",
+        "SELECT f(x)[1]",
+        "SELECT (a + b).c",
+    ] {
+        let mut ast = mz_sql_parser::parser::parse_statements(sql)
+            .unwrap()
+            .remove(0)
+            .ast;
+        StripNested.visit_statement_mut(&mut ast);
+        let displayed = ast.to_ast_string_simple();
+        let mut reparsed = mz_sql_parser::parser::parse_statements(&displayed)
+            .unwrap()
+            .remove(0)
+            .ast;
+        StripNested.visit_statement_mut(&mut reparsed);
+        assert_eq!(
+            ast, reparsed,
+            "postfix-access receiver display did not round-trip after Nested was stripped: {sql:?} -> {displayed:?}"
+        );
+    }
+}