Preserve user-supplied grouping parentheses in boolean expressions

[gmr]

Summary

• Parenthesized boolean sub-expressions like (a IS NULL OR b > 1) AND c = 'x' now preserve the parentheses in formatted output instead of dropping them
• The tree-sitter-postgres grammar produces ERROR nodes for these constructs; the error tolerance now recognizes ERROR nodes inside c_expr parents as valid parenthesized expressions
• format_c_expr detects ( ... ) grouping and joins inner content without extra spaces

Fixes #2

Test plan

• New test: preserve_parens_around_or_in_and verifies parens are kept
• New test: no_unnecessary_parens verifies no spurious parens added
• All 72 existing tests pass
• clippy clean, fmt clean

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Preserve parenthesized groups and spacing in formatted SQL expressions, including nested, adjacent, and partially unclosed parentheses.
  • Refine parse-error detection to avoid false positives when a valid top-level statement exists.

• Tests

  • Added tests validating parenthesis preservation and a comprehensive fixture runner exercising many formatting cases.

[coderabbitai]

📝 Walkthrough

Walkthrough

Buffer and emit user-supplied literal parentheses in expression formatting; change structural-error detection to require a top-level statement at the parse root; add targeted parenthesis tests and a fixture-driven test runner.

Changes

Cohort / File(s)	Summary
Expression formatter src/formatter/expr.rs	format_c_expr now tracks literal (/) with paren_depth and a paren_parts buffer, routing child outputs into the buffer while inside groups and emitting buffered content as a single ({inner}) part when the group closes (or flushing if unclosed). Named-child formatting refactored to compute one formatted string before routing.
Structural-error detection src/lib.rs	Replaced recursive error-node heuristics with a root-level check that treats the parse root as structurally valid if any toplevel_stmt exists among its named_children; format() still returns syntax error only when root.has_error() && has_structural_error(&root).
Parenthesis unit tests tests/test_parens.rs	Added three integration tests asserting preservation of user-supplied grouping parentheses and ensuring no unnecessary parentheses are introduced for simple conjunctive predicates.
Fixture-driven test runner tests/all_fixtures.rs	Added a suite that scans tests/fixtures/<style>/*.sql, compares formatted output to .expected, catches panics per fixture, supports a KNOWN_FAILING allowlist, and reports an aggregated summary with unexpected failures.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I tuck each paren, snug and round,
Into my buffer, safe and sound.
When closes click, I stitch them whole,
Keep your grouping true — that's my goal.
A carrot hop for faithful code.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly matches the main objective: preserving user-supplied grouping parentheses in boolean expressions during SQL formatting.
Linked Issues check	✅ Passed	Changes fully address issue #2 requirements: parenthesis detection in format_c_expr [changes in src/formatter/expr.rs], error-tolerance updates [src/lib.rs], and comprehensive test coverage [tests/test_parens.rs, tests/all_fixtures.rs].
Out of Scope Changes check	✅ Passed	All changes directly support parenthesis preservation and error handling improvements aligned with issue #2 objectives; no unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/preserve-grouping-parens

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gmr]

@CodeRabbit review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/formatter/expr.rs (1)

376-399: Parenthesis depth tracking looks correct.

The logic properly handles:

• Opening parens: increments depth, pushes nested ( to buffer
• Closing parens: decrements depth, joins and emits group when returning to depth 0, or pushes nested ) to buffer
• Other tokens: routes to appropriate buffer based on depth

One minor observation: select.rs uses saturating_add(1) and saturating_sub(1) for its paren_depth counter (see context snippet lines 831-931). Using plain +=/-= here is fine for practical purposes (overflow requires ~4 billion nested parens), but you may want to align the style for consistency across the codebase.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/formatter/expr.rs` around lines 376 - 399, The paren depth
increments/decrements in expr.rs currently use plain += 1 / -= 1; change these
to use saturating_add(1) and saturating_sub(1) on the paren_depth counter to
match the style in select.rs and avoid potential overflow edge-cases—update the
branches that handle "(" (where paren_depth is increased) and ")" (where
paren_depth is decreased) while keeping the same logic that pushes to
paren_parts and emits to parts.

tests/all_fixtures.rs (1)

98-103: Consider failing when known-failing fixtures unexpectedly pass.

Currently, when a KNOWN_FAILING fixture passes, a warning is printed but the test still succeeds. This could allow the list to become stale. Making it a failure would enforce that KNOWN_FAILING is kept up to date.

♻️ Optional: fail on unexpected passes

                 Ok(()) => {
-                    passed += 1;
                     if is_known_failing {
-                        eprintln!("UNEXPECTED PASS {fixture_key}: remove from KNOWN_FAILING");
+                        failures.push(format!(
+                            "{fixture_key}: UNEXPECTED PASS - remove from KNOWN_FAILING"
+                        ));
+                    } else {
+                        passed += 1;
                     }
                 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/all_fixtures.rs` around lines 98 - 103, The current Ok(()) branch
treats known-failing fixtures as passes; change it so unexpected passes count as
failures: move passed += 1 into the non-known-failing branch and, when
is_known_failing is true, print the "UNEXPECTED PASS {fixture_key}: remove from
KNOWN_FAILING" message and increment failed (or otherwise mark overall test as
failing) instead of incrementing passed; update the Ok(()) arm around
fixture_key, is_known_failing, passed, and failed to reflect this behavior so
the run exits non-zero when a KNOWN_FAILING unexpectedly succeeds.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/all_fixtures.rs`:
- Around line 97-124: The truncation of panic message bytes using &msg[..200]
can panic on UTF-8 boundaries; update the logic that builds short (in the match
-> Err branch where msg is created and failures.push is called) to perform a
char-safe truncation (e.g., take at most 200 characters rather than bytes) and
append "..." when truncated, then push the formatted "{fixture_key}: {short}" as
before; ensure you operate on msg (the String) and produce short without slicing
by byte indices.

---

Nitpick comments:
In `@src/formatter/expr.rs`:
- Around line 376-399: The paren depth increments/decrements in expr.rs
currently use plain += 1 / -= 1; change these to use saturating_add(1) and
saturating_sub(1) on the paren_depth counter to match the style in select.rs and
avoid potential overflow edge-cases—update the branches that handle "(" (where
paren_depth is increased) and ")" (where paren_depth is decreased) while keeping
the same logic that pushes to paren_parts and emits to parts.

In `@tests/all_fixtures.rs`:
- Around line 98-103: The current Ok(()) branch treats known-failing fixtures as
passes; change it so unexpected passes count as failures: move passed += 1 into
the non-known-failing branch and, when is_known_failing is true, print the
"UNEXPECTED PASS {fixture_key}: remove from KNOWN_FAILING" message and increment
failed (or otherwise mark overall test as failing) instead of incrementing
passed; update the Ok(()) arm around fixture_key, is_known_failing, passed, and
failed to reflect this behavior so the run exits non-zero when a KNOWN_FAILING
unexpectedly succeeds.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ed138e91-ca0c-4369-972d-dcd27c2ef2c4

📥 Commits

Reviewing files that changed from the base of the PR and between 048210b and bcb3aa4.

📒 Files selected for processing (4)

• src/formatter/expr.rs
• src/lib.rs
• tests/all_fixtures.rs
• tests/test_parens.rs

✅ Files skipped from review due to trivial changes (1)

• tests/test_parens.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/lib.rs

[coderabbitai]

🧹 Nitpick comments (1)

tests/all_fixtures.rs (1)

53-61: Consider deriving style list from Style::ALL to reduce drift.

The styles are hardcoded here while Style::ALL exists in src/style.rs. If a new style variant is added to the enum but not updated here, its fixtures would silently be skipped.

One option is to derive the directory name from the enum variant (e.g., via Debug formatting or a method on Style), iterating Style::ALL to ensure completeness.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/all_fixtures.rs` around lines 53 - 61, Replace the hardcoded styles
array with an iteration over Style::ALL so new variants cannot be missed; for
each variant from Style::ALL produce the tuple previously in `styles` by
deriving the directory/name string from the enum (use Debug formatting or add a
small method like `Style::dir_name(&self)` on the `Style` enum) and collect into
the same &[(&str, Style)] shape used in tests; update the code that defines
`styles` to build it from `Style::ALL` (or map to a Vec and borrow) and keep the
test logic unchanged so every enum variant is covered automatically.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/all_fixtures.rs`:
- Around line 53-61: Replace the hardcoded styles array with an iteration over
Style::ALL so new variants cannot be missed; for each variant from Style::ALL
produce the tuple previously in `styles` by deriving the directory/name string
from the enum (use Debug formatting or add a small method like
`Style::dir_name(&self)` on the `Style` enum) and collect into the same &[(&str,
Style)] shape used in tests; update the code that defines `styles` to build it
from `Style::ALL` (or map to a Vec and borrow) and keep the test logic unchanged
so every enum variant is covered automatically.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 97023f2a-dde5-4ff0-a55f-a6d53c6b980b

📥 Commits

Reviewing files that changed from the base of the PR and between bcb3aa4 and 9165f87.

📒 Files selected for processing (4)

• src/formatter/expr.rs
• src/lib.rs
• tests/all_fixtures.rs
• tests/test_parens.rs

✅ Files skipped from review due to trivial changes (1)

• tests/test_parens.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• src/lib.rs