Formatter overhaul: never remove comments, fmt directives, line wrapping, K&R style#7346
Open
ewels wants to merge 24 commits into
Open
Formatter overhaul: never remove comments, fmt directives, line wrapping, K&R style#7346ewels wants to merge 24 commits into
ewels wants to merge 24 commits into
Conversation
The formatter loses comments in many scenarios (language-server issues #111, #127, #140): - comments after the last statement of a block or file are deleted, including entire commented-out processes - inline trailing comments are moved below their statement - files with Windows (CRLF) line endings are corrupted: a comment and the following statement are merged onto a single line - comments are also deleted from: empty blocks, if/else and try/catch branches, closures, multi-line expressions, between process/workflow sections, above workflow emits/takes, typed process inputs, record fields, `when:` sections, and in config files (inline comments and comments at the end of a block or file) The root cause is that the parser only captures comments that immediately precede a statement or declaration (as LEADING_COMMENTS metadata) and drops everything else, and the formatter has no way to emit a comment that was never captured. CRLF corruption occurs because "\r\n" entries fail the "\n" checks in the parser and formatter. This commit makes formatting comment-lossless: - CommentReattacher (new): re-lexes the source with the ANTLR lexer (comments and newlines are NL tokens on the default channel, so string literals are handled exactly) and re-derives all comment metadata positionally: leading comments, same-line trailing comments, and two new kinds -- dangling comments (end of a block/file, emitted before the closing brace) and dangling-after comments (end of a section with no closing brace, e.g. the last workflow take). Comments inside multi-line expressions, which the formatter cannot emit in place, are hoisted above their statement instead of being deleted. All entries are normalized to LF. - Formatter, ScriptFormattingVisitor, ConfigFormattingVisitor: emit trailing comments at end of line, dangling comments before closing braces and at end of file, format closures with comments as multi-line blocks, and handle CRLF defensively. Also fixes the formatter emitting invalid syntax for workflows with onComplete/onError handlers (the main: label was omitted). - The formatting visitors accept the original source text as an optional constructor argument; when it is not provided, the source is re-read from the SourceUnit. Ported from nextflow-io/language-server, where these changes were originally developed as patched copies of the nf-lang formatter classes. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Add a maxLineLength setting to the FormattingOptions record (default 120, 0 to disable). The setting takes effect when line wrapping is implemented. The previous canonical constructor is kept as a compatibility constructor so that existing positional callers are unaffected. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
A comment on the closing-brace line of an if/else or try/catch statement was attached as a leading comment of the next statement, and since its line terminator belongs to a line of code, the next statement was emitted onto the comment line, corrupting the output. Track the enclosing statement as the trailing-comment candidate across its nested block regions so the comment is claimed as its trailing comment, and synthesize a line terminator for any comment entry that shares its line with code so a mis-attributed comment can never swallow the following code. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Emit `} else {` and `} catch (e: Exception) {` instead of putting the
keyword on its own line (language-server issue #153). When an else or
catch clause has a leading comment, fall back to the keyword on its own
line so the comment can be emitted above it.
Signed-off-by: Phil Ewels <phil.ewels@seqera.io>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Standardize the number of blank lines when formatting (language-server issues #115, #150): - runs of blank lines are collapsed into one; - blank lines at the start of a block or section are removed; - a blank line is enforced above every block declaration (process, workflow, function, params, output, record, enum) and between declarations of different kinds, while consecutive simple declarations of the same kind (includes, feature flags, legacy params) may remain grouped; - a shebang is followed by exactly one blank line. The same rules apply to config files: config blocks get a blank line above, and assignments and includeConfig statements may be grouped. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Multi-line strings were emitted verbatim, so their contents kept the indentation of the original source even when the formatter emitted the statement at a different indentation (e.g. when reformatting with a different tab size). Shift the interior lines of multi-line strings and GStrings by the difference between the source column and the emitted column of the opening quote, preserving the relative indentation within the string. Empty lines are left untouched, lines are never shifted past their first non-whitespace character, and files indented with tabs are left as-is (language-server issue #116). Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Allow sections of code to be excluded from formatting, following the semantics of Black (language-server issue #75): - `// fmt: skip` at the end of a line excludes the statement or declaration that ends on that line; - `// fmt: off` ... `// fmt: on` excludes the enclosed statements or declarations (an unterminated `fmt: off` extends to the end of the file). Excluded code is emitted verbatim from the source text, including its comments and blank lines. The directives work in both scripts and config files. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
When the formatted line of a statement (or config assignment) exceeds the maximum line length, roll it back and re-emit it with call arguments, list and map literals, and method chains wrapped onto multiple lines -- first only the outermost construct, then every construct if a line is still too long (e.g. deeply nested arguments). Expressions that were wrapped in the source are still wrapped either way, preserving the user's choice. Lines that cannot be wrapped (e.g. a single long string) are left as they are. The limit defaults to 120 columns and is configurable with the maxLineLength formatting option (0 disables wrapping). Ported from language-server issue #26. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
When the sort-declarations option is enabled, sort include declarations case-insensitively by source path within blank-line-separated groups (language-server issue #54). Comments above a group are pinned to the top of the group, and comments attached to an include move with it. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Comments inside multi-line expressions were hoisted above their statement because the formatter re-synthesizes expressions and had no emission point for them. Give the two constructs that render one line per part their own comment slots: - comments before a link of the statement's root method chain are emitted before that link, and a chain that carries comments is always wrapped; - comments before an element of a multi-line call, list or map are emitted before that element, and a collection whose elements carry comments is always wrapped. Comments in positions the formatter still cannot emit in place (e.g. inside operator expressions, GString interpolations, or directive arguments, which are never wrapped) continue to be hoisted above the statement rather than being removed. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Extend fmt: skip / fmt: off/on coverage to the remaining emission sites in the script formatter: process directives, typed outputs, workflow publishers, when-sections (v1 and v2), record fields, enum constants, and output-block bodies. Each site now checks fmt.appendVerbatim() before formatting so that suppressed nodes are emitted verbatim from the original source. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Complete the in-place expression comment support so that fewer comments are hoisted above the statement: - a comment on the same line as a wrapped element, chain link or ternary condition stays on that line as a trailing comment - comments between the last element of a wrapped call, list or map and its closing bracket are emitted in place, before the bracket - comments on the branches of a wrapped ternary stay at the branches - blank lines around element comments are preserved, so comment groups inside wrapped constructs keep their spacing - multiple comments in one expression slot keep their source order (previously they were emitted in reverse) - method chains in emit/publish values, process outputs, param and config values are recognized as wrappable roots (via the new Formatter#visitRootExpression), so their comments stay at their links; these values also participate in line-length wrapping When-expressions keep their previous hoisting behavior: they receive their own leading comments from the section emission, so chain-link comments there would be emitted twice. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Comments between the closing brace of a try (or previous catch) block and the following catch clause were emitted inside the previous block, before its closing brace. Each catch clause's block region now starts at the end of the previous block and carries the clause as its leading target, so those comments become the clause's leading comments and are emitted directly above it (the clause falls back from K&R style to its own line so the comments can be emitted). Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Add CommentReattacher.commentTexts, which collects the comments of a source text as a sorted list of normalized comment texts (LF line endings, surrounding whitespace stripped). Comparing the texts before and after formatting detects not only lost comments but also duplicated or altered ones, which a plain count comparison misses. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Pass the original file text to the formatting visitors and verify, before writing the formatted output back to disk, that formatting neither removed, duplicated nor altered any comment (using CommentReattacher.commentTexts). If the check fails, the file is left untouched and a warning is reported, so no formatter edge case can silently delete comments. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
In a script consisting of top-level statements (a code snippet), the synthetic entry workflow has no source position, so the region builder skipped it entirely: every comment fell into the file-level dangling slot while the statements kept their parser-attached leading comments, and formatting emitted each comment twice. Anchor the top-level statements of a code snippet directly in the file region so that their comment metadata is re-derived like any other statements. Found by formatting this repo's own test fixtures and asserting that comment texts are preserved. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
A blank line between a compound statement (if/else, try/catch) and the next statement was stripped: in slot order the compound statement's nested block regions come after the statement itself, so the gap assignment saw a region as the previous slot and applied the start-of-block blank-line stripping. Recognize the enclosing statement (already tracked as the trailing-comment candidate) as the previous statement, so blank lines between two statements are preserved. With that fixed, a declaration that is sorted into first position could carry a blank-line prefix from its original location and emit blank lines at the top of the file; strip the prefix of the first emitted declaration. Found by asserting formatting idempotence over this repo's own test fixtures. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
The chain-wrap decision walked the chain through method calls only, so a property link (e.g. `foo.out` in `foo.out.collect().view()`) became the chain root. When the source wrapped the chain at the property link, the root appeared multi-line and the chain was not wrapped -- flattening a source-wrapped chain on the first pass and re-wrapping it on the second, so formatting was not idempotent. Step through property links when locating the root, attributing their multi-line-ness to the chain layout rather than the root expression. Also add a corpus harness (FormatterCorpusHarness) that formats the repository's own test fixtures (tests/, docs/snippets/, validation/) under every formatting option combination and asserts that comment texts are preserved and formatting is idempotent. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
The main: label is preceded by a blank line to separate it from the take: section above it. When there is no take: section (e.g. the label is only required because of an emit: section or an onComplete/onError handler), the blank line was emitted at the start of the workflow body, contradicting the blank-line normalization rules. Emit the separator only when a take: section was emitted. Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Add unit tests pinning the behavior for every formatting issue reported against the language server that is addressed by the formatter, plus ported changes that previously had no dedicated tests: - trailing comments after compound statements stay in place - fmt: skip on process directives, record fields and when-sections - string escapes (backslashes, escaped dollars and quotes) are preserved (language-server #41, #55) - the spread operator is preserved in method calls (#70) - source-wrapped calls stay wrapped and trailing commas are preserved/added (#60, #74) - line wrapping is disabled when maxLineLength is 0 (#26) - harshil alignment of includes and workflow emits, and the intended non-alignment of legacy process outputs (#71, #135) - mahesh form moves process outputs after the script (#129) - the comment safety-check API (countComments/commentTexts), including CRLF normalization and shebang exclusion - config: harshil alignment of block assignments, and fmt: skip / fmt: off/on in config files (#75) Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Pin the formatter behavior for the reproduction cases reported in formatter issues across nextflow and language-server, gathered from the full issue threads: - a multi-line list stays wrapped only with a trailing comma; without one it is collapsed when it fits (nextflow #7328, as intended per the magic trailing comma) - a chained closure piped to a process formats to valid, idempotent output (nextflow #6971) - parentheses around dynamic map keys are preserved (nextflow #6892) - comments in chained closures and at the end of the file are preserved (nextflow #6365) - trailing comments on the elements of a params list are preserved (language-server #140) - commented-out elements of a wrapped call are preserved (language-server #67) - the full string-escape matrix from language-server #41 and the heredoc case from #55 format losslessly and idempotently - mahesh form keeps the when: section before the script and emits a valid section order (language-server #129) Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Code-quality pass over the formatter overhaul, no behavior changes: - share the sourceText fallback between the two formatting visitors as CommentReattacher.readSourceText - move leadingStartsWithBlankLine/stripLeadingBlankPrefix into Formatter, next to the other LEADING_COMMENTS accessors, and reuse them from both visitors - extract the duplicated when-section emission of the two process visitors into visitProcessWhen - extract shouldWrapConstruct for the wrap condition triplicated across the tuple/list/map visitors - extract claimTrailingComment, splitAfterLastBlankLine and nestedInAnchor for logic duplicated within CommentReattacher, and drop an unreachable half of a guard in appendDangling - use hasLeadingComments instead of raw metadata null-checks - implement countComments in terms of commentTexts - binary-search the sorted token list in gapTokens instead of scanning all tokens per gap (the hottest loop of the reattacher was quadratic in file size) - skip the comment-preservation check in CmdLint when formatting did not change the file, and avoid re-reading the file to detect changes - reuse parsers and hoist loop-invariant work in the corpus harness - replace imports for inlined fully-qualified class names - collapse 17 checkFormat(input, output) test calls with identical literals into the single-argument round-trip form Signed-off-by: Phil Ewels <phil.ewels@seqera.io> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Three fixes found by adversarial review of the formatter overhaul: - A fmt: off / fmt: on region that straddles declaration boundaries (e.g. off in one workflow, on in the next) emitted mismatched braces and duplicated declarations. Verbatim ranges are now promoted to the smallest enclosing anchor when the covered anchors span more than one region, so whole declarations are excluded from formatting instead. - Re-deriving comment metadata twice on the same AST (as the language server does when a document is formatted again without re-parsing) duplicated comments inside wrapped expressions, because the interior slot targets are not covered by clearMetadata. Stale metadata is now cleared before the slot assignment, and verbatim markers are cleared alongside the other comment metadata. - A declaration suppressed by a verbatim region emitted a stray blank line, because the blank-line separation loop counted it as a regular declaration. Suppressed declarations and config statements are now skipped. Also removes the unused isFirst parameter left over from the if/else K&R refactoring. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XjbDLPtxGXNFCWnTtAytV Signed-off-by: Claude <noreply@anthropic.com>
Line wrapping at the default 120 characters was not configurable from the CLI. Add a -line-length option (0 disables wrapping) and document it, along with the fmt: skip / fmt: off / fmt: on directives, in the CLI reference and the VS Code page. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014XjbDLPtxGXNFCWnTtAytV Signed-off-by: Claude <noreply@anthropic.com>
✅ Deploy Preview for nextflow-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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.
The formatter (
nextflow lint -formatand the language server / VS Code extension) can silently delete or corrupt comments: inline comments are moved off their line, comments after the last statement of a block are dropped, commented-out processes vanish, and CRLF files are corrupted by merging comments with the following code. For a tool that rewrites users' files in place, silent data loss is the worst possible failure mode, and it has been the most-reported formatter problem across both this repo and the language-server tracker. This PR overhauls the formatter innf-langso that formatting can never lose or alter a comment, and closes out the backlog of most-requested formatting features on top of that guarantee:fmt: skip/fmt: off/fmt: ondirectives, line wrapping at a configurable maximum length, K&R-styleif/elseandtry/catch, blank-line normalization, multi-line string re-indentation, and include sorting.Closes:
nextflow lint -formatis removes comments #6365elseformatting language-server#153Every closed issue's reproduction case is pinned as a unit test, along with the repro cases from previously-closed formatter issues (#6892, #6971, #7328, and language-server #41, #55, #60, #67, #70, #71, #129, #135). Beyond unit tests, the branch adds a corpus harness that formats this repository's own fixtures (
tests/,docs/snippets/,validation/— 255 files) under all 48 formatting-option combinations and asserts comment-text preservation and idempotence: 12,240 checks, all green. As a final backstop,nextflow lint -formatnow verifies comment preservation before writing a file back and refuses to format (with a warning) rather than ever losing a comment.Implementation details
Why comments were being lost
The parser only captures comments that immediately precede a statement or declaration (as
LEADING_COMMENTSmetadata) and drops everything else — comments after the last statement of a block, at the end of the file, in empty blocks, between process/workflow sections, inside multi-line expressions, and so on. The formatter re-synthesizes output from the AST, so a comment that was never captured cannot be emitted. CRLF corruption occurred because"\r\n"entries fail the"\n"checks in the parser and formatter.CommentReattacher
The core of the fix is a new class,
CommentReattacher, which runs before formatting and re-derives all comment metadata from the original source text:NLtokens on the default channel), so string literals, GStrings and slashy strings are handled exactly — no regex heuristics.)/], ternary branches, and catch clauses all have their own comment slots. Every attachment site has a matching force-wrap condition, so an attached comment always has an emission point. Comments in positions the formatter genuinely cannot emit (e.g. inside operator expressions) are hoisted above their statement instead of being dropped.The formatter gained the corresponding emission support: trailing comments at end of line, dangling comments before closing braces and at end of file, closures with comments formatted as multi-line blocks, and a fix for workflows with
onComplete/onErrorhandlers (themain:label was previously omitted, producing invalid output).Features
fmt: skipandfmt: off/fmt: on(Black semantics): excluded code is emitted verbatim from the source, including its comments and blank lines. Works in scripts and config files, on statements, declarations, process directives, section labels, record fields and enum constants. A region that straddles declaration boundaries is promoted to cover whole declarations, so the formatter can never emit mismatched braces.-line-lengthCLI option (0 disables).} else {and} catch (e: Exception) {; a commentedelse/catchfalls back to its own line so the comment can be emitted.-sort-declarationsoption): case-insensitive by module path within blank-line-separated groups; group header comments stay pinned to the group top.Safety check in
nextflow lintCommentReattacher.commentTextscollects the comments of a source text as a sorted, normalized list.nextflow lint -formatcompares this list before and after formatting and refuses to write the file (with a warning) on any mismatch — so no future formatter edge case can silently delete, duplicate or alter a comment.Bugs found by corpus testing
Formatting this repository's own fixtures under every option combination surfaced and fixed several bugs beyond the reported issues: comment duplication in code-snippet scripts (top-level statements), blank lines eaten after compound statements, a sorted-first declaration emitting blank lines at the top of the file, non-idempotent wrapping of method chains rooted at property accesses (
foo.out.collect()), stray blank lines from verbatim-suppressed declarations, and comment duplication when re-deriving metadata twice on the same AST (as the language server does with cached ASTs).API notes
FormattingOptionsgained amaxLineLengthcomponent; the previous canonical constructor is kept, so existing positional callers compile unchanged.SourceUnit, so existing 2-arg callers keep working.CommentReattacher.countComments/commentTextsare public so that other formatter front-ends (e.g. the language server) can implement the same refuse-to-format guard.Testing
nf-langformatter suites (about 100 new), including a pinned reproduction for every issue closed by this PR and for the previously-closed formatter issues.FormatterCorpusHarnessformats 255 fixture files under 48 option combinations (tabs/spaces × harshilAlignment × maheshForm × sortDeclarations × maxLineLength 0/40/120), assertingcommentTexts(before) == commentTexts(after)andformat(format(x)) == format(x)throughout: 12,240 checks, 0 failures.🤖 Generated with Claude Code
https://claude.ai/code/session_01WFMWUB1sQEXpHgLa74xsRQ
Generated by Claude Code