|
| 1 | +# Comment-after closing bracket design |
| 2 | + |
| 3 | +Branch: `comment-after-rebased` |
| 4 | +Issue: 1233 and related |
| 5 | + |
| 6 | +## Problem |
| 7 | + |
| 8 | +When a comment is attached as `ContentAfter` on the last item inside an indented block (list `[]`, record `{}`), the emit order causes the closing bracket to land at the wrong indentation. |
| 9 | + |
| 10 | +### Current emit order |
| 11 | + |
| 12 | +``` |
| 13 | +genExpr lastItem |
| 14 | + → enterNode / leaveNode emits ContentAfter(comment) |
| 15 | + → WriteLineBecauseOfTrivia |
| 16 | +→ UnIndentBy ← indent level drops here |
| 17 | +→ [lastWriteEventIsNewline? → true, so skip newline] |
| 18 | +→ Write "]" ← ends up at wrong indent |
| 19 | +``` |
| 20 | + |
| 21 | +`lastWriteEventIsNewline` (Context.fs:340) skips `UnIndentBy` when scanning backwards, finds the trivia newline, returns `true`. The caller skips emitting a newline before `]`. But the indent level changed, so `]` stays on the comment's indentation instead of getting its own line. |
| 22 | + |
| 23 | +### Example (list with Stroustrup style) |
| 24 | + |
| 25 | +Input: |
| 26 | +```fsharp |
| 27 | +let list = [ |
| 28 | + someItem |
| 29 | + // comment |
| 30 | +] |
| 31 | +``` |
| 32 | + |
| 33 | +Current output (wrong — `]` at column 4): |
| 34 | +```fsharp |
| 35 | +let list = [ |
| 36 | + someItem |
| 37 | + // comment |
| 38 | + ] |
| 39 | +``` |
| 40 | + |
| 41 | +Expected output (comment stays at content indent, `]` at column 0): |
| 42 | +```fsharp |
| 43 | +let list = [ |
| 44 | + someItem |
| 45 | + // comment |
| 46 | +] |
| 47 | +``` |
| 48 | + |
| 49 | +### All 4 call sites of `lastWriteEventIsNewline` have this pattern |
| 50 | + |
| 51 | +1. **CodePrinter.fs:1799** — record `}` after `indentSepNlnUnindent` |
| 52 | +2. **CodePrinter.fs:1834** — aligned record `}` closing brace |
| 53 | +3. **CodePrinter.fs:1904** — list/array `]` closing bracket (the failing test case) |
| 54 | +4. **Context.fs:584** — `sepNlnUnlessLastEventIsNewline` general helper |
| 55 | + |
| 56 | +## Approaches considered and rejected |
| 57 | + |
| 58 | +### 1. Flip unindent before ContentAfter |
| 59 | + |
| 60 | +Emit `unindent` before `leaveNode` so the comment prints at `]`'s indent level: |
| 61 | + |
| 62 | +``` |
| 63 | +genExprAux lastItem → UnIndentBy → ContentAfter(comment) → WriteLineBecauseOfTrivia → "]" |
| 64 | +``` |
| 65 | + |
| 66 | +**Rejected**: This shifts the comment from column 4 to column 0, changing its association from "after `someItem`" to "before `]`". The comment should stay at `someItem`'s indentation level for correct semantics and idempotency. |
| 67 | + |
| 68 | +### 2. Fix `lastWriteEventIsNewline` to track pending unindents |
| 69 | + |
| 70 | +Make `lastWriteEventIsNewline` return `false` when it crosses an `UnIndentBy` to find the newline. |
| 71 | + |
| 72 | +**Rejected**: This only prevents the extra newline from being skipped, but we still need a newline at the *new* indent level. The `WriteLineBecauseOfTrivia` already produced a newline at the *old* indent level. Adding another newline would produce a blank line. |
| 73 | + |
| 74 | +### 3. Unindent-aware `genTrivia` / flag on `CommentOnSingleLine` |
| 75 | + |
| 76 | +Embed an "also unindent" flag in `CommentOnSingleLine` trivia content, or teach `genTrivia`/`leaveNode` about the caller's indentation needs. |
| 77 | + |
| 78 | +**Rejected**: Mixes formatting concerns into the trivia data model. Breaks the clean separation between trivia content and formatting decisions. |
| 79 | + |
| 80 | +## Chosen approach: capture and splice writer events |
| 81 | + |
| 82 | +### Key insight |
| 83 | + |
| 84 | +The comment should stay at `someItem`'s indentation (column 4). The `UnIndentBy` needs to be spliced into the event stream *between* the `WriteComment` and the final `WriteLineBecauseOfTrivia`. This way: |
| 85 | +- The comment is emitted at the content's indent level (correct) |
| 86 | +- The unindent takes effect before the trailing newline |
| 87 | +- The trailing newline lands at the reduced indent level |
| 88 | +- `]` follows naturally at column 0 |
| 89 | + |
| 90 | +### Desired event stream |
| 91 | + |
| 92 | +``` |
| 93 | +Write "someItem" |
| 94 | +WriteLineBecauseOfTrivia ← newline before comment (at indent 4) |
| 95 | +WriteComment "// comment" ← comment at indent 4 ✓ |
| 96 | +UnIndentBy 4 ← spliced in here |
| 97 | +WriteLineBecauseOfTrivia ← newline now at indent 0 |
| 98 | +Write "]" ← at column 0 ✓ |
| 99 | +``` |
| 100 | + |
| 101 | +### Implementation: `captureTrailingTriviaEvents` |
| 102 | + |
| 103 | +A helper in CodePrinter.fs that runs `leaveNode` on a dummy context to capture the writer events it would produce, without applying them to the real context: |
| 104 | + |
| 105 | +```fsharp |
| 106 | +let captureTrailingTriviaEvents (node: Node) (currentCtx: Context) : WriterEvent list = |
| 107 | + let dummyCtx = { currentCtx with WriterModel = { currentCtx.WriterModel with Mode = Dummy }} |
| 108 | + let eventsBefore = dummyCtx.WriterEvents.Length |
| 109 | + let ctxAfter = leaveNode node dummyCtx |
| 110 | + let eventsAfter = ctxAfter.WriterEvents.Length |
| 111 | + let take = eventsAfter - eventsBefore |
| 112 | + ctxAfter.WriterEvents.Rev() |
| 113 | + |> Seq.take take |
| 114 | + |> Seq.toList |
| 115 | +``` |
| 116 | + |
| 117 | +The caller then splices `UnIndentBy` before the last `WriteLineBecauseOfTrivia` and applies the modified events to the real context: |
| 118 | + |
| 119 | +```fsharp |
| 120 | +let updatedEvents = |
| 121 | + let rec visit (continuation: WriterEvent list -> WriterEvent list) next = |
| 122 | + match next with |
| 123 | + | [] -> continuation [] |
| 124 | + | [ WriteComment c; WriteLineBecauseOfTrivia ] -> |
| 125 | + visit |
| 126 | + (fun current -> |
| 127 | + WriteComment c |
| 128 | + :: UnIndentBy ctx.Config.IndentSize |
| 129 | + :: WriteLineBecauseOfTrivia |
| 130 | + :: current |
| 131 | + |> continuation) |
| 132 | + [] |
| 133 | + | head :: rest -> visit (fun current -> (head :: current) |> continuation) rest |
| 134 | + visit id events |
| 135 | +
|
| 136 | +List.fold (fun acc event -> writerEvent event acc) ctx updatedEvents |
| 137 | +``` |
| 138 | + |
| 139 | +### Using `colWithLast` for last-item handling |
| 140 | + |
| 141 | +`colWithLast` (Context.fs) processes a list where the last item gets different treatment: |
| 142 | + |
| 143 | +```fsharp |
| 144 | +colWithLast |
| 145 | + genExpr // normal items |
| 146 | + sepNln // separator |
| 147 | + (fun lastExpr -> // last item: genExpr content + captured/spliced trivia events |
| 148 | + genExprContent lastExpr |
| 149 | + +> fun ctx -> |
| 150 | + let events = captureTrailingTriviaEvents (Expr.Node lastExpr) ctx |
| 151 | + let updatedEvents = spliceUnindent events ctx.Config.IndentSize |
| 152 | + List.fold (fun acc event -> writerEvent event acc) ctx updatedEvents) |
| 153 | + node.Elements |
| 154 | +``` |
| 155 | + |
| 156 | +### Gating |
| 157 | + |
| 158 | +Only use the special path when `HasContentAfterOfLastDescendant` is true on the container or last element. Normal path (no trailing trivia) stays unchanged. |
| 159 | + |
| 160 | +## What's already done on this branch |
| 161 | + |
| 162 | +- `ColMultilineItem` carries `Node` instead of pre-computed separator function (the `option` was removed since all callers pass a node) |
| 163 | +- `HasContentAfterOfLastDescendant` + `MarkContentAfterOfLastDescendant` added to `Node` interface in SyntaxOak.fs |
| 164 | +- Flag is set inline in `simpleTriviaToTriviaInstruction` (Trivia.fs) when `AddAfter` is called on a descendant |
| 165 | +- `findNodeBeforeWithMatchingColumn` in Trivia.fs for column-matching comment assignment (indented comments attach to preceding node at same column) |
| 166 | +- `colWithNlnWhenItemIsMultiline` computes `sepNlnItem` from `currentNode.HasContentBefore` instead of a pre-computed separator |
| 167 | +- `addFinalNewline` in CodePrinter.fs handles trailing blank lines from deeply nested ContentAfter |
| 168 | +- 4 new test cases in CommentTests.fs for issue 1233 |
| 169 | +- `lastDescendantHasContentAfter` removed from Context.fs (was dead code, replaced by the flag approach) |
| 170 | +- `colWithLast` + `foldExceptLast` helpers added in Context.fs |
| 171 | +- `captureTrailingTriviaEvents` helper added in CodePrinter.fs (prototype working for list case) |
| 172 | +- Prototype produces correct output for the `comment before closing list bracket, 3079` test case |
| 173 | + |
| 174 | +## Deeper problem: `unindent` and trailing trivia in `expressionExceedsPageWidth` |
| 175 | + |
| 176 | +### Discovery |
| 177 | + |
| 178 | +The `genLambdaAux` function (CodePrinter.fs:2047) has the same unindent-before-trivia problem, but it surfaces through a different path: `sepSpaceOrIndentAndNlnIfExpressionExceedsPageWidthUnlessStroustrup`. |
| 179 | + |
| 180 | +Call chain: |
| 181 | +1. `sepSpaceOrIndentAndNlnIfExpressionExceedsPageWidthUnlessStroustrup` (Context.fs:833) |
| 182 | +2. → `sepSpaceOrIndentAndNlnIfExceedsPageWidthUnlessStroustrup` (Context.fs:827) |
| 183 | +3. → `sepSpaceOrIndentAndNlnIfExpressionExceedsPageWidth` (Context.fs:789) |
| 184 | +4. → `expressionExceedsPageWidth` (Context.fs:741) with `beforeLong = indent +> sepNln` and `afterLong = unindent` |
| 185 | + |
| 186 | +Inside `expressionExceedsPageWidth`, the long-expression fallback is: |
| 187 | + |
| 188 | +```fsharp |
| 189 | +let fallbackExpression = beforeLong +> expr +> afterLong |
| 190 | +``` |
| 191 | + |
| 192 | +Which expands to: `indent +> sepNln +> expr +> unindent` |
| 193 | + |
| 194 | +The `unindent` runs *after* the expression, but trailing trivia (comments) attached to the last node inside `expr` may have already been flushed — or will be flushed after the unindent changes the indentation level. |
| 195 | + |
| 196 | +### This is systemic |
| 197 | + |
| 198 | +Every caller of `expressionExceedsPageWidth` that passes `unindent` as `afterLong` has this latent bug. The callers include: |
| 199 | +- `autoIndentAndNlnIfExpressionExceedsPageWidth` (Context.fs:780) |
| 200 | +- `sepSpaceOrIndentAndNlnIfExpressionExceedsPageWidth` (Context.fs:789) |
| 201 | +- `sepSpaceOrDoubleIndentAndNlnIfExpressionExceedsPageWidth` (Context.fs:798) |
| 202 | +- And transitively, `sepSpaceOrIndentAndNlnIfExpressionExceedsPageWidthUnlessStroustrup` (Context.fs:833) |
| 203 | + |
| 204 | +The pattern should conceptually be: |
| 205 | + |
| 206 | +``` |
| 207 | +indent +> sepNln +> expr +> [flush trailing trivia at current indent] +> unindent |
| 208 | +``` |
| 209 | + |
| 210 | +But today it is: |
| 211 | + |
| 212 | +``` |
| 213 | +indent +> sepNln +> expr +> unindent |
| 214 | +``` |
| 215 | + |
| 216 | +### Why `unindent` timing isn't the issue |
| 217 | + |
| 218 | +At first glance it looks like `UnIndentBy` fires too early. But actually, `IndentBy`/`UnIndentBy` only update `WriterModel.Indent` — the indent value is inert until the next `WriteLine`/`WriteLineBecauseOfTrivia`, which reads `m.Indent` to produce leading spaces (`String.replicate m.Indent " "` in `doNewline`). So the indent model itself is fine. |
| 219 | + |
| 220 | +The real problem is the **interaction between trivia-emitted newlines and `lastWriteEventIsNewline`**: |
| 221 | + |
| 222 | +1. `genExpr lastItem` runs → `leaveNode` emits `ContentAfter` trivia: `WriteComment "// comment"` + `WriteLineBecauseOfTrivia` (newline at current indent ✓) |
| 223 | +2. `unindent` fires → updates `m.Indent` (no visible effect yet) |
| 224 | +3. Caller checks `lastWriteEventIsNewline` → sees the trivia newline → returns `true` → **skips** emitting a newline before the closing bracket |
| 225 | +4. Closing bracket writes on the same line as the trivia newline, but at the **old** indent level (the trivia newline used the pre-unindent indent) |
| 226 | + |
| 227 | +If you force a newline anyway, you get a **blank line** (one from trivia, one forced). There's no good place to put `unindent` with the current emit order — before the trivia newline is wrong (comment at wrong indent), after is wrong (bracket at wrong indent), and adding an extra newline doubles up. |
| 228 | + |
| 229 | +### The fix pattern (from the array/list case) |
| 230 | + |
| 231 | +The last commit (`4279cbd`) solved this for `genArrayOrList` using `captureTrailingTriviaEvents` + `insertUnindent`: |
| 232 | + |
| 233 | +1. **Release** `ContentAfter` from the last node before `genExpr` runs (`node.ReleaseContentAfter()`) |
| 234 | +2. **Run** `genExpr` — which now skips the trivia since it's been released |
| 235 | +3. **Capture** the trivia events on a dummy context (`captureTrailingTriviaEvents`) |
| 236 | +4. **Splice** `UnIndentBy` between the comment and its trailing newline (`insertUnindent`) |
| 237 | +5. **Replay** the modified events onto the real context |
| 238 | + |
| 239 | +This gives exactly one newline at the correct (reduced) indent level. |
| 240 | + |
| 241 | +### The `expressionExceedsPageWidth` problem |
| 242 | + |
| 243 | +The same pattern needs to happen inside `expressionExceedsPageWidth` (Context.fs:741) and its 45 call sites in CodePrinter.fs. The long-expression fallback is: |
| 244 | + |
| 245 | +```fsharp |
| 246 | +let fallbackExpression = beforeLong +> expr +> afterLong |
| 247 | +``` |
| 248 | + |
| 249 | +Where `afterLong = unindent`. The `unindent` needs to be spliced into the trivia events, not run after them. But `expressionExceedsPageWidth` doesn't have a handle on the node whose trivia needs releasing — it only receives an opaque `expr: Context -> Context` function. |
| 250 | + |
| 251 | +### Implications |
| 252 | + |
| 253 | +Weaving the fix into `expressionExceedsPageWidth` itself would fix all 45 call sites at once, but the abstraction doesn't have enough information: it doesn't know which node to call `ReleaseContentAfter()` on. The `expr` function has already closed over the node. |
| 254 | + |
| 255 | +Possible directions: |
| 256 | +- **Option A**: Change `expr` to cooperate — e.g., `expr` releases its own trivia and returns captured events alongside the context, so `expressionExceedsPageWidth` can splice before applying `afterLong` |
| 257 | +- **Option B**: Give `expressionExceedsPageWidth` a node parameter so it can do the release/capture/splice itself |
| 258 | +- **Option C**: Accept the whack-a-mole approach — fix each call site individually, using the same release/capture/splice pattern as `genArrayOrList` |
| 259 | +- **Option D**: Rethink more fundamentally — e.g., make `unindent` trivia-aware so it defers past pending trivia events, or change how `leaveNode` emits trailing trivia so the caller retains control |
| 260 | + |
| 261 | +Option A or B would require changing the signature of `expressionExceedsPageWidth` and all its wrappers. Option C is pragmatic but fragile (45 potential sites). Option D is the cleanest but the biggest change. |
| 262 | + |
| 263 | +## Still TODO |
| 264 | + |
| 265 | +- Replace hardcoded `!-"TODO"` / `!-"someItem"` with real `genExpr` / `genExprAux` (a `genExpr` variant without `genNode` wrapper) |
| 266 | +- The splice `visit` pattern only handles `CommentOnSingleLine` — needs to cover `BlockComment`, `Directive` too |
| 267 | +- Gate on `HasContentAfterOfLastDescendant` so normal path is unchanged |
| 268 | +- Generalize across the other 3 `lastWriteEventIsNewline` call sites (records `{}`, etc.) |
| 269 | +- Triage the 16 failing tests: which are regressions vs pre-existing |
| 270 | +- Clean up temporary debug code (hardcoded strings, user's `+` experiment) |
| 271 | + |
| 272 | +## Notes |
| 273 | + |
| 274 | +- The `shared.fsx` editorconfig parser was fixed: commas → newlines, glob covers `*.{fs,fsx,fsi}` |
| 275 | +- Writer events script (`scripts/writer-events.fsx`) and Oak script (`scripts/oak.fsx`) are useful for diagnosing these issues |
0 commit comments