From c8a039752b21d1f74656665e51a22e916ec8d495 Mon Sep 17 00:00:00 2001 From: Johannes Date: Mon, 23 Feb 2026 12:19:43 +0100 Subject: [PATCH 1/2] feat: enhance source map handling in NLS and private field conversion - Implemented inline source maps in the NLS plugin to ensure accurate mapping from transformed source back to original. - Modified `transformToPlaceholders` to return edits for source map adjustments. - Added `adjustSourceMap` function to update source maps based on text edits in `convertPrivateFields`. - Introduced tests for source map accuracy in both NLS and private field conversion scenarios. - Updated documentation to reflect changes in source map handling and the rationale behind accepting column imprecision. --- build/next/index.ts | 19 +- build/next/nls-plugin.ts | 109 +++++- build/next/private-to-property.ts | 119 ++++++- build/next/test/nls-sourcemap.test.ts | 373 ++++++++++++++++++++ build/next/test/private-to-property.test.ts | 183 +++++++++- build/next/working.md | 45 ++- 6 files changed, 819 insertions(+), 29 deletions(-) create mode 100644 build/next/test/nls-sourcemap.test.ts diff --git a/build/next/index.ts b/build/next/index.ts index e8f5b1f72d1c1..72d86a9020656 100644 --- a/build/next/index.ts +++ b/build/next/index.ts @@ -10,7 +10,7 @@ import { promisify } from 'util'; import glob from 'glob'; import gulpWatch from '../lib/watch/index.ts'; import { nlsPlugin, createNLSCollector, finalizeNLS, postProcessNLS } from './nls-plugin.ts'; -import { convertPrivateFields, type ConvertPrivateFieldsResult } from './private-to-property.ts'; +import { convertPrivateFields, adjustSourceMap, type ConvertPrivateFieldsResult } from './private-to-property.ts'; import { getVersion } from '../lib/getVersion.ts'; import product from '../../product.json' with { type: 'json' }; import packageJson from '../../package.json' with { type: 'json' }; @@ -883,6 +883,8 @@ ${tslib}`, // Post-process and write all output files let bundled = 0; const mangleStats: { file: string; result: ConvertPrivateFieldsResult }[] = []; + // Map from JS file path to pre-mangle content + edits, for source map adjustment + const mangleEdits = new Map(); for (const { result } of buildResults) { if (!result.outputFiles) { continue; @@ -903,10 +905,12 @@ ${tslib}`, // Skip extension host bundles - they expose API surface to extensions // where true encapsulation matters more than the perf gain. if (file.path.endsWith('.js') && doManglePrivates && !isExtensionHostBundle(file.path)) { + const preMangleCode = content; const mangleResult = convertPrivateFields(content, file.path); content = mangleResult.code; if (mangleResult.editCount > 0) { mangleStats.push({ file: path.relative(path.join(REPO_ROOT, outDir), file.path), result: mangleResult }); + mangleEdits.set(file.path, { preMangleCode, edits: mangleResult.edits }); } } @@ -924,8 +928,19 @@ ${tslib}`, } await fs.promises.writeFile(file.path, content); + } else if (file.path.endsWith('.map')) { + // Source maps may need adjustment if private fields were mangled + const jsPath = file.path.replace(/\.map$/, ''); + const editInfo = mangleEdits.get(jsPath); + if (editInfo) { + const mapJson = JSON.parse(file.text); + const adjusted = adjustSourceMap(mapJson, editInfo.preMangleCode, editInfo.edits); + await fs.promises.writeFile(file.path, JSON.stringify(adjusted)); + } else { + await fs.promises.writeFile(file.path, file.contents); + } } else { - // Write other files (source maps, assets) as-is + // Write other files (assets, etc.) as-is await fs.promises.writeFile(file.path, file.contents); } } diff --git a/build/next/nls-plugin.ts b/build/next/nls-plugin.ts index 56cb84fa33a06..8282bc9f8973a 100644 --- a/build/next/nls-plugin.ts +++ b/build/next/nls-plugin.ts @@ -6,6 +6,7 @@ import * as esbuild from 'esbuild'; import * as path from 'path'; import * as fs from 'fs'; +import { SourceMapGenerator } from 'source-map'; import { TextModel, analyzeLocalizeCalls, @@ -160,10 +161,17 @@ export function postProcessNLS( // Transformation // ============================================================================ +interface NLSEdit { + line: number; // 0-based line in original source + startCol: number; // 0-based start column in original + endCol: number; // 0-based end column in original + newLength: number; // length of replacement text +} + function transformToPlaceholders( source: string, moduleId: string -): { code: string; entries: NLSEntry[] } { +): { code: string; entries: NLSEntry[]; edits: NLSEdit[] } { const localizeCalls = analyzeLocalizeCalls(source, 'localize'); const localize2Calls = analyzeLocalizeCalls(source, 'localize2'); @@ -176,10 +184,11 @@ function transformToPlaceholders( ); if (allCalls.length === 0) { - return { code: source, entries: [] }; + return { code: source, entries: [], edits: [] }; } const entries: NLSEntry[] = []; + const edits: NLSEdit[] = []; const model = new TextModel(source); // Process in reverse order to preserve positions @@ -201,14 +210,92 @@ function transformToPlaceholders( placeholder }); + const replacementText = `"${placeholder}"`; + + // Track the edit for source map generation (positions are in original source coords) + edits.push({ + line: call.keySpan.start.line, + startCol: call.keySpan.start.character, + endCol: call.keySpan.end.character, + newLength: replacementText.length, + }); + // Replace the key with the placeholder string - model.apply(call.keySpan, `"${placeholder}"`); + model.apply(call.keySpan, replacementText); } - // Reverse entries to match source order + // Reverse entries and edits to match source order entries.reverse(); + edits.reverse(); + + return { code: model.toString(), entries, edits }; +} + +/** + * Generates a source map that maps from the NLS-transformed source back to the + * original source. esbuild composes this with its own bundle source map so that + * the final source map points all the way back to the untransformed TypeScript. + */ +function generateNLSSourceMap( + originalSource: string, + filePath: string, + edits: NLSEdit[] +): string { + const generator = new SourceMapGenerator(); + generator.setSourceContent(filePath, originalSource); + + const lineCount = originalSource.split('\n').length; + + // Group edits by line + const editsByLine = new Map(); + for (const edit of edits) { + let arr = editsByLine.get(edit.line); + if (!arr) { + arr = []; + editsByLine.set(edit.line, arr); + } + arr.push(edit); + } + + for (let line = 0; line < lineCount; line++) { + const smLine = line + 1; // source maps use 1-based lines + + // Always map start of line + generator.addMapping({ + generated: { line: smLine, column: 0 }, + original: { line: smLine, column: 0 }, + source: filePath, + }); + + const lineEdits = editsByLine.get(line); + if (lineEdits) { + lineEdits.sort((a, b) => a.startCol - b.startCol); + + let cumulativeShift = 0; + + for (const edit of lineEdits) { + const origLen = edit.endCol - edit.startCol; + + // Map start of edit: the replacement begins at the same original position + generator.addMapping({ + generated: { line: smLine, column: edit.startCol + cumulativeShift }, + original: { line: smLine, column: edit.startCol }, + source: filePath, + }); + + cumulativeShift += edit.newLength - origLen; + + // Map content after edit: columns resume with the shift applied + generator.addMapping({ + generated: { line: smLine, column: edit.endCol + cumulativeShift }, + original: { line: smLine, column: edit.endCol }, + source: filePath, + }); + } + } + } - return { code: model.toString(), entries }; + return generator.toString(); } function replaceInOutput( @@ -300,7 +387,7 @@ export function nlsPlugin(options: NLSPluginOptions): esbuild.Plugin { .replace(/\.ts$/, ''); // Transform localize() calls to placeholders - const { code, entries: fileEntries } = transformToPlaceholders(source, moduleId); + const { code, entries: fileEntries, edits } = transformToPlaceholders(source, moduleId); // Collect entries for (const entry of fileEntries) { @@ -308,7 +395,15 @@ export function nlsPlugin(options: NLSPluginOptions): esbuild.Plugin { } if (fileEntries.length > 0) { - return { contents: code, loader: 'ts' }; + // Generate a source map that maps from the NLS-transformed source + // back to the original. Embed it inline so esbuild composes it + // with its own bundle source map, making the final map point to + // the original TS source. + const sourceName = path.basename(args.path); + const sourcemap = generateNLSSourceMap(source, sourceName, edits); + const encodedMap = Buffer.from(sourcemap).toString('base64'); + const contentsWithMap = code + `\n//# sourceMappingURL=data:application/json;base64,${encodedMap}\n`; + return { contents: contentsWithMap, loader: 'ts' }; } // No NLS calls, return undefined to let esbuild handle normally diff --git a/build/next/private-to-property.ts b/build/next/private-to-property.ts index 64f1c4e74bf97..22d5b2be0d531 100644 --- a/build/next/private-to-property.ts +++ b/build/next/private-to-property.ts @@ -4,6 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import * as ts from 'typescript'; +import { type RawSourceMap, type Mapping, SourceMapConsumer, SourceMapGenerator } from 'source-map'; /** * Converts native ES private fields (`#foo`) into regular JavaScript properties with short, @@ -52,12 +53,20 @@ interface Edit { // Private name → replacement name per class (identified by position in file) type ClassScope = Map; +export interface TextEdit { + readonly start: number; + readonly end: number; + readonly newText: string; +} + export interface ConvertPrivateFieldsResult { readonly code: string; readonly classCount: number; readonly fieldCount: number; readonly editCount: number; readonly elapsed: number; + /** Sorted edits applied to the original code, for source map adjustment. */ + readonly edits: readonly TextEdit[]; } /** @@ -72,7 +81,7 @@ export function convertPrivateFields(code: string, filename: string): ConvertPri const t1 = Date.now(); // Quick bail-out: if there are no `#` characters, nothing to do if (!code.includes('#')) { - return { code, classCount: 0, fieldCount: 0, editCount: 0, elapsed: Date.now() - t1 }; + return { code, classCount: 0, fieldCount: 0, editCount: 0, elapsed: Date.now() - t1, edits: [] }; } const sourceFile = ts.createSourceFile(filename, code, ts.ScriptTarget.ESNext, false, ts.ScriptKind.JS); @@ -92,7 +101,7 @@ export function convertPrivateFields(code: string, filename: string): ConvertPri visit(sourceFile); if (edits.length === 0) { - return { code, classCount: 0, fieldCount: 0, editCount: 0, elapsed: Date.now() - t1 }; + return { code, classCount: 0, fieldCount: 0, editCount: 0, elapsed: Date.now() - t1, edits: [] }; } // Apply edits using substring concatenation (O(N+K), not O(N*K) like char-array splice) @@ -105,7 +114,7 @@ export function convertPrivateFields(code: string, filename: string): ConvertPri lastEnd = edit.end; } parts.push(code.substring(lastEnd)); - return { code: parts.join(''), classCount, fieldCount: nameCounter, editCount: edits.length, elapsed: Date.now() - t1 }; + return { code: parts.join(''), classCount, fieldCount: nameCounter, editCount: edits.length, elapsed: Date.now() - t1, edits }; // --- AST walking --- @@ -189,3 +198,107 @@ export function convertPrivateFields(code: string, filename: string): ConvertPri return undefined; } } + +/** + * Adjusts a source map to account for text edits applied to the generated JS. + * + * Each edit replaced a span `[start, end)` in the original generated JS with `newText`. + * This shifts all subsequent columns on the same line. The source map's generated + * columns are updated so they still point to the correct original positions. + * + * @param sourceMapJson The parsed source map JSON object. + * @param originalCode The original generated JS (before edits were applied). + * @param edits The sorted edits that were applied. + * @returns A new source map JSON object with adjusted generated columns. + */ +export function adjustSourceMap( + sourceMapJson: RawSourceMap, + originalCode: string, + edits: readonly TextEdit[] +): RawSourceMap { + if (edits.length === 0) { + return sourceMapJson; + } + + // Build a line-offset table for the original code to convert byte offsets to line/column + const lineStarts: number[] = [0]; + for (let i = 0; i < originalCode.length; i++) { + if (originalCode.charCodeAt(i) === 10 /* \n */) { + lineStarts.push(i + 1); + } + } + + function offsetToLineCol(offset: number): { line: number; col: number } { + let lo = 0, hi = lineStarts.length - 1; + while (lo < hi) { + const mid = (lo + hi + 1) >> 1; + if (lineStarts[mid] <= offset) { + lo = mid; + } else { + hi = mid - 1; + } + } + return { line: lo, col: offset - lineStarts[lo] }; + } + + // Convert edits from byte offsets to per-line column shifts + interface LineEdit { col: number; origLen: number; newLen: number } + const editsByLine = new Map(); + for (const edit of edits) { + const pos = offsetToLineCol(edit.start); + const origLen = edit.end - edit.start; + let arr = editsByLine.get(pos.line); + if (!arr) { + arr = []; + editsByLine.set(pos.line, arr); + } + arr.push({ col: pos.col, origLen, newLen: edit.newText.length }); + } + + // Use source-map library to read, adjust, and write + const consumer = new SourceMapConsumer(sourceMapJson); + const generator = new SourceMapGenerator({ file: sourceMapJson.file }); + + // Copy sourcesContent + for (let i = 0; i < sourceMapJson.sources.length; i++) { + const content = sourceMapJson.sourcesContent?.[i]; + if (content !== null && content !== undefined) { + generator.setSourceContent(sourceMapJson.sources[i], content); + } + } + + // Walk every mapping, adjust the generated column, and add to the new generator + consumer.eachMapping(mapping => { + const lineEdits = editsByLine.get(mapping.generatedLine - 1); // 0-based for our data + const adjustedCol = adjustColumn(mapping.generatedColumn, lineEdits); + + const newMapping: Mapping = { + generated: { line: mapping.generatedLine, column: adjustedCol }, + original: { line: mapping.originalLine, column: mapping.originalColumn }, + source: mapping.source, + }; + if (mapping.name !== null) { + newMapping.name = mapping.name; + } + generator.addMapping(newMapping); + }); + + return JSON.parse(generator.toString()); +} + +function adjustColumn(col: number, lineEdits: { col: number; origLen: number; newLen: number }[] | undefined): number { + if (!lineEdits) { + return col; + } + let shift = 0; + for (const edit of lineEdits) { + if (edit.col + edit.origLen <= col) { + shift += edit.newLen - edit.origLen; + } else if (edit.col < col) { + return edit.col + shift; + } else { + break; + } + } + return col + shift; +} diff --git a/build/next/test/nls-sourcemap.test.ts b/build/next/test/nls-sourcemap.test.ts new file mode 100644 index 0000000000000..e940a8d1cd81f --- /dev/null +++ b/build/next/test/nls-sourcemap.test.ts @@ -0,0 +1,373 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import assert from 'assert'; +import * as esbuild from 'esbuild'; +import * as path from 'path'; +import * as fs from 'fs'; +import * as os from 'os'; +import { SourceMapConsumer } from 'source-map'; +import { nlsPlugin, createNLSCollector, finalizeNLS, postProcessNLS } from '../nls-plugin.ts'; + +// analyzeLocalizeCalls requires the import path to end with `/nls` +const NLS_STUB = [ + 'export function localize(key: string, message: string, ...args: any[]): string {', + '\treturn message;', + '}', + 'export function localize2(key: string, message: string, ...args: any[]): { value: string; original: string } {', + '\treturn { value: message, original: message };', + '}', +].join('\n'); + +interface BundleResult { + js: string; + mapJson: any; + map: SourceMapConsumer; + cleanup: () => void; +} + +/** + * Helper: create a temp directory with source files, bundle with NLS, and return + * the generated JS + parsed source map. The NLS stub is automatically placed at + * `vs/nls.ts` so test files can import from `../vs/nls` (when placed in `test/`). + */ +async function bundleWithNLS( + files: Record, + entryPoint: string, + opts?: { postProcess?: boolean } +): Promise { + const tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'nls-sm-test-')); + const srcDir = path.join(tmpDir, 'src'); + const outDir = path.join(tmpDir, 'out'); + await fs.promises.mkdir(srcDir, { recursive: true }); + await fs.promises.mkdir(outDir, { recursive: true }); + + // Write source files (always include the NLS stub at vs/nls.ts) + const allFiles = { 'vs/nls.ts': NLS_STUB, ...files }; + for (const [name, content] of Object.entries(allFiles)) { + const filePath = path.join(srcDir, name); + await fs.promises.mkdir(path.dirname(filePath), { recursive: true }); + await fs.promises.writeFile(filePath, content); + } + + const collector = createNLSCollector(); + + const result = await esbuild.build({ + entryPoints: [path.join(srcDir, entryPoint)], + outfile: path.join(outDir, entryPoint.replace(/\.ts$/, '.js')), + bundle: true, + format: 'esm', + platform: 'neutral', + target: ['es2024'], + packages: 'external', + sourcemap: 'linked', + sourcesContent: true, + write: false, + plugins: [ + nlsPlugin({ baseDir: srcDir, collector }), + ], + tsconfigRaw: JSON.stringify({ + compilerOptions: { + experimentalDecorators: true, + useDefineForClassFields: false + } + }), + logLevel: 'warning', + }); + + let jsContent = ''; + let mapContent = ''; + + for (const file of result.outputFiles!) { + if (file.path.endsWith('.js')) { + jsContent = file.text; + } else if (file.path.endsWith('.map')) { + mapContent = file.text; + } + } + + // Optionally apply NLS post-processing (replaces placeholders with indices) + if (opts?.postProcess) { + const nlsResult = await finalizeNLS(collector, outDir); + jsContent = postProcessNLS(jsContent, nlsResult.indexMap, false); + } + + assert.ok(jsContent, 'Expected JS output'); + assert.ok(mapContent, 'Expected source map output'); + + const mapJson = JSON.parse(mapContent); + const map = new SourceMapConsumer(mapJson); + const cleanup = () => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }; + + return { js: jsContent, mapJson, map, cleanup }; +} + +/** + * Find the 1-based line number in `text` that contains `needle`. + */ +function findLine(text: string, needle: string): number { + const lines = text.split('\n'); + for (let i = 0; i < lines.length; i++) { + if (lines[i].includes(needle)) { + return i + 1; // 1-based + } + } + throw new Error(`Could not find "${needle}" in text`); +} + +/** + * Find the 0-based column of `needle` within the line that contains it. + */ +function findColumn(text: string, needle: string): number { + const lines = text.split('\n'); + for (const line of lines) { + const col = line.indexOf(needle); + if (col !== -1) { + return col; + } + } + throw new Error(`Could not find "${needle}" in text`); +} + +suite('NLS plugin source maps', () => { + + test('NLS plugin transforms localize calls into placeholders', async () => { + const source = [ + 'import { localize } from "../vs/nls";', + 'export const msg = localize("testKey", "Test Message");', + ].join('\n'); + + const { js, cleanup } = await bundleWithNLS( + { 'test/verify.ts': source }, + 'test/verify.ts', + ); + + try { + assert.ok(js.includes('%%NLS:'), + 'Bundle should contain %%NLS: placeholder.\nActual JS (first 500 chars):\n' + js.substring(0, 500)); + } finally { + cleanup(); + } + }); + + test('file without localize calls has correct source map', async () => { + const source = [ + 'export function add(a: number, b: number): number {', + '\treturn a + b;', + '}', + ].join('\n'); + + const { js, map, cleanup } = await bundleWithNLS( + { 'simple.ts': source }, + 'simple.ts', + ); + + try { + const bundleLine = findLine(js, 'return a + b'); + const bundleCol = findColumn(js, 'return a + b'); + const pos = map.originalPositionFor({ line: bundleLine, column: bundleCol }); + assert.ok(pos.source, 'Should have source'); + assert.strictEqual(pos.line, 2, 'Should map to line 2 of original'); + } finally { + cleanup(); + } + }); + + test('sourcesContent should contain original source, not NLS-transformed', async () => { + const source = [ + 'import { localize } from "../vs/nls";', + 'export const msg = localize("myKey", "Hello World");', + 'export function greet(): string {', + '\treturn msg;', + '}', + ].join('\n'); + + const { mapJson, cleanup } = await bundleWithNLS( + { 'test/greeting.ts': source }, + 'test/greeting.ts', + ); + + try { + const sourcesContent: string[] = mapJson.sourcesContent ?? []; + const sources: string[] = mapJson.sources ?? []; + const greetingIdx = sources.findIndex((s: string) => s.includes('greeting')); + assert.ok(greetingIdx >= 0, 'Should find greeting.ts in sources'); + + const greetingContent = sourcesContent[greetingIdx]; + assert.ok(greetingContent, 'Should have sourcesContent for greeting.ts'); + + assert.ok(!greetingContent.includes('%%NLS:'), + 'sourcesContent should NOT contain NLS placeholder.\nActual:\n' + greetingContent); + assert.ok(greetingContent.includes('localize("myKey", "Hello World")'), + 'sourcesContent should contain the exact original localize call.\nActual:\n' + greetingContent); + } finally { + cleanup(); + } + }); + + test('line mapping correct for code after localize calls', async () => { + const source = [ + 'import { localize } from "../vs/nls";', // 1 + 'const label = localize("key1", "A long message");', // 2 + 'const label2 = localize("key2", "Another message");', // 3 + 'export function computeResult(x: number): number {', // 4 + '\treturn x * 42;', // 5 + '}', // 6 + ].join('\n'); + + const { js, map, cleanup } = await bundleWithNLS( + { 'test/multi.ts': source }, + 'test/multi.ts', + ); + + try { + const bundleLine = findLine(js, 'return x * 42'); + const bundleCol = findColumn(js, 'return x * 42'); + const pos = map.originalPositionFor({ line: bundleLine, column: bundleCol }); + assert.ok(pos.source, 'Should have source'); + assert.strictEqual(pos.line, 5, 'Should map back to line 5'); + } finally { + cleanup(); + } + }); + + test('column mapping for code on same line after localize call', async () => { + // The NLS placeholder is longer than the original key, so column offsets + // for tokens AFTER the localize call on the same line will drift if + // source map mappings point to the NLS-transformed source positions. + const source = [ + 'import { localize } from "../vs/nls";', + 'const x = localize("k", "m"); const z = "FINDME"; export { x, z };', + ].join('\n'); + + const { js, map, cleanup } = await bundleWithNLS( + { 'test/coldrift.ts': source }, + 'test/coldrift.ts', + ); + + try { + assert.ok(js.includes('%%NLS:'), 'Bundle should contain NLS placeholders'); + + const bundleLine = findLine(js, 'FINDME'); + const bundleCol = findColumn(js, '"FINDME"'); + const pos = map.originalPositionFor({ line: bundleLine, column: bundleCol }); + + assert.ok(pos.source, 'Should have source'); + assert.strictEqual(pos.line, 2, 'Should map to line 2'); + + // The original column of "FINDME" in the source + const originalCol = findColumn(source, '"FINDME"'); + + // The mapped column should match the ORIGINAL source positions. + // Allow drift from TS->JS transform (const->var, export removal, etc.) + // but NOT the large NLS placeholder drift (~100+ chars) from before the fix. + const columnDrift = Math.abs(pos.column! - originalCol); + assert.ok(columnDrift <= 20, + `Column should be close to original. Expected ~${originalCol}, got ${pos.column} (drift: ${columnDrift}). ` + + `A drift > 20 indicates the NLS placeholder shift leaked into the source map.`); + } finally { + cleanup(); + } + }); + + test('class with localize - method positions map correctly', async () => { + const source = [ + 'import { localize } from "../vs/nls";', // 1 + '', // 2 + 'export class MyWidget {', // 3 + '\tprivate readonly label = localize("widgetLabel", "My Cool Widget");', // 4 + '', // 5 + '\tconstructor(private readonly name: string) {}', // 6 + '', // 7 + '\tgetDescription(): string {', // 8 + '\t\treturn this.name + ": " + this.label;', // 9 + '\t}', // 10 + '', // 11 + '\tdispose(): void {', // 12 + '\t\tconsole.log("disposed");', // 13 + '\t}', // 14 + '}', // 15 + ].join('\n'); + + const { js, map, cleanup } = await bundleWithNLS( + { 'test/widget.ts': source }, + 'test/widget.ts', + ); + + try { + const bundleLine = findLine(js, '"disposed"'); + const bundleCol = findColumn(js, 'console.log'); + const pos = map.originalPositionFor({ line: bundleLine, column: bundleCol }); + assert.ok(pos.source, 'Should have source'); + assert.strictEqual(pos.line, 13, 'Should map dispose method body to line 13'); + } finally { + cleanup(); + } + }); + + test('many localize calls - line mappings remain correct', async () => { + const source = [ + 'import { localize } from "../vs/nls";', // 1 + '', // 2 + 'const a = localize("a", "Alpha");', // 3 + 'const b = localize("b", "Bravo with a longer message");', // 4 + 'const c = localize("c", "Charlie");', // 5 + 'const d = localize("d", "Delta is the fourth");', // 6 + 'const e = localize("e", "Echo");', // 7 + '', // 8 + 'export function getAll(): string {', // 9 + '\treturn [a, b, c, d, e].join(", ");', // 10 + '}', // 11 + ].join('\n'); + + const { js, map, cleanup } = await bundleWithNLS( + { 'test/many.ts': source }, + 'test/many.ts', + ); + + try { + const bundleLine = findLine(js, '.join(", ")'); + const bundleCol = findColumn(js, '.join(", ")'); + const pos = map.originalPositionFor({ line: bundleLine, column: bundleCol }); + assert.ok(pos.source, 'Should have source'); + assert.strictEqual(pos.line, 10, 'Should map join() back to line 10'); + } finally { + cleanup(); + } + }); + + test('post-processed NLS - source map still has original content', async () => { + const source = [ + 'import { localize } from "../vs/nls";', + 'export const msg = localize("greeting", "Hello World");', + ].join('\n'); + + const { js, mapJson, cleanup } = await bundleWithNLS( + { 'test/post.ts': source }, + 'test/post.ts', + { postProcess: true } + ); + + try { + assert.ok(!js.includes('%%NLS:'), 'JS should not contain NLS placeholders after post-processing'); + + const sources: string[] = mapJson.sources ?? []; + const postIdx = sources.findIndex((s: string) => s.includes('post')); + assert.ok(postIdx >= 0, 'Should find post.ts in sources'); + + const postContent = (mapJson.sourcesContent ?? [])[postIdx]; + assert.ok(postContent, 'Should have sourcesContent for post.ts'); + + assert.ok(postContent.includes('localize("greeting"'), + 'sourcesContent should still contain original localize("greeting") call'); + assert.ok(!postContent.includes('%%NLS:'), + 'sourcesContent should not contain NLS placeholders'); + } finally { + cleanup(); + } + }); +}); diff --git a/build/next/test/private-to-property.test.ts b/build/next/test/private-to-property.test.ts index 3cde63a4bdaf4..aa9da72ce9a51 100644 --- a/build/next/test/private-to-property.test.ts +++ b/build/next/test/private-to-property.test.ts @@ -4,7 +4,8 @@ *--------------------------------------------------------------------------------------------*/ import assert from 'assert'; -import { convertPrivateFields } from '../private-to-property.ts'; +import { convertPrivateFields, adjustSourceMap } from '../private-to-property.ts'; +import { SourceMapConsumer, SourceMapGenerator, type RawSourceMap } from 'source-map'; suite('convertPrivateFields', () => { @@ -272,4 +273,184 @@ suite('convertPrivateFields', () => { assert.ok(result.code.includes('$aa')); }); }); + + test('returns edits array', () => { + const code = 'class Foo { #x = 1; get() { return this.#x; } }'; + const result = convertPrivateFields(code, 'test.js'); + assert.strictEqual(result.edits.length, 2); + // Edits should be sorted by start position + assert.ok(result.edits[0].start < result.edits[1].start); + // First edit is the declaration #x, second is the usage this.#x + assert.strictEqual(result.edits[0].newText, '$a'); + assert.strictEqual(result.edits[1].newText, '$a'); + }); + + test('no edits when no private fields', () => { + const code = 'class Foo { x = 1; }'; + const result = convertPrivateFields(code, 'test.js'); + assert.deepStrictEqual(result.edits, []); + }); +}); + +suite('adjustSourceMap', () => { + + /** + * Helper: creates a source map with dense 1:1 mappings (every character) + * for a single-source file. Each column maps generated -> original identity. + */ + function createIdentitySourceMap(code: string, sourceName: string): RawSourceMap { + const gen = new SourceMapGenerator(); + gen.setSourceContent(sourceName, code); + const lines = code.split('\n'); + for (let line = 0; line < lines.length; line++) { + for (let col = 0; col < lines[line].length; col++) { + gen.addMapping({ + generated: { line: line + 1, column: col }, + original: { line: line + 1, column: col }, + source: sourceName, + }); + } + } + return JSON.parse(gen.toString()); + } + + test('no edits - returns mappings unchanged', () => { + const code = 'class Foo { x = 1; }'; + const map = createIdentitySourceMap(code, 'test.js'); + const originalMappings = map.mappings; + const result = adjustSourceMap(map, code, []); + assert.strictEqual(result.mappings, originalMappings); + }); + + test('single edit shrinks token - columns after edit shift left', () => { + // "var #longName = 1; var y = 2;" + // 0 4 14 22 + // After: "var $a = 1; var y = 2;" + // 0 4 7 15 + const code = 'var #longName = 1; var y = 2;'; + // Create a sparse map with mappings only at known token positions + const gen = new SourceMapGenerator(); + gen.setSourceContent('test.js', code); + // Map 'var' at col 0 + gen.addMapping({ generated: { line: 1, column: 0 }, original: { line: 1, column: 0 }, source: 'test.js' }); + // Map '#longName' at col 4 + gen.addMapping({ generated: { line: 1, column: 4 }, original: { line: 1, column: 4 }, source: 'test.js' }); + // Map '=' at col 14 + gen.addMapping({ generated: { line: 1, column: 14 }, original: { line: 1, column: 14 }, source: 'test.js' }); + // Map 'var' at col 19 + gen.addMapping({ generated: { line: 1, column: 19 }, original: { line: 1, column: 19 }, source: 'test.js' }); + // Map 'y' at col 23 + gen.addMapping({ generated: { line: 1, column: 23 }, original: { line: 1, column: 23 }, source: 'test.js' }); + const map = JSON.parse(gen.toString()); + + const result = adjustSourceMap(map, code, [{ start: 4, end: 13, newText: '$a' }]); + + const consumer = new SourceMapConsumer(result); + // 'y' was at gen col 23, edit shrunk 9->2 chars (delta -7), so now at gen col 16 + const pos = consumer.originalPositionFor({ line: 1, column: 16 }); + assert.strictEqual(pos.column, 23, 'y should map back to original column 23'); + + // '=' was at gen col 14, edit shrunk by 7, so now at gen col 7 + const pos2 = consumer.originalPositionFor({ line: 1, column: 7 }); + assert.strictEqual(pos2.column, 14, '= should map back to original column 14'); + }); + + test('edit on line does not affect other lines', () => { + const code = 'class Foo {\n #x = 1;\n get() { return 42; }\n}'; + const map = createIdentitySourceMap(code, 'test.js'); + + const hashPos = code.indexOf('#x'); + const result = adjustSourceMap(map, code, [{ start: hashPos, end: hashPos + 2, newText: '$a' }]); + + const consumer = new SourceMapConsumer(result); + // Line 3 (1-based) should be completely unaffected + const pos = consumer.originalPositionFor({ line: 3, column: 0 }); + assert.strictEqual(pos.line, 3); + assert.strictEqual(pos.column, 0); + }); + + test('multiple edits on same line accumulate shifts', () => { + // "this.#aaa + this.#bbb + this.#ccc;" + // 0 5 11 17 23 29 + const code = 'this.#aaa + this.#bbb + this.#ccc;'; + // Sparse map at token boundaries (not inside edit spans) + const gen = new SourceMapGenerator(); + gen.setSourceContent('test.js', code); + gen.addMapping({ generated: { line: 1, column: 0 }, original: { line: 1, column: 0 }, source: 'test.js' }); // 'this' + gen.addMapping({ generated: { line: 1, column: 5 }, original: { line: 1, column: 5 }, source: 'test.js' }); // '#aaa' + gen.addMapping({ generated: { line: 1, column: 10 }, original: { line: 1, column: 10 }, source: 'test.js' }); // '+' + gen.addMapping({ generated: { line: 1, column: 12 }, original: { line: 1, column: 12 }, source: 'test.js' }); // 'this' + gen.addMapping({ generated: { line: 1, column: 17 }, original: { line: 1, column: 17 }, source: 'test.js' }); // '#bbb' + gen.addMapping({ generated: { line: 1, column: 22 }, original: { line: 1, column: 22 }, source: 'test.js' }); // '+' + gen.addMapping({ generated: { line: 1, column: 24 }, original: { line: 1, column: 24 }, source: 'test.js' }); // 'this' + gen.addMapping({ generated: { line: 1, column: 29 }, original: { line: 1, column: 29 }, source: 'test.js' }); // '#ccc' + gen.addMapping({ generated: { line: 1, column: 33 }, original: { line: 1, column: 33 }, source: 'test.js' }); // ';' + const map = JSON.parse(gen.toString()); + + const edits = [ + { start: 5, end: 9, newText: '$a' }, // #aaa(4) -> $a(2), delta -2 + { start: 17, end: 21, newText: '$b' }, // #bbb(4) -> $b(2), delta -2 + { start: 29, end: 33, newText: '$c' }, // #ccc(4) -> $c(2), delta -2 + ]; + const result = adjustSourceMap(map, code, edits); + + const consumer = new SourceMapConsumer(result); + // After edits: "this.$a + this.$b + this.$c;" + // '#ccc' was at gen col 29, now at 29-2-2=25 + const pos = consumer.originalPositionFor({ line: 1, column: 25 }); + assert.strictEqual(pos.column, 29, 'third edit position should map to original column'); + + // '+' after #bbb was at gen col 22, both prior edits shift by -2 each: 22-4=18 + const pos2 = consumer.originalPositionFor({ line: 1, column: 18 }); + assert.strictEqual(pos2.column, 22, 'plus after second edit should map correctly'); + }); + + test('end-to-end: convertPrivateFields + adjustSourceMap', () => { + const code = [ + 'class MyWidget {', + ' #count = 0;', + ' increment() { this.#count++; }', + ' getValue() { return this.#count; }', + '}', + ].join('\n'); + + const map = createIdentitySourceMap(code, 'widget.js'); + const result = convertPrivateFields(code, 'widget.js'); + + assert.ok(result.edits.length > 0, 'should have edits'); + assert.ok(!result.code.includes('#count'), 'should not contain #count'); + + // Adjust the source map + const adjusted = adjustSourceMap(map, code, result.edits); + const consumer = new SourceMapConsumer(adjusted); + + // Find 'getValue' in the edited output and verify it maps back correctly + const editedLines = result.code.split('\n'); + const getValueLine = editedLines.findIndex(l => l.includes('getValue')); + assert.ok(getValueLine >= 0, 'should find getValue in edited code'); + + const getValueCol = editedLines[getValueLine].indexOf('getValue'); + const pos = consumer.originalPositionFor({ line: getValueLine + 1, column: getValueCol }); + + // getValue was on line 4 (1-based), same column in original + const origLines = code.split('\n'); + const origGetValueCol = origLines[3].indexOf('getValue'); + assert.strictEqual(pos.line, 4, 'getValue should map to original line 4'); + assert.strictEqual(pos.column, origGetValueCol, 'getValue column should match original'); + }); + + test('brand check: #field in obj -> string replacement adjusts map', () => { + const code = 'class C { #x; check(o) { return #x in o; } }'; + const map = createIdentitySourceMap(code, 'test.js'); + + const result = convertPrivateFields(code, 'test.js'); + const adjusted = adjustSourceMap(map, code, result.edits); + const consumer = new SourceMapConsumer(adjusted); + + // 'check' method should still map correctly + const editedCheckCol = result.code.indexOf('check'); + const pos = consumer.originalPositionFor({ line: 1, column: editedCheckCol }); + assert.strictEqual(pos.line, 1); + assert.strictEqual(pos.column, code.indexOf('check')); + }); }); diff --git a/build/next/working.md b/build/next/working.md index 71aac3fbdaf4c..b59b347611dbd 100644 --- a/build/next/working.md +++ b/build/next/working.md @@ -202,31 +202,44 @@ npm run gulp vscode-reh-web-darwin-arm64-min ## Source Maps +### Principle: Every Code Transform Must Preserve Source Maps + +Any step that modifies JS output - whether in an esbuild plugin or in post-processing - **must** update the source map accordingly. Failing to do so causes column drift that makes debuggers, crash reporters, and breakpoints point to wrong positions. The `source-map` library (v0.6.1, already a dependency) provides the `SourceMapConsumer`/`SourceMapGenerator` APIs for this. + +### Root Causes (before fixes) + +Source maps worked in transpile mode but failed in bundle mode. The observed pattern was "class-heavy files fail, simple utilities work" but the real cause was **"files with NLS calls vs files without"**. Class-heavy UI components use `localize()` extensively; utility files in `vs/base/` don't. + +Two categories of corruption: + +1. **NLS plugin `onLoad`** returned modified source without a source map. esbuild treated the NLS-transformed text as the "original" - `sourcesContent` embedded placeholders, and column mappings pointed to wrong positions. + +2. **Post-processing** (`postProcessNLS`, `convertPrivateFields`) modified bundled JS output without updating `.map` files. Column positions drifted by the cumulative length deltas of all replacements. + ### Fixes Applied -1. **`sourcesContent: true`** — Production bundles now embed original TypeScript source content in `.map` files, matching the old build's `includeContent: true` behavior. Without this, crash reports from CDN-hosted source maps can't show original source. +1. **`sourcesContent: true`** - Production bundles embed original TypeScript source content in `.map` files, matching the old build's `includeContent: true` behavior. + +2. **`--source-map-base-url` option** - Rewrites `sourceMappingURL` comments to point to CDN URLs. + +3. **NLS plugin inline source maps** (`nls-plugin.ts`) - The `onLoad` handler now generates an inline source map (`//# sourceMappingURL=data:...`) mapping from NLS-transformed source back to original. esbuild composes this with its own bundle source map. `SourceMapGenerator.setSourceContent` embeds the original source so `sourcesContent` in the final `.map` has the real TypeScript. Tests in `test/nls-sourcemap.test.ts`. -2. **`--source-map-base-url` option** — The `bundle` command accepts an optional `--source-map-base-url ` flag. When set, post-processing rewrites `sourceMappingURL` comments in `.js` and `.css` output files to point to the CDN (e.g., `https://main.vscode-cdn.net/sourcemaps//core/vs/...`). This matches the old build's `sourceMappingURL` function in `minifyTask()`. Wired up in `gulpfile.vscode.ts` for `core-ci-esbuild` and `vscode-esbuild-min` tasks. +4. **`convertPrivateFields` source map adjustment** (`private-to-property.ts`) - `convertPrivateFields` returns its sorted edits as `TextEdit[]`. `adjustSourceMap()` uses `SourceMapConsumer` to walk every mapping, adjusts generated columns based on cumulative edit shifts per line, and rebuilds with `SourceMapGenerator`. The post-processing loop in `index.ts` saves pre-mangle content + edits per JS file, then applies `adjustSourceMap` to the corresponding `.map`. Tests in `test/private-to-property.test.ts`. -### NLS Source Map Accuracy (Decision: Accept Imprecision) +### Not Yet Fixed -**Problem:** `postProcessNLS()` replaces `"%%NLS:moduleId#key%%"` placeholders (~40 chars) with short index values like `null` (4 chars) in the final JS output. This shifts column positions without updating the `.map` files. +**`postProcessNLS` column drift** - Replaces NLS placeholders with short indices in bundled output without updating `.map` files. Shifts columns but never lines, so line-level debugging and crash reporting work correctly. Fixing would require tracking replacement offsets through regex matches and adjusting the source map, similar to `adjustSourceMap`. -**Options considered:** +### Key Technical Details -| Option | Description | Effort | Accuracy | -|--------|-------------|--------|----------| -| A. Fixed-width placeholders | Pad placeholders to match replacement length | Hard — indices unknown until all modules are collected across parallel bundles | Perfect | -| B. Post-process source map | Parse `.map`, track replacement offsets per line, adjust VLQ mappings | Medium | Perfect | -| C. Two-pass build | Assign NLS indices during plugin phase | Not feasible with parallel bundling | N/A | -| **D. Accept imprecision** | NLS replacements only affect column positions; line-level debugging works | Zero | Line-level | +**esbuild `onLoad` source map composition:** esbuild's `onLoad` return type does NOT have a `sourcemap` field (as of v0.27.2). The only way to provide input source maps is to embed them inline in the returned `contents`. esbuild reads this and composes it with its own transform/bundle map. With `sourcesContent: true`, esbuild uses the source content from the inline map, not the `contents` string. -**Decision: Option D — accept imprecision.** Rationale: +**`adjustColumn` algorithm** handles three cases per edit on a line: +1. Edit entirely before the column: accumulate the delta (newLen - origLen) +2. Column falls inside the edit span: map to the start of the edit +3. Edit is after the column: stop (edits are sorted) -- NLS replacements only shift **columns**, never lines — line-level stack traces and breakpoints remain correct. -- Production crash reporting (the primary consumer of CDN source maps) uses line numbers; column-level accuracy is rarely needed. -- The old gulp build had the same fundamental issue in its `nls.nls()` step and used `SourceMapConsumer`/`SourceMapGenerator` to fix it — but that approach was fragile and slow. -- If column-level precision becomes important later (e.g., for minified+NLS bundles), Option B can be implemented: after NLS replacement, re-parse the source map, walk replacement sites, and adjust column offsets. This is a localized change in the post-processing loop. +**Plugin interaction:** Both the NLS plugin and `fileContentMapperPlugin` register `onLoad({ filter: /\.ts$/ })`. In esbuild, the first `onLoad` to return non-`undefined` wins. The NLS plugin is `unshift`ed (runs first), so files with NLS calls skip `fileContentMapperPlugin`. This is safe in practice since `product.ts` (which has `BUILD->INSERT_PRODUCT_CONFIGURATION`) has no localize calls. --- From 501bf7e229704b604c2ba3512b17c57c7e4369ee Mon Sep 17 00:00:00 2001 From: Johannes Date: Mon, 23 Feb 2026 12:42:38 +0100 Subject: [PATCH 2/2] address PR review: fix source name collision, guard unmapped segments, reorder mangle before NLS --- build/next/index.ts | 14 ++++++++------ build/next/nls-plugin.ts | 2 +- build/next/private-to-property.ts | 19 +++++++++++-------- build/next/test/nls-sourcemap.test.ts | 4 ++-- 4 files changed, 22 insertions(+), 17 deletions(-) diff --git a/build/next/index.ts b/build/next/index.ts index 72d86a9020656..b0120837efa26 100644 --- a/build/next/index.ts +++ b/build/next/index.ts @@ -896,12 +896,9 @@ ${tslib}`, if (file.path.endsWith('.js') || file.path.endsWith('.css')) { let content = file.text; - // Apply NLS post-processing if enabled (JS only) - if (file.path.endsWith('.js') && doNls && indexMap.size > 0) { - content = postProcessNLS(content, indexMap, preserveEnglish); - } - - // Convert native #private fields to regular properties. + // Convert native #private fields to regular properties BEFORE NLS + // post-processing, so that the edit offsets align with esbuild's + // source map coordinate system (both reference the raw esbuild output). // Skip extension host bundles - they expose API surface to extensions // where true encapsulation matters more than the perf gain. if (file.path.endsWith('.js') && doManglePrivates && !isExtensionHostBundle(file.path)) { @@ -914,6 +911,11 @@ ${tslib}`, } } + // Apply NLS post-processing if enabled (JS only) + if (file.path.endsWith('.js') && doNls && indexMap.size > 0) { + content = postProcessNLS(content, indexMap, preserveEnglish); + } + // Rewrite sourceMappingURL to CDN URL if configured if (sourceMapBaseUrl) { const relativePath = path.relative(path.join(REPO_ROOT, outDir), file.path); diff --git a/build/next/nls-plugin.ts b/build/next/nls-plugin.ts index 8282bc9f8973a..7be3faccf2439 100644 --- a/build/next/nls-plugin.ts +++ b/build/next/nls-plugin.ts @@ -399,7 +399,7 @@ export function nlsPlugin(options: NLSPluginOptions): esbuild.Plugin { // back to the original. Embed it inline so esbuild composes it // with its own bundle source map, making the final map point to // the original TS source. - const sourceName = path.basename(args.path); + const sourceName = relativePath.replace(/\\/g, '/'); const sourcemap = generateNLSSourceMap(source, sourceName, edits); const encodedMap = Buffer.from(sourcemap).toString('base64'); const contentsWithMap = code + `\n//# sourceMappingURL=data:application/json;base64,${encodedMap}\n`; diff --git a/build/next/private-to-property.ts b/build/next/private-to-property.ts index 22d5b2be0d531..11f977774a5fd 100644 --- a/build/next/private-to-property.ts +++ b/build/next/private-to-property.ts @@ -272,15 +272,18 @@ export function adjustSourceMap( const lineEdits = editsByLine.get(mapping.generatedLine - 1); // 0-based for our data const adjustedCol = adjustColumn(mapping.generatedColumn, lineEdits); - const newMapping: Mapping = { - generated: { line: mapping.generatedLine, column: adjustedCol }, - original: { line: mapping.originalLine, column: mapping.originalColumn }, - source: mapping.source, - }; - if (mapping.name !== null) { - newMapping.name = mapping.name; + // Some mappings may be unmapped (no original position/source) - skip those. + if (mapping.source !== null && mapping.originalLine !== null && mapping.originalColumn !== null) { + const newMapping: Mapping = { + generated: { line: mapping.generatedLine, column: adjustedCol }, + original: { line: mapping.originalLine, column: mapping.originalColumn }, + source: mapping.source, + }; + if (mapping.name !== null) { + newMapping.name = mapping.name; + } + generator.addMapping(newMapping); } - generator.addMapping(newMapping); }); return JSON.parse(generator.toString()); diff --git a/build/next/test/nls-sourcemap.test.ts b/build/next/test/nls-sourcemap.test.ts index e940a8d1cd81f..fd732b8680217 100644 --- a/build/next/test/nls-sourcemap.test.ts +++ b/build/next/test/nls-sourcemap.test.ts @@ -8,7 +8,7 @@ import * as esbuild from 'esbuild'; import * as path from 'path'; import * as fs from 'fs'; import * as os from 'os'; -import { SourceMapConsumer } from 'source-map'; +import { type RawSourceMap, SourceMapConsumer } from 'source-map'; import { nlsPlugin, createNLSCollector, finalizeNLS, postProcessNLS } from '../nls-plugin.ts'; // analyzeLocalizeCalls requires the import path to end with `/nls` @@ -23,7 +23,7 @@ const NLS_STUB = [ interface BundleResult { js: string; - mapJson: any; + mapJson: RawSourceMap; map: SourceMapConsumer; cleanup: () => void; }