Fix JSDoc comment of elided import being preserved in declaration emit#3963
Fix JSDoc comment of elided import being preserved in declaration emit#3963Copilot wants to merge 4 commits into
Conversation
Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/69748b26-6b50-4058-807a-6481c138b4af Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
When OnlyPrintJSDocStyle is true (declaration emit), the shouldWriteComment filter was applied during detached comment detection, causing non-JSDoc comments to be excluded from gap calculations. This made preceding JSDoc comments incorrectly appear "detached" and get emitted. The fix aligns with the TypeScript reference: collect ALL comments for gap detection, then filter by shouldWriteComment only when emitting. Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/69748b26-6b50-4058-807a-6481c138b4af Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a declaration emit bug where a JSDoc comment on an elided import could incorrectly attach to the next declaration if a non-JSDoc comment appeared between them.
Changes:
- Added a compiler test case reproducing the JSDoc “leak” onto the next declaration in
.d.tsoutput. - Updated detached-comment handling to include all comments for gap detection and filter by
shouldWriteCommentonly at emission time.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| testdata/tests/cases/compiler/jsDocCommentOfElidedImportPreserved.ts | Adds a regression test input demonstrating the incorrect JSDoc attachment scenario. |
| testdata/baselines/reference/compiler/jsDocCommentOfElidedImportPreserved.types | Adds the expected type baseline for the new test case. |
| testdata/baselines/reference/compiler/jsDocCommentOfElidedImportPreserved.symbols | Adds the expected symbol baseline for the new test case. |
| testdata/baselines/reference/compiler/jsDocCommentOfElidedImportPreserved.js | Adds the expected JS/.d.ts baseline proving JSDoc no longer appears on foo. |
| internal/printer/printer.go | Adjusts detached comment detection vs emission filtering to prevent JSDoc from being misclassified as detached. |
| } | ||
| result = detachedCommentsInfo{nodePos: textRange.Pos(), detachedCommentEndPos: core.LastOrNil(detachedComments).End()} | ||
| hasResult = true |
There was a problem hiding this comment.
Investigated and the current behavior is correct. The detachedCommentEndPos must cover all detached comments (not just the ones that pass shouldWriteComment), because it's used in emitLeadingComments (line 5497) to advance pos past already-processed comments. If we limited it to only emitted comments, non-JSDoc comments like /* trigger */ that are part of the detached block but filtered out would still be visible to emitLeadingComments and could be re-emitted by a different code path.
This matches the TypeScript reference implementation (utilities.ts:7069) which uses last(detachedComments).end - i.e., the end of ALL detached comments regardless of any filter.
I've added test cases in c1d7dc3 that specifically validate this: detachedCopyrightNonJSDoc.d.ts has a non-JSDoc detached comment that should be fully skipped in .d.ts output. If detachedCommentEndPos were based on commentsToEmit (which would be empty), the comment would leak through - but it doesn't because the end pos correctly covers it.
Added additional test cases to validate: - JSDoc detached copyright headers are preserved in .d.ts - Non-JSDoc detached comments are correctly skipped in .d.ts - detachedCommentEndPos correctly covers all comments (not just emitted ones) Agent-Logs-Url: https://github.com/microsoft/typescript-go/sessions/cb11d299-5553-414d-ad54-7812a59df90a Co-authored-by: jakebailey <5341706+jakebailey@users.noreply.github.com>
emitDetachedCommentsfunctiondetachedCommentEndPos- confirmed current behavior is correct