Skip to content

Fix formatter comment handling around SQL expressions #884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mk3008 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix formatter comment handling around SQL expressions #884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-comment-formatting.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"rawsql-ts": patch
---

Fix SQL formatter comment handling for comma-prefixed expressions, LIMIT/OFFSET values, and parenthesized predicates. Comments are no longer duplicated around function arguments, ORDER BY/GROUP BY items, and parenthesized WHERE predicates, comments after HAVING/JOIN ON/LIMIT/OFFSET keywords are preserved, and comments after list commas now use readable before-comma and after-comma layouts for SELECT, ORDER BY, and GROUP BY items.
9 changes: 8 additions & 1 deletion packages/core/src/parsers/HavingParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,22 @@ export class HavingClauseParser {
if (lexemes[idx].value !== 'having') {
throw new Error(`Syntax error at position ${idx}: Expected 'HAVING' keyword but found "${lexemes[idx].value}". HAVING clauses must start with the HAVING keyword.`);
}
const havingKeywordComments = lexemes[idx].positionedComments;
idx++;

if (idx >= lexemes.length) {
throw new Error(`Syntax error: Unexpected end of input after 'HAVING' keyword. The HAVING clause requires a condition expression.`);
}

const item = ValueParser.parseFromLexeme(lexemes, idx);
const afterKeywordComments = havingKeywordComments
?.filter(comment => comment.position === 'after')
.flatMap(comment => comment.comments) ?? [];
if (afterKeywordComments.length > 0) {
item.value.addPositionedComments('before', afterKeywordComments);
}
const clause = new HavingClause(item.value);

return { value: clause, newIndex: item.newIndex };
}
}
}
7 changes: 7 additions & 0 deletions packages/core/src/parsers/JoinOnClauseParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,16 @@ export class JoinOnClauseParser {
public static tryParse(lexemes: Lexeme[], index: number): { value: JoinOnClause; newIndex: number } | null {
let idx = index;
if (idx < lexemes.length && lexemes[idx].value === 'on') {
const onKeywordComments = lexemes[idx].positionedComments;
idx++; // Skip 'on' keyword
// Parse the condition expression
const condition = ValueParser.parseFromLexeme(lexemes, idx);
const afterKeywordComments = onKeywordComments
?.filter(comment => comment.position === 'after')
.flatMap(comment => comment.comments) ?? [];
if (afterKeywordComments.length > 0) {
condition.value.addPositionedComments('before', afterKeywordComments);
}
idx = condition.newIndex;
const joinOn = new JoinOnClause(condition.value);
return { value: joinOn, newIndex: idx };
Expand Down
9 changes: 8 additions & 1 deletion packages/core/src/parsers/LimitClauseParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export class LimitClauseParser {
if (lexemes[idx].value !== 'limit') {
throw new Error(`Syntax error at position ${idx}: Expected 'LIMIT' keyword but found "${lexemes[idx].value}". LIMIT clauses must start with the LIMIT keyword.`);
}
const limitKeywordComments = lexemes[idx].positionedComments;
idx++;

if (idx >= lexemes.length) {
Expand All @@ -35,10 +36,16 @@ export class LimitClauseParser {

// Parse LIMIT value
const limitItem = ValueParser.parseFromLexeme(lexemes, idx);
const afterKeywordComments = limitKeywordComments
?.filter(comment => comment.position === 'after')
.flatMap(comment => comment.comments) ?? [];
if (afterKeywordComments.length > 0) {
limitItem.value.addPositionedComments('before', afterKeywordComments);
}
idx = limitItem.newIndex;

const clause = new LimitClause(limitItem.value);

return { value: clause, newIndex: idx };
}
}
}
7 changes: 7 additions & 0 deletions packages/core/src/parsers/OffsetClauseParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export class OffsetClauseParser {
if (lexemes[idx].value !== 'offset') {
throw new Error(`Syntax error at position ${idx}: Expected 'OFFSET' keyword but found "${lexemes[idx].value}". OFFSET clauses must start with the OFFSET keyword.`);
}
const offsetKeywordComments = lexemes[idx].positionedComments;
idx++;

if (idx >= lexemes.length) {
Expand All @@ -35,6 +36,12 @@ export class OffsetClauseParser {

// Parse OFFSET value
const offsetItem = ValueParser.parseFromLexeme(lexemes, idx);
const afterKeywordComments = offsetKeywordComments
?.filter(comment => comment.position === 'after')
.flatMap(comment => comment.comments) ?? [];
if (afterKeywordComments.length > 0) {
offsetItem.value.addPositionedComments('before', afterKeywordComments);
}
idx = offsetItem.newIndex;

// If there is a "row" or "rows" command, skip it
Expand Down
33 changes: 30 additions & 3 deletions packages/core/src/parsers/SqlPrintTokenParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ export class SqlPrintTokenParser implements SqlComponentVisitor<SqlPrintToken> {
private sourceAliasStyle: SourceAliasStyle;
private orderByDefaultDirectionStyle: OrderByDefaultDirectionStyle;
private readonly normalizeJoinConditionOrder: boolean;
private readonly listContinuationCommentComponents = new WeakSet<SqlComponent>();
private joinConditionContexts: Array<{ aliasOrder: Map<string, number> }> = [];

constructor(options?: {
Expand Down Expand Up @@ -491,6 +492,7 @@ export class SqlPrintTokenParser implements SqlComponentVisitor<SqlPrintToken> {

private visitQualifiedName(arg: QualifiedName): SqlPrintToken {
const token = new SqlPrintToken(SqlPrintTokenType.container, '', SqlPrintTokenContainerType.QualifiedName);
const hasOwnComments = this.hasPositionedComments(arg) || this.hasLegacyComments(arg);

if (arg.namespaces) {
for (let i = 0; i < arg.namespaces.length; i++) {
Expand All @@ -517,7 +519,7 @@ export class SqlPrintTokenParser implements SqlComponentVisitor<SqlPrintToken> {
arg.name.comments = originalNameLegacyComments;

// Apply the name's comments to the qualified name token
if (this.hasPositionedComments(arg.name) || this.hasLegacyComments(arg.name)) {
if (!hasOwnComments && (this.hasPositionedComments(arg.name) || this.hasLegacyComments(arg.name))) {
this.addComponentComments(token, arg.name);
}

Expand Down Expand Up @@ -1120,8 +1122,22 @@ export class SqlPrintTokenParser implements SqlComponentVisitor<SqlPrintToken> {

private visitColumnReference(arg: ColumnReference): SqlPrintToken {
const token = new SqlPrintToken(SqlPrintTokenType.container, '', SqlPrintTokenContainerType.ColumnReference);
const hasOwnComments = this.hasPositionedComments(arg) || this.hasLegacyComments(arg);
const originalNamePositionedComments = arg.qualifiedName.name.positionedComments;
const originalNameComments = arg.qualifiedName.name.comments;

if (hasOwnComments) {
arg.qualifiedName.name.positionedComments = null;
arg.qualifiedName.name.comments = null;
}

token.innerTokens.push(arg.qualifiedName.accept(this));

if (hasOwnComments) {
arg.qualifiedName.name.positionedComments = originalNamePositionedComments;
arg.qualifiedName.name.comments = originalNameComments;
}

this.addComponentComments(token, arg);

return token;
Expand Down Expand Up @@ -1480,6 +1496,14 @@ export class SqlPrintTokenParser implements SqlComponentVisitor<SqlPrintToken> {
innerAfterComments = arg.expression.getPositionedComments('after');
arg.expression.positionedComments = null;
}
if (hasOwnComments && innerBeforeComments.length > 0) {
const innerBeforeSet = new Set(innerBeforeComments);
arg.positionedComments = arg.positionedComments
?.map(comment => comment.position === 'after'
? { ...comment, comments: comment.comments.filter(value => !innerBeforeSet.has(value)) }
: comment)
.filter(comment => comment.comments.length > 0) ?? null;
}

// Build basic structure first
token.innerTokens.push(SqlPrintTokenParser.PAREN_OPEN_TOKEN);
Expand Down Expand Up @@ -1976,10 +2000,9 @@ export class SqlPrintTokenParser implements SqlComponentVisitor<SqlPrintToken> {
const afterComments = arg.getPositionedComments('after');

if (beforeComments.length > 0) {
if (arg.value instanceof CaseExpression) {
if (arg.value instanceof CaseExpression || this.listContinuationCommentComponents.has(arg)) {
const commentBlocks = this.createCommentBlocks(beforeComments);
token.innerTokens.push(...commentBlocks);
token.innerTokens.push(new SqlPrintToken(SqlPrintTokenType.commentNewline, ''));
} else {
const commentTokens = this.createInlineCommentSequence(beforeComments);
token.innerTokens.push(...commentTokens);
Expand Down Expand Up @@ -2119,8 +2142,12 @@ export class SqlPrintTokenParser implements SqlComponentVisitor<SqlPrintToken> {
for (let i = 0; i < arg.items.length; i++) {
if (i > 0) {
token.innerTokens.push(...SqlPrintTokenParser.commaSpaceTokens());
this.listContinuationCommentComponents.add(arg.items[i]);
}
token.innerTokens.push(this.visit(arg.items[i]));
if (i > 0) {
this.listContinuationCommentComponents.delete(arg.items[i]);
}
Comment on lines 2143 to +2150

@coderabbitai coderabbitai Bot Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Ensure list-continuation marker cleanup is exception-safe.

Line 2145 adds to listContinuationCommentComponents, but cleanup at Line 2149 is skipped if this.visit(arg.items[i]) throws. That can leak transient formatter state into subsequent reuse of the same parser instance.

Suggested fix 
 for (let i = 0; i < arg.items.length; i++) {
     if (i > 0) {
         token.innerTokens.push(...SqlPrintTokenParser.commaSpaceTokens());
-        this.listContinuationCommentComponents.add(arg.items[i]);
+        this.listContinuationCommentComponents.add(arg.items[i]);
     }
-    token.innerTokens.push(this.visit(arg.items[i]));
-    if (i > 0) {
-        this.listContinuationCommentComponents.delete(arg.items[i]);
-    }
+    try {
+        token.innerTokens.push(this.visit(arg.items[i]));
+    } finally {
+        if (i > 0) {
+            this.listContinuationCommentComponents.delete(arg.items[i]);
+        }
+    }
 }
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/parsers/SqlPrintTokenParser.ts` around lines 2143 - 2150,
The list-continuation marker is added to this.listContinuationCommentComponents
before calling this.visit(arg.items[i]) but removed only after the call, so an
exception from visit will leave the marker in the set; make the operation
exception-safe by wrapping the visit call in a try/finally (or equivalent) so
that for the code path that adds the marker (the i > 0 branch) you always remove
it in the finally block; update the block around
SqlPrintTokenParser.commaSpaceTokens(),
this.listContinuationCommentComponents.add(arg.items[i]),
this.visit(arg.items[i]) and the corresponding delete to ensure delete runs even
if visit throws.

}

return token;
Expand Down
52 changes: 50 additions & 2 deletions packages/core/src/transformers/SqlPrinter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,13 +344,19 @@ export class SqlPrinter {

const hasRenderableLeadingComment = leadingCommentContexts.some(item => item.shouldRender);
const leadingCommentIndentLevel = hasRenderableLeadingComment
? this.getLeadingCommentIndentLevel(parentContainerType, level)
? this.getLeadingCommentIndentLevel(
token.containerType === SqlPrintTokenContainerType.SelectItem
? token.containerType
: parentContainerType,
level
)
: null;

if (
hasRenderableLeadingComment
&& !this.isOnelineMode()
&& this.shouldAddNewlineBeforeLeadingComments(parentContainerType)
&& !this.shouldKeepLeadingCommentOnCommaLine()
) {
const currentLine = this.linePrinter.getCurrentLine();
if (currentLine.text.trim().length > 0) {
Expand All @@ -373,6 +379,12 @@ export class SqlPrinter {
leading.context,
false
);
if (
token.containerType === SqlPrintTokenContainerType.SelectItem &&
this.smartCommentBlockBuilder?.mode === 'line'
) {
this.flushSmartCommentBlockBuilder();
}
}

if (this.smartCommentBlockBuilder && token.containerType !== SqlPrintTokenContainerType.CommentBlock && token.type !== SqlPrintTokenType.commentNewline) {
Expand Down Expand Up @@ -400,7 +412,17 @@ export class SqlPrinter {
if (!this.shouldRenderComment(token, effectiveCommentContext)) {
return;
}
if (this.shouldKeepLeadingCommentOnCommaLine()) {
this.ensureTrailingSpace();
}
const commentLevel = this.getCommentBaseIndentLevel(level, parentContainerType);
if (
parentContainerType === SqlPrintTokenContainerType.SelectItem &&
effectiveCommentContext.position === 'leading' &&
this.linePrinter.getCurrentLine().text.trim() === ''
) {
this.linePrinter.getCurrentLine().level = commentLevel;
}
this.handleCommentBlockContainer(token, commentLevel, effectiveCommentContext);
return;
}
Expand Down Expand Up @@ -1375,7 +1397,11 @@ export class SqlPrinter {
}
const content = this.extractLineCommentContent(trimmed);
if (content !== null) {
lines.push(content);
if (!content && trimmed.startsWith('/*') && trimmed.endsWith('*/')) {
lines.push(this.sanitizeCommentLine(this.escapeCommentDelimiters(trimmed)));
} else {
lines.push(content);
}
Comment on lines +1400 to +1404

@coderabbitai coderabbitai Bot Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid rewriting empty block comments into escaped delimiter text.

At Line 1401, /* */ with empty inner content is converted into escaped delimiter text, which can later print as comment payload text in smart mode. This changes comment semantics instead of preserving an empty comment/blank-comment intent.

Suggested fix 
 if (content !== null) {
-    if (!content && trimmed.startsWith('/*') && trimmed.endsWith('*/')) {
-        lines.push(this.sanitizeCommentLine(this.escapeCommentDelimiters(trimmed)));
-    } else {
-        lines.push(content);
-    }
+    if (!content && trimmed.startsWith('/*') && trimmed.endsWith('*/')) {
+        // Preserve empty block-comment intent without emitting escaped delimiters as text.
+        lines.push('');
+    } else {
+        lines.push(content);
+    }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!content && trimmed.startsWith('/*') && trimmed.endsWith('*/')) {
lines.push(this.sanitizeCommentLine(this.escapeCommentDelimiters(trimmed)));
} else {
lines.push(content);
}
if (!content && trimmed.startsWith('/*') && trimmed.endsWith('*/')) {
// Preserve empty block-comment intent without emitting escaped delimiters as text.
lines.push('');
} else {
lines.push(content);
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/core/src/transformers/SqlPrinter.ts` around lines 1400 - 1404, The
code currently converts an empty block comment (when content is falsy and
trimmed startsWith '/*' and endsWith '*/') into escaped delimiter text via
escapeCommentDelimiters, which changes semantics; change the branch in
SqlPrinter.ts so that when content is empty you preserve the empty block comment
rather than escaping delimiters—e.g. call sanitizeCommentLine(trimmed) or push
the original trimmed string directly instead of
sanitizeCommentLine(this.escapeCommentDelimiters(trimmed)); update the
conditional around sanitizeCommentLine/escapeCommentDelimiters (references:
sanitizeCommentLine, escapeCommentDelimiters, lines, trimmed) so only non-empty
block comments get delimiter-escaping.

}
}
}
Expand Down Expand Up @@ -1575,6 +1601,12 @@ export class SqlPrinter {
if (parentType === SqlPrintTokenContainerType.SelectClause) {
return true;
}
if (
parentType === SqlPrintTokenContainerType.OrderByItem ||
parentType === SqlPrintTokenContainerType.GroupByClause
) {
return true;
}
if (parentType === SqlPrintTokenContainerType.ExplainStatement) {
// Ensure EXPLAIN targets print header comments on a dedicated line.
return true;
Expand All @@ -1586,6 +1618,15 @@ export class SqlPrinter {
}

private getLeadingCommentIndentLevel(parentType: SqlPrintTokenContainerType | undefined, currentLevel: number): number {
if (parentType === SqlPrintTokenContainerType.SelectItem) {
return currentLevel;
}
if (
parentType === SqlPrintTokenContainerType.OrderByItem ||
parentType === SqlPrintTokenContainerType.GroupByClause
) {
return currentLevel;
}
if (parentType === SqlPrintTokenContainerType.TupleExpression) {
return currentLevel + 1;
}
Expand All @@ -1603,6 +1644,13 @@ export class SqlPrinter {
return currentLevel;
}

private shouldKeepLeadingCommentOnCommaLine(): boolean {
if (this.commaBreak !== 'before') {
return false;
}
return this.linePrinter.getCurrentLine().text.trim() === ',';
}

/**
* Determines if the printer is in oneliner mode.
* Oneliner mode uses single spaces instead of actual newlines.
Expand Down
Loading
Loading