From 50a6998698ab631d21bf21f9bf5c85d55a818b96 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Mon, 6 Jul 2026 12:01:19 -0400 Subject: [PATCH 1/2] Fix silent statement/data loss in root render/parse path - #15: format_root only emitted a statement when its toplevel_stmt had a `stmt` child, so `BEGIN` (parsed as TransactionStmtLegacy directly under toplevel_stmt) was silently dropped. Fall back to the toplevel_stmt node when there is no `stmt` child so the inner statement still routes through the statement formatter. Adds river fixtures begin and begin_insert_commit. - #16: has_structural_error returned false as soon as any toplevel_stmt existed, so a broken sibling statement never triggered FormatError::Syntax and was silently discarded. Now, when at least one statement parsed, scan for MISSING nodes or ERROR nodes >= 5 bytes (preserving the documented tolerance for small benign ERROR leaves). Adds a smoke test asserting Err(FormatError::Syntax(..)) for `SELECT 1; THIS IS NOT SQL @@@;`. - #17: partially addressed. Standalone comments between/around top-level statements (direct children of source_file) are now preserved verbatim in source order instead of being dropped. Comments embedded within a statement's clauses are still dropped (out of scope for a surgical change). Adds river fixture comment_between_statements. Co-Authored-By: Claude Fable 5 --- src/formatter/mod.rs | 16 ++++++++++--- src/lib.rs | 24 +++++++++++++++---- tests/fixtures/river/begin.expected | 1 + tests/fixtures/river/begin.sql | 1 + .../river/begin_insert_commit.expected | 6 +++++ tests/fixtures/river/begin_insert_commit.sql | 1 + .../river/comment_between_statements.expected | 5 ++++ .../river/comment_between_statements.sql | 3 +++ tests/smoke_test.rs | 13 ++++++++++ 9 files changed, 63 insertions(+), 7 deletions(-) create mode 100644 tests/fixtures/river/begin.expected create mode 100644 tests/fixtures/river/begin.sql create mode 100644 tests/fixtures/river/begin_insert_commit.expected create mode 100644 tests/fixtures/river/begin_insert_commit.sql create mode 100644 tests/fixtures/river/comment_between_statements.expected create mode 100644 tests/fixtures/river/comment_between_statements.sql diff --git a/src/formatter/mod.rs b/src/formatter/mod.rs index 64fed78..fac9fcf 100644 --- a/src/formatter/mod.rs +++ b/src/formatter/mod.rs @@ -180,14 +180,24 @@ impl<'a> Formatter<'a> { let mut results = Vec::new(); let mut cursor = root.walk(); for child in root.named_children(&mut cursor) { - if child.kind() == "toplevel_stmt" - && let Some(stmt) = child.find_child("stmt") - { + if child.kind() == "toplevel_stmt" { + // Most statements are wrapped in a `stmt` node, but some parse + // as a direct child of `toplevel_stmt` (e.g. the legacy + // transaction statement `BEGIN`, which is a + // `TransactionStmtLegacy`). Format the inner statement in + // either case so it is not silently dropped. + let stmt = child.find_child("stmt").unwrap_or(child); if self.style == Style::PgDump { results.push(self.format_pgdump_stmt(stmt)?); } else { results.push(self.format_stmt(stmt)?); } + } else if child.kind() == "comment" { + // Standalone comments between/around top-level statements are + // direct children of `source_file`; preserve them verbatim in + // source order so they are not silently discarded. (Comments + // embedded within a statement's clauses are not yet preserved.) + results.push(self.text(child).trim_end().to_string()); } } if results.is_empty() { diff --git a/src/lib.rs b/src/lib.rs index 751ab8f..a2b1737 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -179,11 +179,27 @@ fn has_structural_error(root: &tree_sitter::Node) -> bool { let has_valid_stmt = root .named_children(&mut cursor) .any(|c| c.kind() == "toplevel_stmt"); - if has_valid_stmt { - return false; + if !has_valid_stmt { + // No valid statements at all — this is genuinely broken input. + return true; } - // No valid statements at all — this is genuinely broken input. - true + // At least one statement parsed, but a sibling statement may still be + // broken (tree-sitter emits an ERROR/MISSING node rather than a + // toplevel_stmt for it). Small ERROR leaf nodes (< 5 bytes, e.g. decimal + // fractions) are grammar limitations for otherwise-valid SQL and are + // tolerated; a MISSING node or a substantial ERROR node signals a + // genuinely unparseable statement that must not be silently dropped. + has_significant_error(root) +} + +/// Recursively check for a MISSING node or a non-trivial ERROR node. +fn has_significant_error(node: &tree_sitter::Node) -> bool { + if node.is_missing() || (node.is_error() && node.byte_range().len() >= 5) { + return true; + } + let mut cursor = node.walk(); + node.children(&mut cursor) + .any(|child| has_significant_error(&child)) } fn find_error_message(node: &tree_sitter::Node, source: &str) -> String { diff --git a/tests/fixtures/river/begin.expected b/tests/fixtures/river/begin.expected new file mode 100644 index 0000000..1775571 --- /dev/null +++ b/tests/fixtures/river/begin.expected @@ -0,0 +1 @@ +BEGIN; diff --git a/tests/fixtures/river/begin.sql b/tests/fixtures/river/begin.sql new file mode 100644 index 0000000..1775571 --- /dev/null +++ b/tests/fixtures/river/begin.sql @@ -0,0 +1 @@ +BEGIN; diff --git a/tests/fixtures/river/begin_insert_commit.expected b/tests/fixtures/river/begin_insert_commit.expected new file mode 100644 index 0000000..94b2073 --- /dev/null +++ b/tests/fixtures/river/begin_insert_commit.expected @@ -0,0 +1,6 @@ +BEGIN; + +INSERT INTO t + VALUES (1); + +COMMIT; diff --git a/tests/fixtures/river/begin_insert_commit.sql b/tests/fixtures/river/begin_insert_commit.sql new file mode 100644 index 0000000..6e50dba --- /dev/null +++ b/tests/fixtures/river/begin_insert_commit.sql @@ -0,0 +1 @@ +BEGIN; INSERT INTO t VALUES (1); COMMIT; diff --git a/tests/fixtures/river/comment_between_statements.expected b/tests/fixtures/river/comment_between_statements.expected new file mode 100644 index 0000000..31d6848 --- /dev/null +++ b/tests/fixtures/river/comment_between_statements.expected @@ -0,0 +1,5 @@ +SELECT 1; + +-- note + +SELECT 2; diff --git a/tests/fixtures/river/comment_between_statements.sql b/tests/fixtures/river/comment_between_statements.sql new file mode 100644 index 0000000..e777017 --- /dev/null +++ b/tests/fixtures/river/comment_between_statements.sql @@ -0,0 +1,3 @@ +SELECT 1; +-- note +SELECT 2; diff --git a/tests/smoke_test.rs b/tests/smoke_test.rs index 596db86..e334f3c 100644 --- a/tests/smoke_test.rs +++ b/tests/smoke_test.rs @@ -1,3 +1,4 @@ +use libpgfmt::error::FormatError; use libpgfmt::{format, style::Style}; #[test] @@ -110,6 +111,18 @@ fn typed_literal_constants_river() { } } +// Regression for https://github.com/gmr/libpgfmt/issues/16: a broken +// statement in multi-statement input must surface as a syntax error rather +// than being silently dropped while the parseable statements are returned. +#[test] +fn broken_statement_errors_instead_of_dropping() { + let result = format("SELECT 1; THIS IS NOT SQL @@@;", Style::River); + assert!( + matches!(result, Err(FormatError::Syntax(_))), + "expected Err(FormatError::Syntax(..)), got {result:?}" + ); +} + #[test] fn cast_multiword_type_names() { // Regression: multi-word type names in :: casts must not be truncated From 22486df6882f6ba2da261ab2d32df65c6e76d069 Mon Sep 17 00:00:00 2001 From: "Gavin M. Roy" Date: Mon, 6 Jul 2026 12:10:09 -0400 Subject: [PATCH 2/2] Dispatch pg_dump renderer via StyleConfig flag Address CodeRabbit review feedback: the root statement loop branched directly on `self.style == Style::PgDump`, which violates the formatter coding guideline of reading StyleConfig flags rather than matching on Style variants. Add a `pg_dump` boolean to StyleConfig and branch on `self.config.pg_dump` instead, consistent with the rest of the module. Co-Authored-By: Claude Fable 5 --- src/formatter/mod.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/formatter/mod.rs b/src/formatter/mod.rs index fac9fcf..7c1816e 100644 --- a/src/formatter/mod.rs +++ b/src/formatter/mod.rs @@ -36,6 +36,9 @@ pub(crate) struct StyleConfig { pub strip_inner_join: bool, /// Wrap CASE expressions when ELSE is present (AWeber, mattmc3). pub wrap_case_else: bool, + /// Use the dedicated pg_dump/ruleutils renderer instead of the standard + /// statement formatter. + pub pg_dump: bool, } impl StyleConfig { @@ -54,6 +57,7 @@ impl StyleConfig { blank_lines_in_ctes: false, strip_inner_join: false, wrap_case_else: false, + pg_dump: false, }, Style::Mozilla => Self { upper_keywords: true, @@ -68,6 +72,7 @@ impl StyleConfig { blank_lines_in_ctes: false, strip_inner_join: false, wrap_case_else: false, + pg_dump: false, }, Style::Aweber => Self { upper_keywords: true, @@ -82,6 +87,7 @@ impl StyleConfig { blank_lines_in_ctes: false, strip_inner_join: false, wrap_case_else: true, + pg_dump: false, }, Style::Dbt => Self { upper_keywords: false, @@ -96,6 +102,7 @@ impl StyleConfig { blank_lines_in_ctes: false, strip_inner_join: false, wrap_case_else: false, + pg_dump: false, }, Style::Gitlab => Self { upper_keywords: true, @@ -110,6 +117,7 @@ impl StyleConfig { blank_lines_in_ctes: true, strip_inner_join: false, wrap_case_else: false, + pg_dump: false, }, Style::Kickstarter => Self { upper_keywords: true, @@ -124,6 +132,7 @@ impl StyleConfig { blank_lines_in_ctes: false, strip_inner_join: false, wrap_case_else: false, + pg_dump: false, }, Style::Mattmc3 => Self { upper_keywords: false, @@ -138,6 +147,7 @@ impl StyleConfig { blank_lines_in_ctes: false, strip_inner_join: true, wrap_case_else: true, + pg_dump: false, }, // PgDump uses a dedicated renderer (see formatter::pgdump); these // values are placeholders and are not consulted on that path. @@ -154,6 +164,7 @@ impl StyleConfig { blank_lines_in_ctes: false, strip_inner_join: false, wrap_case_else: false, + pg_dump: true, }, } } @@ -187,7 +198,7 @@ impl<'a> Formatter<'a> { // `TransactionStmtLegacy`). Format the inner statement in // either case so it is not silently dropped. let stmt = child.find_child("stmt").unwrap_or(child); - if self.style == Style::PgDump { + if self.config.pg_dump { results.push(self.format_pgdump_stmt(stmt)?); } else { results.push(self.format_stmt(stmt)?);