Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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:
formatremoves comments #767 then reported comments being removed in real filesThis 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_workspacekeeps the original statement.How
1. Node locations (generated)
pgls_pretty_print_codegengeneratesnode_location(&NodeRef) -> Option<i32>from the protobuf descriptor: a node reports its byte offset when its message carries alocationfield.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:
;documents the next statement, and attaching it here would move it across a statement boundary. It is reported as unattached.3. Emission
EventEmitter::with_commentsconsumes 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_statementnow 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
pgls_workspacelosesstatement_contains_commentand 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 -- ...),INSERTcolumn lists, column type names,UPDATEassignment lists, window definitions,WITHclauses, the gap betweenWITHand 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.