docs: rewrite multi-line comment spec around chroma tokenizer
This commit is contained in:
@@ -1,96 +1,138 @@
|
||||
# Multi-Line Block Comment Support — Design
|
||||
# Multi-Line Block Comment Support (chroma-based) — Design
|
||||
|
||||
## Problem
|
||||
|
||||
Comment removal is per-line and stateless (`removeCommentsFromLine`). Multi-line block comments (`/* ... */`, `<!-- ... -->`) are corrupted: only the first line is truncated at the start marker, and the comment body plus closing marker are left as live code, breaking compilation. This is worst in the auto-hook path (`shush --changes-only`), which rewrites files unattended with no backup.
|
||||
Comment removal is per-line and stateless (`removeCommentsFromLine`). Multi-line block comments (`/* ... */`, `<!-- ... -->`) are corrupted: only the first line is truncated at the start marker; the body and closing marker remain as live code, breaking compilation. Worst in the auto-hook path (`shush --changes-only`), which rewrites files unattended with no backup. The root cause is that shush hand-rolls a lexer — the same class of bug also lurks in raw strings, template literals, and per-language char/lifetime rules.
|
||||
|
||||
Proven repros (both real runs, one compile-verified): a C license header and a straddling `/* ... */` both turn comment prose into code.
|
||||
## Approach: tokenize with chroma, drop comment tokens
|
||||
|
||||
## Goal
|
||||
Replace the hand-rolled scanner with `github.com/alecthomas/chroma/v2` (pure Go, same author as the existing `kong` dependency). chroma has lexers for ~200 languages that tokenize losslessly (concatenating token values reproduces the input byte-for-byte). Comment removal becomes: tokenize, drop the comment tokens, reassemble, then apply shush's line/whitespace semantics.
|
||||
|
||||
Correctly remove multi-line block comments across every supported language that has them, via a shared stateful engine, without regressing single-line behavior (locked by the existing `TestWholeFile` golden suite).
|
||||
A spike validated this against every hard case, all `lossless=true`:
|
||||
|
||||
| Case | old hand-rolled | chroma |
|
||||
|------|-----------------|--------|
|
||||
| C multi-line block / license header | corrupts | correct |
|
||||
| `//`/`#` marker inside string | correct | correct |
|
||||
| `//` inside block; `/*` inside line comment | correct | correct |
|
||||
| Go raw string `` `.. /* .. */ ..` `` | corrupts | correct |
|
||||
| JS template literal across lines with `/*` | corrupts | correct |
|
||||
| Rust `'a` lifetime + `'z'` char + block | corrupts | correct |
|
||||
| HTML `<!-- -->` multi-line | — | correct |
|
||||
| C `#include` / `#define` | n/a | **kept** (see preproc rule) |
|
||||
|
||||
**Build cost:** `CGO_ENABLED=0` static build succeeds, statically linked; binary grows 4.1M → ~5.7M (+~1.6M for chroma + pure-Go regexp2). Single static binary and cross-compile story preserved. tree-sitter was rejected: its grammars are C, forcing CGO and per-platform grammar bundling, which breaks the static build (a local nvim grammar install does not ship in the binary).
|
||||
|
||||
## Decisions (locked)
|
||||
|
||||
1. **Coverage:** one block fixture per extension for all block-comment languages — 26 `/* */` extensions (js, ts, jsx, tsx, go, c, cpp, cc, cxx, h, hpp, java, cs, rs, swift, kt, kts, dart, scala, php, css, scss, sass, less, sql) and 4 `<!-- -->` extensions (html, htm, xml, svg). Mixed and edge-case scenarios use a representative subset (they test the algorithm, not per-extension wiring).
|
||||
2. **Git boundary:** skip straddling blocks. A block fully inside the changed range is removed; a block crossing a range boundary is left completely intact. Honors the "surgical, only changed lines" promise; never corrupts.
|
||||
3. **Multi-line strings** (Go raw strings, JS/TS template literals spanning lines with markers inside): documented non-goal for v1. String protection stays per-line.
|
||||
4. **Preview:** `--dry-run` preview is routed through the same engine so it never misreports multi-line blocks.
|
||||
1. **Coverage:** one block fixture per extension for all block-comment languages — 26 `/* */` extensions and 4 `<!-- -->` extensions (list in Testing). Mixed and edge scenarios use a representative subset.
|
||||
2. **Git boundary:** skip straddling blocks — a comment fully inside the changed range is removed; one crossing a boundary is left intact. Falls out naturally from token-range gating.
|
||||
3. **Multi-line strings:** now handled correctly by chroma (no longer a non-goal).
|
||||
4. **Preview:** `--dry-run` routes through the same engine so it never misreports.
|
||||
|
||||
## Architecture
|
||||
|
||||
New file `internal/processor/engine.go` holds the single source of truth. It replaces the per-line `removeCommentsFromLine` as the transform used by all four call sites.
|
||||
New file `internal/processor/engine.go`:
|
||||
|
||||
```go
|
||||
type lineResult struct {
|
||||
num int
|
||||
original string
|
||||
result string
|
||||
action string
|
||||
emit bool
|
||||
type Options struct {
|
||||
Inline bool
|
||||
Block bool
|
||||
PreserveLines bool
|
||||
InRange func(line int) bool
|
||||
}
|
||||
|
||||
func (p *Processor) stripComments(lines []string, lang types.Language, cfg *config.Config, inRange func(lineNum int) bool) []lineResult
|
||||
type LineChange struct {
|
||||
Num int
|
||||
Original string
|
||||
Result string
|
||||
Action string
|
||||
}
|
||||
|
||||
func StripComments(src []byte, filename string, cfg *config.Config, opts Options) ([]byte, []LineChange, error)
|
||||
```
|
||||
|
||||
- `action` is one of `"kept"`, `"removed"`, `"modified"`, `"preserved"`.
|
||||
- `emit == false` means the line is dropped from the output file (a fully-removed line, unless `--preserve-lines` turns it into a blank/leading-whitespace line, which is emitted).
|
||||
- `inRange == nil` means whole-file processing; otherwise it is a predicate over 1-based line numbers used by git mode.
|
||||
- `filename` selects the lexer. `InRange == nil` means whole-file; otherwise a 1-based line predicate (git mode).
|
||||
- Returns the rewritten source plus per-original-line changes (`Action` in `"kept" | "removed" | "modified" | "preserved"`) for preview rendering.
|
||||
|
||||
Consumers:
|
||||
- `processFileInMemory` and `processFileWithLineRanges` (mutation): write `result` for every `emit` line.
|
||||
- `showPreview` and `showGitPreviewWithTotals` (display): render each line by `action`, count changed/kept/preserved.
|
||||
- `processFileInMemory`, `processFileWithLineRanges` (mutation): write the returned bytes.
|
||||
- `showPreview`, `showGitPreviewWithTotals` (display): render from `[]LineChange`.
|
||||
|
||||
## Algorithm
|
||||
The legacy `removeCommentsFromLine` path is retained only as the fallback described below.
|
||||
|
||||
Scan line by line, carrying block state via lookahead (a block is resolved to its end before deciding, so preservation and the straddle rule can see the whole block).
|
||||
## Lexer resolution
|
||||
|
||||
For each line, outside of strings, find the leftmost meaningful token:
|
||||
1. `lexer := lexers.Match("f." + ext)` (drive by extension so behavior matches shush's `languageMap`, not chroma's content analysis).
|
||||
2. Override map for known gaps: `less` → `lexers.Get("SCSS")` (identical `//` + `/* */` comment syntax; chroma has no Less lexer).
|
||||
3. If still nil, **fall back to the existing hand-rolled `languageMap` remover.** The only uncovered extension is `conf` (line comments only), which the legacy remover handles correctly — the multi-line block bug cannot occur for line-only languages.
|
||||
4. `lexer = chroma.Coalesce(lexer)` before tokenising.
|
||||
|
||||
- **String open** (`"`, `'`, `` ` ``): skip to its matching close on the same line, respecting `\` escapes. If it does not close on the line, the rest of the line is treated as string and state resets on the next line (per-line string handling, status quo).
|
||||
- **Line-comment marker** (processed only when not `--block`): cut from the marker to end of line. Because this is chosen only when it is the leftmost token, a `/*` appearing after `//` is inert (inside the line comment).
|
||||
- **Block-start marker** (skipped entirely when `--inline`): resolve the matching end marker via lookahead across following lines to form a block region, then decide:
|
||||
- **Straddle rule:** if `inRange != nil` and any line of the block is outside the range, keep the whole block verbatim.
|
||||
- **Preserve:** accumulate the full block text; if `cfg.ShouldPreserveComment` matches, keep the whole block verbatim.
|
||||
- **Remove:** keep code before the start marker on the first line and code after the end marker on the last line; drop interior lines. Start and end on the same line collapse to `prefix + suffix`. An unterminated block (no end by EOF) removes from the start marker to EOF.
|
||||
Verified coverage: 43 of 45 shush extensions map directly; `less` via the override; `conf` via fallback.
|
||||
|
||||
Line comments in git mode are gated per line: strip only if that line satisfies `inRange`.
|
||||
## Which tokens to drop
|
||||
|
||||
`--preserve-lines`: a line that becomes empty due to comment removal (including block interior lines) is emitted as its leading whitespace instead of being dropped.
|
||||
Drop tokens whose `Type.Category() == chroma.Comment`, **except** `chroma.CommentPreproc` and `chroma.CommentPreprocFile`.
|
||||
|
||||
This exclusion is mandatory and load-bearing: chroma classifies C/C++ preprocessor directives (`#include <stdio.h>`, `#define N 4`) as `CommentPreproc` / `CommentPreprocFile`, which live in the `Comment` category. Dropping the whole category deletes them and breaks the file. The spike confirmed both the bug (naive drop removed `#include`) and the fix (excluding preproc keeps them while still removing real comments).
|
||||
|
||||
Flag mapping:
|
||||
- default: drop line and block comments (`CommentSingle`, `CommentMultiline`, `CommentHashbang`, `CommentSpecial`), never preproc.
|
||||
- `--inline`: drop only non-multiline comment tokens (`CommentSingle`, `CommentHashbang`, `CommentSpecial`).
|
||||
- `--block`: drop only `CommentMultiline` (this also covers HTML/XML `<!-- -->`, which chroma tags `CommentMultiline`).
|
||||
|
||||
## Preservation, git range, and shush line semantics
|
||||
|
||||
Applied while walking the token stream, tracking the current line by counting newlines in token values:
|
||||
|
||||
- **Preserve patterns:** before dropping a comment token, check `cfg.ShouldPreserveComment(token.Value)`. For a multi-line block the token value is the whole block, so patterns match across it. On match, keep the token (action `"preserved"`).
|
||||
- **Git straddle rule:** a comment token covers `[startLine, endLine]`. Drop it only if `InRange` is nil or every covered line satisfies `InRange`. A block crossing the boundary is kept intact.
|
||||
- **Comment-only line:** if, after dropping, a line's remaining content is only whitespace and that line had a dropped comment, remove the line entirely (action `"removed"`). Distinguish from a pre-existing blank line (no dropped comment) which is kept.
|
||||
- **Inline comment:** stripped, code kept, trailing whitespace trimmed (action `"modified"`).
|
||||
- **`--preserve-lines`:** a comment-only line (including block interior lines) is emitted as its leading whitespace instead of being dropped.
|
||||
- **Multi-line block collapse:** chroma emits a block (with its interior newlines) as a single token; dropping it collapses the interior lines automatically, leaving at most a blank line where a trailing newline remains — handled by the comment-only-line rule.
|
||||
|
||||
## Safety net
|
||||
|
||||
chroma lexers are best-effort tokenizers. Guard against any lexer that fails to round-trip: if the concatenation of all tokens does not equal the input (`lossless == false`), **abort chroma for that file and fall back to the legacy remover** rather than risk emitting corrupted output. This makes non-lossless tokenization a safe degradation, not a data-loss bug.
|
||||
|
||||
## Edge-case semantics
|
||||
|
||||
| Input | Result | Reason |
|
||||
|-------|--------|--------|
|
||||
| `/* a // b */` | removed | `//` inside a block is not a line comment |
|
||||
| `/* a // b */` (multi-line) | removed | `//` inside a block is part of the block |
|
||||
| `x // a /* b` | `x` | `/*` inside a line comment is inert |
|
||||
| `x /* a */ // b` | `x` | block removed, then line comment removed |
|
||||
| `x /* a */ // b` | `x` | block then line comment removed |
|
||||
| `code /* c` / `body` / `*/ y()` | `code` / `y()` | multi-line block; boundary code kept |
|
||||
| `/* no end` / `more` | both removed | unterminated block runs to EOF |
|
||||
| `*/ x()` (no open) | unchanged | no open block; not a comment token |
|
||||
| `s = "/* not */"` | unchanged | marker inside a string is protected |
|
||||
| `/* no end` / `more` | both removed | unterminated block to EOF |
|
||||
| `#include <stdio.h>` | unchanged | preproc excluded from drop |
|
||||
| `` `a /* x */ b` `` (raw string) | unchanged | marker inside string token |
|
||||
| `s = "/* not */"` | unchanged | marker inside string token |
|
||||
|
||||
## Non-goals (v1)
|
||||
|
||||
- Multi-line strings spanning lines with comment markers inside.
|
||||
- Nested block comments (C-family block comments do not nest).
|
||||
- `--dry-run` preview line-number remapping beyond what `action` rendering needs.
|
||||
- Nested block comments (C-family block comments do not nest; chroma matches non-nested).
|
||||
- Content-based language detection (lexer is chosen by extension, matching `languageMap`).
|
||||
|
||||
## Testing (TDD)
|
||||
## Testing (TDD: red first, then green)
|
||||
|
||||
Sequence: write fixtures and failing tests, run and observe red, build the engine and rewire call sites, run and observe green, then `make check`.
|
||||
Sequence: add chroma dep; write fixtures and failing tests; run and observe red; implement `StripComments` and rewire the four call sites; run and observe green; `make check`.
|
||||
|
||||
- **Block, every extension (~30):** `testdata/multiline/block_<ext>.<ext>` plus `.golden`, for all 26 `/* */` and 4 `<!-- -->` extensions. Each fixture is a multi-line block with code before and after it.
|
||||
- **Mixed (representative — C, Go, CSS, HTML):** interleaved line and block comments in one file.
|
||||
- **Edge cases (representative):** one fixture per row of the edge-case table — marker-in-block, marker-in-line-comment, block-then-line, unterminated, stray close, string-protected, boundary code.
|
||||
- **Git straddle (JS):** a range fully inside a block is removed; a straddling block is kept. Uses `processFileWithLineRanges` with explicit `[]git.LineRange` (no real git).
|
||||
- **Regression:** every existing `TestWholeFile` single-line golden must stay green.
|
||||
- **Block, every extension (~30):** `testdata/multiline/block_<ext>.<ext>` + `.golden` for all 26 `/* */` extensions (js, ts, jsx, tsx, go, c, cpp, cc, cxx, h, hpp, java, cs, rs, swift, kt, kts, dart, scala, php, css, scss, sass, less, sql) and 4 `<!-- -->` extensions (html, htm, xml, svg). Each fixture: a multi-line block with code before and after.
|
||||
- **Preproc preservation (C, C++):** a fixture with `#include` / `#define` plus real comments; assert directives survive, comments go.
|
||||
- **String/edge correctness (must-pass, formerly non-goals):** Go raw string containing `/* */`; JS template literal spanning lines with `/*`; Rust lifetime + char literal + block.
|
||||
- **Mixed (representative — C, Go, CSS, HTML):** interleaved line and block comments.
|
||||
- **Edge table (representative):** one fixture per row of the edge-case table.
|
||||
- **Git straddle (JS):** range fully inside a block is removed; a straddling block is kept (via `processFileWithLineRanges` with explicit `[]git.LineRange`).
|
||||
- **Fallback (conf):** a `.conf` file with `#` comments strips correctly via the legacy path.
|
||||
- **Regression:** every existing `TestWholeFile` single-line golden stays green.
|
||||
|
||||
Driver reuses the existing golden harness pattern (copy input to a temp dir, run the mutate path, byte-compare against `.golden`), extended for the multi-line fixtures.
|
||||
Driver reuses the existing golden harness (copy input to a temp dir, run the mutate path, byte-compare `.golden`).
|
||||
|
||||
## Constraints
|
||||
|
||||
- No code comments anywhere (hard project rule).
|
||||
- Version control: jj only.
|
||||
- Go 1.24, module path `github.com/carlosarraes/shush`.
|
||||
- Gate: `make check` (fmt + vet + test) clean before every commit.
|
||||
- New dependency: `github.com/alecthomas/chroma/v2` (pure Go).
|
||||
- Gate: `make check` (fmt + vet + test) clean before every commit; static `CGO_ENABLED=0` build must keep working.
|
||||
|
||||
Reference in New Issue
Block a user