Skip to content

feat(format): format statements containing SQL comments - #800

Open
edjubert wants to merge 23 commits into
supabase-community:mainfrom
edjubert:edjubert/comment-support
Open

edjubert wants to merge 23 commits into
supabase-community:mainfrom
edjubert:edjubert/comment-support

Conversation

@edjubert

Copy link
Copy Markdown

What

Comments are not represented in the AST produced by libpg_query.
The formatter therefore cannot preserve them by rendering the AST alone.

The current safeguard, introduced by #768, preserves comments but leaves any statement containing an internal comment completely unchanged.
This PR implements comment-aware formatting: it attaches source comments to nearby AST nodes, emits them with those nodes, and formats the rest of the statement normally.

For example, these statements are currently preserved verbatim and are now formatted while keeping each comment with the expression it documents:

SELECT a, -- temporarily omit b
b FROM t;

SELECT * FROM t WHERE a = 1 -- keep condition context
AND b = 2;

Both round-trip to a stable layout, and formatting them a second time is a no-op.

Previous work

This is the follow-up iteration discussed while the formatter was being introduced:

This PR builds on that safety fix rather than replacing it with an unconditional best effort: if every comment cannot be placed, or if repeated formatting does not converge to a stable layout, the formatter returns an error and pgls_workspace keeps the original statement.

How

1. Node locations (generated)

pgls_pretty_print_codegen generates node_location(&NodeRef) -> Option<i32> from the protobuf descriptor: a node reports its byte offset when its message carries a location field.
Attachment is positional because that is the only information libpg_query keeps.
It does not change the parser or pretend that comments are part of the PostgreSQL AST.

2. Attachment (comments.rs)

For every comment in the source:

  • a comment preceded only by whitespace on its line is leading and belongs to the next node
  • a comment following SQL on the same line is trailing and belongs to the previous node
  • the preferred side is tried first and the other one is the fallback, so a comment closing a list or a statement — with no node after it - is printed after the node it already follows in the source, where its author wrote it
  • a line comment written after a ; documents the next statement, and attaching it here would move it across a statement boundary. It is reported as unattached.

3. Emission

EventEmitter::with_comments consumes the leading/trailing maps as nodes are emitted, and the renderer gained comment layout events - a -- comment forces a break, a /* */ one does not.

4. Refusing rather than dropping

Two new errors, both leaving the statement exactly as written:

  • UnplaceableComment — a comment could not be placed (unattached, or still pending after emission)
  • NonIdempotentCommentLayout — reformatting the statement cycled between layouts instead of converging.

format_statement now takes the source text alongside the AST and iterates up to 6 passes until the output is a fixed point, keeping the set of layouts seen to detect a cycle.
A statement with no comment takes the single-pass path and is unaffected.

This deliberately preserves the failure mode of #768: an unsafe statement is returned unchanged rather than formatted with a missing or displaced comment.

API change

// before
pub fn format_statement(ast: &NodeEnum, config: &FormatConfig) -> Result<FormatResult, FormatError>

// after
pub fn format_statement(ast: &NodeEnum, sql: &str, config: &FormatConfig) -> Result<FormatResult, FormatError>

pgls_workspace loses statement_contains_comment and both of its call sites.

Coverage

Beyond the generic leading/trailing cases, each of the following needed its own emitter to consume the comments of a node it was printing itself: DML target relations (UPDATE t -- ...), INSERT column lists, column type names, UPDATE assignment lists, window definitions, WITH clauses, the gap between WITH and the DML it feeds, sequence options (CREATE SEQUENCE ... START 1 -- ...), comments after a grouped condition, and a leading comment between two boolean operands.
There is an idempotence test per case.

Tests

cargo test -p pgls_pretty_print -p pgls_workspace. Every comment test asserts both that the comment survives and that a second formatting pass is a no-op.

#767 was already closed by #768. This PR is a follow-up to #768 and to the comment-support iteration discussed during review of #546.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant