ReviewOS
Reload

also looking at this

stacks/dtsx

fix(scanner): end a statement at the newline after a trailing line comment

#3117
Open mkantautas wants to merge fix/trailing-line-comment into main
2 files +160 -13

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

packages/dtsx/src/extractor/scanner.tsmodified+92-13
@@ -37,6 +37,16 @@ const CH_AT = 64
3737const CH_DOLLAR = 36
3838const CH_UNDERSCORE = 95
3939
40// A declaration keyword and the name it declares, in the shape only a
41// declaration has, on one line. See `startsDeclaration`. A name is never an
42// operator word (`as // note` then `const satisfies T` continues), and a
43// keyword never runs on into an escape (`export\u0056alue` is an identifier).
44const DECL_NAME = String.raw`(?!(?:as|satisfies|in|of|instanceof|extends|is|keyof|typeof)(?![\p{L}\p{N}_$\\]))[\p{L}_$][\p{L}\p{N}_$]*`
45const DECL_END = String.raw`(?![\p{L}\p{N}_$\\])`
46const DECLARATION_START_RE = new RegExp(String.raw`^(?:(?:export|enum|var)${DECL_END}|import[ \t]+[\p{L}_$*{'"]|(?:const|let)[ \t]+${DECL_NAME}${DECL_END}|declare[ \t]+(?:const|let|var|function|class|enum|type|interface|namespace|module|global|abstract|async)${DECL_END}|abstract[ \t]+class${DECL_END}|interface[ \t]+${DECL_NAME}[ \t]*(?:[{<]|extends${DECL_END})|(?:namespace|module)[ \t]+${DECL_NAME}(?:\.${DECL_NAME})*[ \t]*\{|module[ \t]+['"]|type[ \t]+${DECL_NAME}[ \t]*=)`, 'u')
47// A line break other than LF or CRLF. See `hasOtherLineBreak`.
48const OTHER_LINE_BREAK_RE = /\r(?!\n)|[\u2028\u2029]/
49
4050function isWhitespace(ch: number): boolean {
4151 return ch === CH_SPACE || ch === CH_TAB || ch === CH_LF || ch === CH_CR
4252}
@@ -128,6 +138,14 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
128138 const nonExportedTypes = new Map<string, Declaration>()
129139 const funcBodyIndices = new Set<number>()
130140 let pos = 0
141 // The last line comment `skipNonCode` stepped over: its `//` and the newline
142 // that ends it. See `codeEndBefore`.
143 let skippedCommentStart = -1
144 let skippedCommentEnd = -1
145 // Where the run of comments it belongs to starts, with only whitespace
146 // between them, and where the last comment skipped ended. See `noteComment`.
147 let commentRunStart = -1
148 let lastCommentEnd = -1
131149
132150 // Skip BOM (byte order mark)
133151 if (pos < len && source.charCodeAt(pos) === 0xFEFF)
@@ -340,15 +358,23 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
340358 if (ch === CH_SLASH && pos + 1 < len) {
341359 const next = source.charCodeAt(pos + 1)
342360 if (next === CH_SLASH) {
343 // Line comment — use native indexOf for speed
361 // Line comment – stop AT its newline, not past it. That newline can end
362 // the statement the comment trails, and the ASI checks only run on a
363 // newline: stepping over it read `export const a = 1 // note` on into
364 // the next line, which dropped that declaration and typed `a` unknown.
344365 const nl = source.indexOf('\n', pos + 2)
345 pos = nl === -1 ? len : nl + 1
366 const end = nl === -1 ? len : nl
367 noteComment(end)
368 skippedCommentStart = pos
369 pos = skippedCommentEnd = end
346370 return true
347371 }
348372 if (next === CH_STAR) {
349 // Block comment — use native indexOf for speed
350 const end = source.indexOf('*/', pos + 2)
351 pos = end === -1 ? len : end + 2
373 // Block comment – use native indexOf for speed
374 const close = source.indexOf('*/', pos + 2)
375 const end = close === -1 ? len : close + 2
376 noteComment(end)
377 pos = end
352378 return true
353379 }
354380 // Check for regex literal: / not followed by / or *, preceded by non-expression char
@@ -409,6 +435,47 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
409435 }
410436 }
411437
438 /**
439 * Whether the source from the last skipped `//` up to `end` holds a lone CR,
440 * U+2028 or U+2029. Comments are read to the next LF, so code after such a
441 * break passes for comment text; that newline is then left as it was.
442 */
443 function hasOtherLineBreak(end: number): boolean {
444 return OTHER_LINE_BREAK_RE.test(source.slice(skippedCommentStart, end))
445 }
446
447 /** Record a comment from `pos` to `end`, extending the run before it if only whitespace separates them. */
448 function noteComment(end: number): void {
449 let gap = lastCommentEnd
450 while (gap >= 0 && gap < pos && /\s/.test(source[gap])) gap++
451 if (gap !== pos)
452 commentRunStart = pos
453 lastCommentEnd = end
454 }
455
456 /**
457 * `end`, moved back to the start of a line comment that runs up to it, so a
458 * trailing `// note` stays out of the code read before it.
459 */
460 function codeEndBefore(end: number): number {
461 // `end + 1` takes the LF in, so a CRLF's CR is not read as a lone one.
462 return end === skippedCommentEnd && !hasOtherLineBreak(end + 1) ? skippedCommentStart : end
463 }
464
465 /**
466 * Whether a declaration starts at `pos`, the line after a skipped comment:
467 * its keyword followed by the name it declares, which no expression or type
468 * runs on into, unless the comment follows a `.` (`api. // note` then
469 * `export()`). `function` and `class` are left out: either can be an operand
470 * (`void // note` then `function f() {}`, a decorated class expression).
471 */
472 function startsDeclaration(): boolean {
473 let before = commentRunStart - 1
474 while (before >= 0 && /\s/.test(source[before])) before--
475 const lineEnd = source.indexOf('\n', pos)
476 return source.charCodeAt(before) !== CH_DOT && DECLARATION_START_RE.test(source.slice(pos, lineEnd === -1 ? len : lineEnd))
477 }
478
412479 /** Index of the `//` that comments out `upTo` on this line, or -1. */
413480 function lineCommentStart(lineStart: number, upTo: number): number {
414481 let quote = 0
@@ -574,7 +641,13 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
574641 }
575642 break
576643 }
577 const result = pos >= len || isTopLevelKeyword() || source.charCodeAt(pos) === CH_RBRACE
644 let result = pos >= len || isTopLevelKeyword() || source.charCodeAt(pos) === CH_RBRACE
645 // The newline of a trailing `// note` used to be stepped over, so none of
646 // the ASI rules was written for it. It ends the statement only here, before
647 // a `}`, the end of the file or a declaration; the other checks read on
648 // past it, as they did.
649 if (result && saved === skippedCommentEnd)
650 result = !hasOtherLineBreak(pos) && (pos >= len || source.charCodeAt(pos) === CH_RBRACE || startsDeclaration())
578651 pos = saved
579652 return result
580653 }
@@ -648,6 +721,8 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
648721 const ch = source.charCodeAt(pos)
649722 if (ch !== CH_LF && ch !== CH_CR)
650723 return false
724 if (pos === skippedCommentEnd)
725 return false // see checkASITopLevel
651726 if (endsMidExpression(pos))
652727 return false
653728 const saved = pos
@@ -702,6 +777,8 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
702777 const ch = source.charCodeAt(pos)
703778 if (ch !== CH_LF && ch !== CH_CR)
704779 return false
780 if (pos === skippedCommentEnd)
781 return false // see checkASITopLevel
705782 if (endsMidExpression(pos))
706783 return false
707784 const saved = pos
@@ -741,6 +818,8 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
741818 const ch = source.charCodeAt(pos)
742819 if (ch !== CH_LF && ch !== CH_CR)
743820 return false
821 if (pos === skippedCommentEnd)
822 return false // see checkASITopLevel
744823 const saved = pos
745824 pos++
746825 if (ch === CH_CR && pos < len && source.charCodeAt(pos) === CH_LF)
@@ -2289,7 +2368,7 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
22892368 break
22902369 pos++
22912370 }
2292 typeAnnotation = sliceTrimmed(typeStart, pos)
2371 typeAnnotation = sliceTrimmed(typeStart, codeEndBefore(pos))
22932372 }
22942373
22952374 // Initializer
@@ -2364,7 +2443,7 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
23642443 }
23652444 break
23662445 }
2367 initializerText = sliceTrimmed(initStart, pos)
2446 initializerText = sliceTrimmed(initStart, codeEndBefore(pos))
23682447 if (initializerText.endsWith(' as const') || initializerText === 'const') {
23692448 isAsConst = true
23702449 if (!typeAnnotation) {
@@ -2546,7 +2625,7 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
25462625 break
25472626 pos++
25482627 }
2549 const typeBody = sliceTrimmed(typeStart, pos)
2628 const typeBody = sliceTrimmed(typeStart, codeEndBefore(pos))
25502629 if (pos < len && source.charCodeAt(pos) === CH_SEMI)
25512630 pos++
25522631
@@ -3321,7 +3400,7 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
33213400 break
33223401 pos++
33233402 }
3324 vtype = sliceTrimmed(ts, pos)
3403 vtype = sliceTrimmed(ts, codeEndBefore(pos))
33253404 }
33263405
33273406 let initText = ''
@@ -3346,7 +3425,7 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
33463425 break
33473426 pos++
33483427 }
3349 initText = sliceTrimmed(is2, pos)
3428 initText = sliceTrimmed(is2, codeEndBefore(pos))
33503429 }
33513430 if (pos < len && source.charCodeAt(pos) === CH_SEMI)
33523431 pos++
@@ -3431,7 +3510,7 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
34313510 break
34323511 pos++
34333512 }
3434 const typeBody = sliceTrimmed(ts, pos)
3513 const typeBody = sliceTrimmed(ts, codeEndBefore(pos))
34353514 if (pos < len && source.charCodeAt(pos) === CH_SEMI)
34363515 pos++
34373516 lines.push(`${indent}${prefix}type ${tname}${generics} = ${typeBody}`)
@@ -3606,7 +3685,7 @@ function scanDeclarationsInternal(_source: string, _filename: string, _keepComme
36063685 /** Convert a default-export expression into an ambient binding plus export. */
36073686 function extractDefaultExpression(stmtStart: number, expressionStart: number): void {
36083687 skipToStatementEnd()
3609 let expression = sliceTrimmed(expressionStart, pos)
3688 let expression = sliceTrimmed(expressionStart, codeEndBefore(pos))
36103689 if (expression.charCodeAt(expression.length - 1) === CH_SEMI) expression = expression.slice(0, -1).trimEnd()
36113690 if (!expression) return
36123691
packages/dtsx/test/type-asi.test.tsmodified+68-0
@@ -117,3 +117,71 @@ export function go(): void {}
117117 expect(processCode(code)).toContain(expected)
118118 })
119119})
120
121/**
122 * A trailing line comment hid the newline that ends the statement, in every read
123 * loop above:
124 *
125 * export const SLASH_RE = /\//g // %2F
126 * export const PLUS_RE = /\+/g // %2B
127 *
128 * Skipping the comment also stepped over its newline, so the ASI checks never ran
129 * there and the next line was read into the initializer: `SLASH_RE` came out
130 * `unknown` and `PLUS_RE` was missing. After a type annotation or a type alias
131 * the next statement leaked into the `.d.ts` instead.
132 *
133 * Surfaced from vue-router, whose `encoding.ts` exports the two constants above.
134 */
135describe('a line comment before the newline', () => {
136 it.each([
137 ['initializer', 'export const a = 1 // note\nexport const z = 3\n', 'const a: 1;'],
138 ['initializer with CRLF', 'export const a = 1 // note\r\nexport const z = 3\r\n', 'const a: 1;'],
139 ['local declaration next', 'export const a = 1 // note\nconst z = 3\nexport { z }\n', 'const a: 1;'],
140 ['type annotation', 'export let a: number // note\nexport const z = 3\n', 'let a: number;'],
141 ['type alias', 'export type A = string // note\nexport const z = 3\n', 'type A = string;'],
142 ['type ending in void', 'export type F = () => void // note\nexport const z = 3\n', 'type F = () => void;'],
143 ['namespace initializer', 'export namespace N {\n export const a = 1 // note\n export const z = 3\n}\n', 'const a: 1;'],
144 ['namespace annotation', 'export namespace N {\n export let a: number // note\n export const z = 3\n}\n', 'let a: number;'],
145 ['namespace type alias', 'export namespace N {\n export type A = string // note\n export const z = 3\n}\n', 'type A = string'],
146 ['default export', 'const a = 1\nexport default a // note\nexport const z = 3\n', 'export default a;'],
147 ])('ends the statement: %s', (_label, code, expected) => {
148 const dts = processCode(code)
149 expect(dts).toContain(expected)
150 expect(dts).not.toContain('note')
151 expect(dts).toMatch(/const z: 3;/)
152 })
153
154 it('ends an initializer at the end of the file', () => {
155 expect(processCode('export const a = \'x\' // note')).toContain('const a: \'x\';')
156 })
157
158 // The comment ends a statement only before a declaration, `}` or the end of
159 // the file. A statement that continues on the next line reads on, as before.
160 it.each([
161 ['operator before the comment', 'export let a: string | // note\n number\nexport const z = 3\n', /let a: string \|[^;]*number;/],
162 ['comment lines inside a type', 'export type A<T> = T extends string\n ? // note\n // more\n number\n : boolean\n', /\? \/\/ note[^;]*: boolean/],
163 ['return type', 'export function f(): string | // note\n number { return 1 }\n', /f\(\): string \|[^;]*number;/],
164 ['class member type', 'export class C {\n a: string | // note\n number\n}\n', /a: string \|[^;]*number;/],
165 ['keyword before the comment', 'export type A = typeof // note\n import(\'./dep\')\nexport const z = 3\n', /typeof[^;]*import\('\.\/dep'\);/],
166 ['call on the next line', 'export const a = (() => 1) // note\n () as number\nexport const z = 3\n', /const a: number;/],
167 ['void in a value', 'export const a = void // note\n function f() {} as undefined\nexport const z = 3\n', /const a: undefined;/],
168 ['decorator before the comment', 'export const a = @dec() // note\n class {} as { new(): object }\nexport const z = 3\n', /const a: \{ new\(\): object \};/],
169 ['as const across the comment', 'export const a = { x: 1 } as // note\n const satisfies { x: number }\nexport const z = 3\n', /const a: \{ x: number \};/],
170 ['as const with the operator far along the line', `export const a = { x: 1 } as // note\n const${' '.repeat(200)}satisfies { x: number }\nexport const z = 3\n`, /const a: \{ x: number \};/],
171 ['property named like a keyword', 'declare const api: { export(): number }\nexport const a = api. // note\n // more\n export() as number\nexport const z = 3\n', /const a: number;/],
172 ['variable named like a keyword', 'const namespace = \'app\'\nexport const a = false ? \'\' : // note\n namespace as string\nexport const z = 3\n', /const a: string;/],
173 ])('keeps a statement that continues: %s', (_label, code, expected) => {
174 expect(processCode(code)).toMatch(expected)
175 })
176
177 // `//` inside a literal is not a comment.
178 it.each([
179 ['regex', 'export const a = /[//]/g as RegExp\na.test(\'/\') as boolean\n', 'const a: RegExp;'],
180 ['template', 'export type A = `Use:\n// example`\nconsole.log(\'loaded\')\nexport const z = 3\n', 'const z: 3;'],
181 ])('is not fooled by // in a %s', (_label, code, expected) => {
182 const dts = processCode(code)
183 expect(dts).toContain(expected)
184 expect(dts).not.toContain('console.log')
185 expect(dts).not.toContain('a.test')
186 })
187})