Skip to content

fix(filter): preserve code around inline block comments - #2715

Closed
guyoron1 wants to merge 1 commit into
rtk-ai:developfrom
guyoron1:fix/filter-inline-block-comment
Closed

guyoron1 wants to merge 1 commit into
rtk-ai:developfrom
guyoron1:fix/filter-inline-block-comment

Conversation

@guyoron1

@guyoron1 guyoron1 commented Jun 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes #2714

MinimalFilter dropped the whole line when it contained an inline block comment, so int x = 5; /* comment */ lost its code, and so did the closing line of a multi-line comment when code followed the */.

The line is now walked once: code before, between and after /* ... */ is kept, the comments are stripped, and an unterminated /* at the end of a line still carries state to the next line as before. The gap pszymkowiak found (only the first comment on a line was stripped and the rest leaked) is fixed here rather than deferred: every comment on a line is stripped and the code between them kept.

input   : int v1=1; /* c1 */ int v2=2; /* c2 */ int v3=3; /* c3 */ int v10=10; /* c10 */
develop : (the whole line is dropped)
this PR : int v1=1; int v2=2; int v3=3; int v10=10;

Doc comments (///, /** ... */) and Python docstrings are byte-identical to develop across a Rust / Go / JS / Python sweep.

Known limitations, both unchanged from the version that was reviewed: code after a */ that closes a multi-line comment loses its original indentation; and for Python block_start / block_end are both """, so x = """abc""" emits x = (develop drops the line entirely).

Tests

Five block-comment tests, four of which fail on develop's code (the fifth guards that full-line comments are still stripped), including several comments on one line and a mix of terminated and unterminated ones.

  • cargo fmt --all -- --check, cargo clippy --all-targets (0 warnings), cargo test --all (all pass)

@pszymkowiak

Copy link
Copy Markdown
Collaborator

Tested extensively with the compiled binary across 15 popular languages (Python, JavaScript, TypeScript, Java, C#, C++, C, Go, Rust, PHP, Ruby, Swift, Kotlin, SQL, Shell).

Good news: multi-line block comments with code before /* and after */ on the closing line work correctly everywhere tested — this is the PR's core stated fix and it holds up well, including 3+ line spans. Full-line comments still dropped correctly. No regression on trailing ///# comments (never stripped by MinimalFilter, with or without this change) or Python docstrings (intentionally preserved verbatim by a separate, pre-existing code path).

Found one gap, reproduces identically across every /* */-based language tested (generic to MinimalFilter, not language-specific): only the first block comment on a line is stripped — everything after it passes through untouched. Confirmed with 10 comments on one line (C): comment #1 removed, #2–10 all leak through verbatim with their markers.

$ rtk read - -l minimal <<< 'int v1=1; /* c1 */ int v2=2; /* c2 */ ... int v10=10; /* c10 */'
int v1 = 1; int v2 = 2; /* c2 */ ... int v10 = 10; /* c10 */

Scope note: only reachable via explicit rtk read -l minimal or -l aggressive (which chains through MinimalFilter) — rtk read defaults to -l none (no filtering), and no other rtk command uses this code path, so this doesn't affect default usage.

Not a regression either way (previously the whole line would've been dropped, losing the code too), but leaves literal comment markers in output the filter is supposed to remove. The after remainder needs to loop back through comment-stripping instead of being pushed verbatim after the first match. Not blocking — cargo fmt, cargo clippy --all-targets, cargo test --all all clean otherwise (2 unrelated pre-existing locale failures, same as other PRs today).

@pszymkowiak
pszymkowiak requested a review from aeppling July 9, 2026 11:20
@guyoron1

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough testing across 15 languages! The multiple-block-comments-on-one-line gap you found makes sense — it's a pre-existing limitation, not a regression. Happy to tackle it as a follow-up PR if you'd like. Is this one good to merge as-is?

@guyoron1
guyoron1 force-pushed the fix/filter-inline-block-comment branch from 7ff9929 to 7c47ef3 Compare August 14, 2026 19:01
MinimalFilter dropped entire lines containing inline block comments
like `int x = 5; /* comment */`, silently losing the code portion.
Also affected closing lines of multi-line comments when followed by
code.

Walk the line instead of matching a single comment: every block comment
on it is stripped and the code between them is kept, so a line with
several `/* … */` no longer leaks the second and later markers into the
output. An unterminated opener still carries the state to the next line.

Fixes rtk-ai#2714
@guyoron1
guyoron1 force-pushed the fix/filter-inline-block-comment branch from 7c47ef3 to 727fb7f Compare September 21, 2026 12:03
@guyoron1

Copy link
Copy Markdown
Contributor Author

@pszymkowiak thanks for the 15-language sweep. I fixed the gap you found in this PR rather than deferring it: the after remainder now goes back through comment-stripping instead of being pushed verbatim, so every /* ... */ on a line is stripped and the code between them kept. Your ten-comment line comes out as int v1=1; int v2=2; ... int v10=10; (develop drops the whole line; the version I pushed before leaked #2 to #10). An unterminated /* at the end of a line still carries state to the next line.

Two new tests cover several comments on one line and a terminated + unterminated mix; four of the five block-comment tests fail on develop. I re-ran a Rust / Go / JS / Python sweep and doc comments and docstrings come out byte-identical.

One thing I did not change: code after a */ that closes a multi-line comment still loses its original indentation, same as the version you reviewed.

Rebased on current develop. This edits the same function as #2713, so whichever merges second needs a mechanical rebase.

@guyoron1

Copy link
Copy Markdown
Contributor Author

Closing this. Develop moved in a way that puts it in real conflict with this PR's approach, not just a textual rebase conflict.

362065e (fix(read): only treat /* at line start as block comment opener, #2385) changed the opener check from trimmed.contains(start) to trimmed.starts_with(start), specifically because /* inside a string literal or glob ("src/*.rs") was being misread as a comment opener and — worse — leaking in_block_comment state into the rest of the file when no closer followed on that line.

This PR's whole point is the opposite: detecting a genuine /* ... */ that comes after real code on the same line (int x = 5; /* comment */), which by definition is not at the start of the trimmed line. Four of this PR's five new tests rely on exactly that (int v1 = 1; /* c1 */ ..., int a = 1; /* done */ ...). A shallow merge that keeps starts_with would make most of this PR's fix unreachable; keeping contains for the opener would reopen #2385's string/glob bug.

Doing both correctly needs real string-literal-aware scanning per language (recognizing /* inside "..." the way advance_triple_quote already does for Python's """), not a rebase. That's a bigger, separate change. Reopening if the same-line case (code, then a trailing /* comment */, both on one line) turns out to matter in practice — the multi-line case (closing */ followed by code) is already unaffected by #2385 and still works fine on develop as-is.

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.

Bug: MinimalFilter drops code on lines with inline block comments (code loss)

2 participants