Skip to content

Commit 93c7172

Browse files
CopilotRyanCavanaugh
Copilot
and
RyanCavanaugh
committed
Improve JSX comment duplication fix - use targeted node traversal instead of expensive pre-walk
Co-authored-by: RyanCavanaugh <6685088+RyanCavanaugh@users.noreply.github.com>
1 parent 381423f commit 93c7172

13 files changed

Lines changed: 180 additions & 22 deletions

‎src/compiler/emitter.ts‎

Lines changed: 42 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1253,8 +1253,7 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
12531253
var detachedCommentsInfo: { nodePos: number; detachedCommentEndPos: number; }[] | undefined;
12541254
var hasWrittenComment = false;
12551255
var commentsDisabled = !!printerOptions.removeComments;
1256-
// Track JSX text ranges to prevent them from being treated as comments
1257-
var jsxTextRanges: { start: number; end: number; }[] = [];
1256+
var jsxTextPositionCache: Map<number, boolean> | undefined;
12581257
var lastSubstitution: Node | undefined;
12591258
var currentParenthesizerRule: ParenthesizerRule<any> | undefined;
12601259
var { enter: enterComment, exit: exitComment } = performance.createTimerIf(extendedDiagnostics, "commentTime", "beforeComment", "afterComment");
@@ -1389,22 +1388,13 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
13891388
currentSourceFile = sourceFile;
13901389
currentLineMap = undefined;
13911390
detachedCommentsInfo = undefined;
1392-
jsxTextRanges = []; // Clear JSX text ranges for new source file
1391+
jsxTextPositionCache = undefined; // Clear cache for new source file
13931392

1394-
// Pre-collect all JSX text ranges before emission starts
13951393
if (sourceFile) {
1396-
collectJsxTextRanges(sourceFile);
13971394
setSourceMapSource(sourceFile);
13981395
}
13991396
}
14001397

1401-
function collectJsxTextRanges(node: Node) {
1402-
if (node.kind === SyntaxKind.JsxText) {
1403-
jsxTextRanges.push({ start: node.pos, end: node.end });
1404-
}
1405-
forEachChild(node, collectJsxTextRanges);
1406-
}
1407-
14081398
function setWriter(_writer: EmitTextWriter | undefined, _sourceMapGenerator: SourceMapGenerator | undefined) {
14091399
if (_writer && printerOptions.omitTrailingSemicolon) {
14101400
_writer = getTrailingSemicolonDeferringWriter(_writer);
@@ -1415,6 +1405,40 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
14151405
sourceMapsDisabled = !writer || !sourceMapGenerator;
14161406
}
14171407

1408+
function isPositionInJsxText(pos: number): boolean {
1409+
if (!currentSourceFile) return false;
1410+
1411+
// Use cache if available
1412+
if (!jsxTextPositionCache) {
1413+
jsxTextPositionCache = new Map();
1414+
}
1415+
1416+
if (jsxTextPositionCache.has(pos)) {
1417+
return jsxTextPositionCache.get(pos)!;
1418+
}
1419+
1420+
// Walk the AST to find if this position is within a JSX text node
1421+
let found = false;
1422+
1423+
function visitNode(node: Node): void {
1424+
if (found) return;
1425+
1426+
if (node.kind === SyntaxKind.JsxText && pos >= node.pos && pos < node.end) {
1427+
found = true;
1428+
return;
1429+
}
1430+
1431+
// Only continue traversing if this node might contain the position
1432+
if (pos >= node.pos && pos < node.end) {
1433+
forEachChild(node, visitNode);
1434+
}
1435+
}
1436+
1437+
visitNode(currentSourceFile);
1438+
jsxTextPositionCache.set(pos, found);
1439+
return found;
1440+
}
1441+
14181442
function reset() {
14191443
nodeIdToGeneratedName = [];
14201444
nodeIdToGeneratedPrivateName = [];
@@ -6110,9 +6134,8 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
61106134
}
61116135
else {
61126136
forEachLeadingCommentRange(currentSourceFile.text, pos, (commentPos, commentEnd, kind, hasTrailingNewLine, rangePos) => {
6113-
// Check if this comment position falls within any JSX text range
6114-
const isWithinJsxText = jsxTextRanges.some(range => commentPos >= range.start && commentEnd <= range.end);
6115-
if (!isWithinJsxText) {
6137+
// Skip comments that are within JSX text nodes
6138+
if (!isPositionInJsxText(commentPos)) {
61166139
cb(commentPos, commentEnd, kind, hasTrailingNewLine, rangePos);
61176140
}
61186141
}, /*state*/ pos);
@@ -6124,9 +6147,8 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
61246147
// Emit the trailing comments only if the container's end doesn't match because the container should take care of emitting these comments
61256148
if (currentSourceFile && (containerEnd === -1 || (end !== containerEnd && end !== declarationListContainerEnd))) {
61266149
forEachTrailingCommentRange(currentSourceFile.text, end, (commentPos, commentEnd, kind, hasTrailingNewLine) => {
6127-
// Check if this comment position falls within any JSX text range
6128-
const isWithinJsxText = jsxTextRanges.some(range => commentPos >= range.start && commentEnd <= range.end);
6129-
if (!isWithinJsxText) {
6150+
// Skip comments that are within JSX text nodes
6151+
if (!isPositionInJsxText(commentPos)) {
61306152
cb(commentPos, commentEnd, kind, hasTrailingNewLine);
61316153
}
61326154
});
@@ -6149,9 +6171,8 @@ export function createPrinter(printerOptions: PrinterOptions = {}, handlers: Pri
61496171
}
61506172

61516173
forEachLeadingCommentRange(currentSourceFile.text, pos, (commentPos, commentEnd, kind, hasTrailingNewLine, rangePos) => {
6152-
// Check if this comment position falls within any JSX text range
6153-
const isWithinJsxText = jsxTextRanges.some(range => commentPos >= range.start && commentEnd <= range.end);
6154-
if (!isWithinJsxText) {
6174+
// Skip comments that are within JSX text nodes
6175+
if (!isPositionInJsxText(commentPos)) {
61556176
cb(commentPos, commentEnd, kind, hasTrailingNewLine, rangePos);
61566177
}
61576178
}, /*state*/ pos);

‎tests/baselines/reference/jsxCommentDuplication(jsx=preserve).js‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
//// [tests/cases/compiler/jsxCommentDuplication.tsx] ////
2+
3+
//// [jsxCommentDuplication.tsx]
4+
function App() {}
5+
const jsx = <App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App>;
6+
7+
//// [jsxCommentDuplication.jsx]
8+
function App() { }
9+
var jsx = <App>/* no */{/* 1 */123 /* 2 */}/* no */</App>;

‎tests/baselines/reference/jsxCommentDuplication(jsx=preserve).symbols‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
//// [tests/cases/compiler/jsxCommentDuplication.tsx] ////
2+
3+
=== jsxCommentDuplication.tsx ===
4+
function App() {}
5+
>App : Symbol(App, Decl(jsxCommentDuplication.tsx, 0, 0))
6+
7+
const jsx = <App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App>;
8+
>jsx : Symbol(jsx, Decl(jsxCommentDuplication.tsx, 1, 5))
9+
>App : Symbol(App, Decl(jsxCommentDuplication.tsx, 0, 0))
10+
>App : Symbol(App, Decl(jsxCommentDuplication.tsx, 0, 0))
11+

‎tests/baselines/reference/jsxCommentDuplication(jsx=preserve).types‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
//// [tests/cases/compiler/jsxCommentDuplication.tsx] ////
2+
3+
=== jsxCommentDuplication.tsx ===
4+
function App() {}
5+
>App : () => void
6+
> : ^^^^^^^^^^
7+
8+
const jsx = <App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App>;
9+
>jsx : error
10+
><App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App> : error
11+
>App : () => void
12+
> : ^^^^^^^^^^
13+
>123 : 123
14+
> : ^^^
15+
>App : () => void
16+
> : ^^^^^^^^^^
17+

‎tests/baselines/reference/jsxCommentDuplication(jsx=react).errors.txt‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
jsxCommentDuplication.tsx(2,14): error TS2874: This JSX tag requires 'React' to be in scope, but it could not be found.
2+
3+
4+
==== jsxCommentDuplication.tsx (1 errors) ====
5+
function App() {}
6+
const jsx = <App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App>;
7+
~~~
8+
!!! error TS2874: This JSX tag requires 'React' to be in scope, but it could not be found.

‎tests/baselines/reference/jsxCommentDuplication(jsx=react).js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
//// [tests/cases/compiler/jsxCommentDuplication.tsx] ////
2+
3+
//// [jsxCommentDuplication.tsx]
4+
function App() {}
5+
const jsx = <App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App>;
6+
7+
//// [jsxCommentDuplication.js]
8+
function App() { }
9+
var jsx = React.createElement(App, null,
10+
"/* no */", /* 1 */
11+
123 /* 2 */,
12+
"/* no */");

‎tests/baselines/reference/jsxCommentDuplication(jsx=react).symbols‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
//// [tests/cases/compiler/jsxCommentDuplication.tsx] ////
2+
3+
=== jsxCommentDuplication.tsx ===
4+
function App() {}
5+
>App : Symbol(App, Decl(jsxCommentDuplication.tsx, 0, 0))
6+
7+
const jsx = <App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App>;
8+
>jsx : Symbol(jsx, Decl(jsxCommentDuplication.tsx, 1, 5))
9+
>App : Symbol(App, Decl(jsxCommentDuplication.tsx, 0, 0))
10+
>App : Symbol(App, Decl(jsxCommentDuplication.tsx, 0, 0))
11+

‎tests/baselines/reference/jsxCommentDuplication(jsx=react).types‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
//// [tests/cases/compiler/jsxCommentDuplication.tsx] ////
2+
3+
=== jsxCommentDuplication.tsx ===
4+
function App() {}
5+
>App : () => void
6+
> : ^^^^^^^^^^
7+
8+
const jsx = <App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App>;
9+
>jsx : any
10+
> : ^^^
11+
><App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App> : any
12+
> : ^^^
13+
>App : () => void
14+
> : ^^^^^^^^^^
15+
>123 : 123
16+
> : ^^^
17+
>App : () => void
18+
> : ^^^^^^^^^^
19+

‎tests/baselines/reference/jsxCommentDuplication(jsx=react-jsx).errors.txt‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
jsxCommentDuplication.tsx(2,13): error TS2875: This JSX tag requires the module path 'react/jsx-runtime' to exist, but none could be found. Make sure you have types for the appropriate package installed.
2+
3+
4+
==== jsxCommentDuplication.tsx (1 errors) ====
5+
function App() {}
6+
const jsx = <App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App>;
7+
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
8+
!!! error TS2875: This JSX tag requires the module path 'react/jsx-runtime' to exist, but none could be found. Make sure you have types for the appropriate package installed.

‎tests/baselines/reference/jsxCommentDuplication(jsx=react-jsx).js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
//// [tests/cases/compiler/jsxCommentDuplication.tsx] ////
2+
3+
//// [jsxCommentDuplication.tsx]
4+
function App() {}
5+
const jsx = <App>/* no */{/* 1 */ 123 /* 2 */}/* no */</App>;
6+
7+
//// [jsxCommentDuplication.js]
8+
"use strict";
9+
Object.defineProperty(exports, "__esModule", { value: true });
10+
var jsx_runtime_1 = require("react/jsx-runtime");
11+
function App() { }
12+
var jsx = (0, jsx_runtime_1.jsxs)(App, { children: ["/* no */", /* 1 */ 123 /* 2 */, "/* no */"] });

0 commit comments

Comments
 (0)