From 0a5fa2dd8c4c06b200c8ea11d9f83f1aa9c8530b Mon Sep 17 00:00:00 2001 From: dave caruso Date: Fri, 31 May 2024 23:10:02 -0700 Subject: [PATCH] fix sourcemap printing with multiple chunks sharing the same file (#11509) --- src/bun.js/javascript.zig | 23 ++++--- src/bun.js/module_loader.zig | 65 ++++++++++++++----- src/bundler/bundle_v2.zig | 93 +++++++++++++++++---------- src/js_printer.zig | 18 +++++- src/sourcemap/sourcemap.zig | 87 +++++++++++++++++++++---- test/bundler/bundler_edgecase.test.ts | 32 +++++++++ test/bundler/expectBundled.ts | 6 +- 7 files changed, 247 insertions(+), 77 deletions(-) diff --git a/src/bun.js/javascript.zig b/src/bun.js/javascript.zig index b16e346b7b..9940fb6008 100644 --- a/src/bun.js/javascript.zig +++ b/src/bun.js/javascript.zig @@ -195,10 +195,7 @@ pub const SavedSourceMap = struct { pub fn print() void { if (seen_invalid) return; if (path) |note| { - Output.note( - "missing sourcemaps for {s}", - .{note}, - ); + Output.note("missing sourcemaps for {s}", .{note}); Output.note("consider bundling with '--sourcemap' to get an unminified traces", .{}); } } @@ -288,6 +285,7 @@ pub const SavedSourceMap = struct { this: *SavedSourceMap, path: string, hint: SourceMap.ParseUrlResultHint, + log: ?*logger.Log, ) SourceMap.ParseUrl { const hash = bun.hash(path); const mapping = this.map.getEntry(hash) orelse return .{}; @@ -309,7 +307,7 @@ pub const SavedSourceMap = struct { Value.Tag.SourceProviderMap => { var ptr = Value.from(mapping.value_ptr.*).as(SourceProviderMap); - if (ptr.getSourceMap(path, .none, hint)) |parse| + if (ptr.getSourceMap(path, .none, hint, log)) |parse| if (parse.map) |map| { mapping.value_ptr.* = Value.init(map).ptr(); return parse; @@ -334,7 +332,7 @@ pub const SavedSourceMap = struct { } pub fn get(this: *SavedSourceMap, path: string) ?*ParsedSourceMap { - return this.getWithContent(path, .mappings_only).map; + return this.getWithContent(path, .mappings_only, null).map; } pub fn resolveMapping( @@ -343,6 +341,7 @@ pub const SavedSourceMap = struct { line: i32, column: i32, source_handling: SourceMap.SourceContentHandling, + log: ?*logger.Log, ) ?SourceMap.Mapping.Lookup { this.mutex.lock(); defer this.mutex.unlock(); @@ -350,7 +349,7 @@ pub const SavedSourceMap = struct { const parse = this.getWithContent(path, switch (source_handling) { .no_source_contents => .mappings_only, .source_contents => .{ .all = .{ .line = line, .column = column } }, - }); + }, log); const map = parse.map orelse return null; const mapping = parse.mapping orelse SourceMap.Mapping.find(map.mappings, line, column) orelse @@ -2826,6 +2825,7 @@ pub const VirtualMachine = struct { @max(frame.position.line, 0), @max(frame.position.column_start, 0), .no_source_contents, + null, )) |lookup| { if (lookup.displaySourceURLIfNeeded(sourceURL.slice())) |source_url| { frame.source_url.deref(); @@ -2928,6 +2928,12 @@ pub const VirtualMachine = struct { var top_source_url = top.source_url.toUTF8(bun.default_allocator); defer top_source_url.deinit(); + var log = logger.Log.init(bun.default_allocator); + defer log.deinit(); + defer { + log.printForLogLevel(Output.errorWriter()) catch {}; + } + const maybe_lookup = if (top.remapped) SourceMap.Mapping.Lookup{ .mapping = .{ @@ -2948,6 +2954,7 @@ pub const VirtualMachine = struct { @max(top.position.line, 0), @max(top.position.column_start, 0), .source_contents, + &log, ); if (maybe_lookup) |lookup| { @@ -2967,7 +2974,6 @@ pub const VirtualMachine = struct { } } - var log = logger.Log.init(default_allocator); var original_source = fetchWithoutOnLoadPlugins(this, this.global, top.source_url, bun.String.empty, &log, .print_source) catch return; must_reset_parser_arena_later.* = true; break :code original_source.source_code.toUTF8(bun.default_allocator); @@ -3028,6 +3034,7 @@ pub const VirtualMachine = struct { @max(frame.position.line, 0), @max(frame.position.column_start, 0), .no_source_contents, + null, )) |lookup| { if (lookup.displaySourceURLIfNeeded(source_url.slice())) |src| { frame.source_url.deref(); diff --git a/src/bun.js/module_loader.zig b/src/bun.js/module_loader.zig index ce2fb312f1..e3e910dca4 100644 --- a/src/bun.js/module_loader.zig +++ b/src/bun.js/module_loader.zig @@ -154,11 +154,17 @@ inline fn jsSyntheticModule(comptime name: ResolvedSource.Tag, specifier: String /// /// This can technically fail if concurrent access across processes happens, or permission issues. /// Errors here should always be ignored. -fn dumpSource(specifier: string, printer: anytype) void { - dumpSourceString(specifier, printer.ctx.getWritten()); +fn dumpSource(vm: *VirtualMachine, specifier: string, printer: anytype) void { + dumpSourceString(vm, specifier, printer.ctx.getWritten()); } -fn dumpSourceString(specifier: string, written: []const u8) void { +fn dumpSourceString(vm: *VirtualMachine, specifier: string, written: []const u8) void { + dumpSourceStringFailiable(vm, specifier, written) catch |e| { + Output.debugWarn("Failed to dump source string: {}", .{e}); + }; +} + +fn dumpSourceStringFailiable(vm: *VirtualMachine, specifier: string, written: []const u8) !void { if (!Environment.isDebug) return; const BunDebugHolder = struct { @@ -182,10 +188,7 @@ fn dumpSourceString(specifier: string, written: []const u8) void { break :brk win_temp_buffer[0 .. temp.len + suffix.len :0]; }, }; - const dir = std.fs.cwd().makeOpenPath(base_name, .{}) catch |e| { - Output.debug("Failed to dump source string: {}", .{e}); - return; - }; + const dir = try std.fs.cwd().makeOpenPath(base_name, .{}); BunDebugHolder.dir = dir; break :dir dir; }; @@ -195,15 +198,43 @@ fn dumpSourceString(specifier: string, written: []const u8) void { else => "/".len, .windows => bun.path.windowsFilesystemRoot(dir_path).len, }; - var parent = dir.makeOpenPath(dir_path[root_len..], .{}) catch |e| { - Output.debug("Failed to dump source string: makeOpenPath({s}[{d}..]) {}", .{ dir_path, root_len, e }); - return; - }; + var parent = try dir.makeOpenPath(dir_path[root_len..], .{}); defer parent.close(); parent.writeFile(std.fs.path.basename(specifier), written) catch |e| { - Output.debug("Failed to dump source string: writeFile {}", .{e}); + Output.debugWarn("Failed to dump source string: writeFile {}", .{e}); return; }; + if (vm.source_mappings.get(specifier)) |mappings| { + const map_path = std.mem.concat(bun.default_allocator, u8, &.{ std.fs.path.basename(specifier), ".map" }) catch bun.outOfMemory(); + defer bun.default_allocator.free(map_path); + const file = try parent.createFile(map_path, .{}); + defer file.close(); + + const source_file = parent.readFileAlloc( + bun.default_allocator, + specifier, + std.math.maxInt(u64), + ) catch ""; + + var bufw = std.io.bufferedWriter(file.writer()); + const w = bufw.writer(); + try w.print( + \\{{ + \\ "version": 3, + \\ "file": {}, + \\ "sourceRoot": "", + \\ "sources": [{}], + \\ "sourcesContent": [{}], + \\ "names": [], + \\ "mappings": "{}" + \\}} + , .{ + js_printer.formatJSONStringUTF8(std.fs.path.basename(specifier)), + js_printer.formatJSONStringUTF8(specifier), + js_printer.formatJSONStringUTF8(source_file), + mappings.formatVLQs(), + }); + } } else { dir.writeFile(std.fs.path.basename(specifier), written) catch return; } @@ -562,7 +593,7 @@ pub const RuntimeTranspilerStore = struct { }) catch {}; if (comptime Environment.dump_source) { - dumpSourceString(specifier, entry.output_code.byteSlice()); + dumpSourceString(vm, specifier, entry.output_code.byteSlice()); } this.resolved_source = ResolvedSource{ @@ -662,7 +693,7 @@ pub const RuntimeTranspilerStore = struct { } if (comptime Environment.dump_source) { - dumpSource(specifier, &printer); + dumpSource(this.vm, specifier, &printer); } const duped = String.createUTF8(specifier); @@ -1433,7 +1464,7 @@ pub const ModuleLoader = struct { } if (comptime Environment.dump_source) { - dumpSource(specifier, &printer); + dumpSource(jsc_vm, specifier, &printer); } const commonjs_exports = try bun.default_allocator.alloc(ZigString, parse_result.ast.commonjs_export_names.len); @@ -1809,7 +1840,7 @@ pub const ModuleLoader = struct { }) catch {}; if (comptime Environment.allow_assert) { - dumpSourceString(specifier, entry.output_code.byteSlice()); + dumpSourceString(jsc_vm, specifier, entry.output_code.byteSlice()); } return ResolvedSource{ @@ -1911,7 +1942,7 @@ pub const ModuleLoader = struct { }; if (comptime Environment.dump_source) { - dumpSource(specifier, &printer); + dumpSource(jsc_vm, specifier, &printer); } const commonjs_exports = try bun.default_allocator.alloc(ZigString, parse_result.ast.commonjs_export_names.len); diff --git a/src/bundler/bundle_v2.zig b/src/bundler/bundle_v2.zig index 96961bd275..eee32b3c2b 100644 --- a/src/bundler/bundle_v2.zig +++ b/src/bundler/bundle_v2.zig @@ -6999,22 +6999,35 @@ const LinkerContext = struct { const trace = tracer(@src(), "generateSourceMapForChunk"); defer trace.end(); - var j = StringJoiner{ - .allocator = worker.allocator, - }; + var j = StringJoiner{ .allocator = worker.allocator }; const sources = c.parse_graph.input_files.items(.source); const quoted_source_map_contents = c.graph.files.items(.quoted_source_contents); - var source_index_to_sources_index = std.AutoHashMap(u32, u32).init(worker.allocator); - defer source_index_to_sources_index.deinit(); - var next_source_index: u32 = 0; + // Entries in `results` do not 1:1 map to source files, the mapping + // is actually many to one, where a source file can have multiple chunks + // in the sourcemap. + // + // This hashmap is going to map: + // `source_index` (per compilation) in a chunk + // --> + // Which source index in the generated sourcemap, referred to + // as the "mapping source index" within this function to be distinct. + var source_id_map = std.AutoArrayHashMap(u32, i32).init(worker.allocator); + defer source_id_map.deinit(); + const source_indices = results.items(.source_index); - j.pushStatic("{\n \"version\": 3,\n \"sources\": ["); + j.pushStatic( + \\{ + \\ "version": 3, + \\ "sources": [ + ); if (source_indices.len > 0) { { - var path = sources[source_indices[0]].path; + const index = source_indices[0]; + var path = sources[index].path; + try source_id_map.putNoClobber(index, 0); if (path.isFile()) { const rel_path = try std.fs.path.relative(worker.allocator, chunk_abs_dir, path.text); @@ -7025,36 +7038,49 @@ const LinkerContext = struct { quote_buf = try js_printer.quoteForJSON(path.pretty, quote_buf, false); j.pushStatic(quote_buf.list.items); // freed by arena } - if (source_indices.len > 1) { - for (source_indices[1..]) |index| { - var path = sources[index].path; - if (path.isFile()) { - const rel_path = try std.fs.path.relative(worker.allocator, chunk_abs_dir, path.text); - path.pretty = rel_path; - } + var next_mapping_source_index: i32 = 1; + for (source_indices[1..]) |index| { + const gop = try source_id_map.getOrPut(index); + if (gop.found_existing) continue; - var quote_buf = try MutableString.init(worker.allocator, path.pretty.len + ", ".len + 2); - quote_buf.appendAssumeCapacity(", "); - quote_buf = try js_printer.quoteForJSON(path.pretty, quote_buf, false); - j.pushStatic(quote_buf.list.items); // freed by arena + gop.value_ptr.* = next_mapping_source_index; + next_mapping_source_index += 1; + + var path = sources[index].path; + + if (path.isFile()) { + const rel_path = try std.fs.path.relative(worker.allocator, chunk_abs_dir, path.text); + path.pretty = rel_path; } + + var quote_buf = try MutableString.init(worker.allocator, path.pretty.len + ", ".len + 2); + quote_buf.appendAssumeCapacity(", "); + quote_buf = try js_printer.quoteForJSON(path.pretty, quote_buf, false); + j.pushStatic(quote_buf.list.items); // freed by arena } } - j.pushStatic("],\n \"sourcesContent\": ["); - if (source_indices.len > 0) { + j.pushStatic( + \\], + \\ "sourcesContent": [ + ); + + const source_indicies_for_contents = source_id_map.keys(); + if (source_indicies_for_contents.len > 0) { j.pushStatic("\n "); - j.pushStatic(quoted_source_map_contents[source_indices[0]]); + j.pushStatic(quoted_source_map_contents[source_indicies_for_contents[0]]); - if (source_indices.len > 1) { - for (source_indices[1..]) |index| { - j.pushStatic(",\n "); - j.pushStatic(quoted_source_map_contents[index]); - } + for (source_indicies_for_contents[1..]) |index| { + j.pushStatic(",\n "); + j.pushStatic(quoted_source_map_contents[index]); } } - j.pushStatic("\n ],\n \"mappings\": \""); + j.pushStatic( + \\ + \\ ], + \\ "mappings": " + ); const mapping_start = j.len; var prev_end_state = sourcemap.SourceMapState{}; @@ -7062,14 +7088,11 @@ const LinkerContext = struct { const source_map_chunks = results.items(.source_map_chunk); const offsets = results.items(.generated_offset); for (source_map_chunks, offsets, source_indices) |chunk, offset, current_source_index| { - const res = try source_index_to_sources_index.getOrPut(current_source_index); - if (res.found_existing) continue; - res.value_ptr.* = next_source_index; - const source_index = @as(i32, @intCast(next_source_index)); - next_source_index += 1; + const mapping_source_index = source_id_map.get(current_source_index) orelse + unreachable; // the pass above during printing of "sources" must add the index var start_state = sourcemap.SourceMapState{ - .source_index = source_index, + .source_index = mapping_source_index, .generated_line = offset.lines, .generated_column = offset.columns, }; @@ -7081,7 +7104,7 @@ const LinkerContext = struct { try sourcemap.appendSourceMapChunk(&j, worker.allocator, prev_end_state, start_state, chunk.buffer.list.items); prev_end_state = chunk.end_state; - prev_end_state.source_index = source_index; + prev_end_state.source_index = mapping_source_index; prev_column_offset = chunk.final_generated_column; if (prev_end_state.generated_line == 0) { diff --git a/src/js_printer.zig b/src/js_printer.zig index cfe0cd2530..6f473dfacc 100644 --- a/src/js_printer.zig +++ b/src/js_printer.zig @@ -343,9 +343,21 @@ const JSONFormatter = struct { } }; +const JSONFormatterUTF8 = struct { + input: []const u8, + + pub fn format(self: JSONFormatterUTF8, comptime _: []const u8, _: std.fmt.FormatOptions, writer: anytype) !void { + try writeJSONString(self.input, @TypeOf(writer), writer, .utf8); + } +}; + /// Expects latin1 pub fn formatJSONString(text: []const u8) JSONFormatter { - return JSONFormatter{ .input = text }; + return .{ .input = text }; +} + +pub fn formatJSONStringUTF8(text: []const u8) JSONFormatterUTF8 { + return .{ .input = text }; } pub fn writeJSONString(input: []const u8, comptime Writer: type, writer: Writer, comptime encoding: strings.Encoding) !void { @@ -371,13 +383,15 @@ pub fn writeJSONString(input: []const u8, comptime Writer: type, writer: Writer, } else break :brk strings.latin1ToCodepointAssumeNotASCII(char, i32); }; if (canPrintWithoutEscape(i32, c, false)) { - const remain = text[@as(usize, width)..]; + const remain = text[width..]; if (encoding != .utf8 and width > 0) { var codepoint_bytes: [4]u8 = undefined; std.mem.writeInt(i32, &codepoint_bytes, c, .little); try writer.writeAll( codepoint_bytes[0..strings.encodeWTF8Rune(codepoint_bytes[0..4], c)], ); + } else if (encoding == .utf8) { + try writer.writeAll(text[0..width]); } if (strings.indexOfNeedsEscape(remain)) |j| { diff --git a/src/sourcemap/sourcemap.zig b/src/sourcemap/sourcemap.zig index 1e59b552ad..ac1161f73e 100644 --- a/src/sourcemap/sourcemap.zig +++ b/src/sourcemap/sourcemap.zig @@ -286,6 +286,7 @@ pub const Mapping = struct { base_filename, lookup.source_map.underlying_provider.load_hint, .{ .source_only = @intCast(index) }, + null, )) |parsed| if (parsed.source_contents) |contents| break :bytes contents; @@ -634,6 +635,46 @@ pub const Mapping = struct { allocator.destroy(this); } + + pub fn writeVLQs(map: ParsedSourceMap, writer: anytype) !void { + var last_col: i32 = 0; + var last_src: i32 = 0; + var last_ol: i32 = 0; + var last_oc: i32 = 0; + var current_line: i32 = 0; + for ( + map.mappings.items(.generated), + map.mappings.items(.original), + map.mappings.items(.source_index), + 0.., + ) |gen, orig, source_index, i| { + if (current_line != gen.lines) { + assert(gen.lines > current_line); + const inc = gen.lines - current_line; + try writer.writeByteNTimes(';', @intCast(inc)); + current_line = gen.lines; + last_col = 0; + } else if (i != 0) { + try writer.writeByte(','); + } + try encodeVLQ(gen.columns - last_col).writeTo(writer); + last_col = gen.columns; + try encodeVLQ(source_index - last_src).writeTo(writer); + last_src = source_index; + try encodeVLQ(orig.lines - last_ol).writeTo(writer); + last_ol = orig.lines; + try encodeVLQ(orig.columns - last_oc).writeTo(writer); + last_oc = orig.columns; + } + } + + pub fn formatVLQs(map: *const ParsedSourceMap) std.fmt.Formatter(formatVLQsImpl) { + return .{ .data = map }; + } + + fn formatVLQsImpl(map: *const ParsedSourceMap, comptime _: []const u8, _: std.fmt.FormatOptions, w: anytype) !void { + try map.writeVLQs(w); + } }; }; @@ -662,7 +703,7 @@ pub const SourceProviderMap = opaque { extern fn ZigSourceProvider__getSourceSlice(*SourceProviderMap) bun.String; fn findSourceMappingURL(comptime T: type, source: []const T, alloc: std.mem.Allocator) ?bun.JSC.ZigString.Slice { - const needle = comptime bun.strings.literal(T, "//# sourceMappingURL="); + const needle = comptime bun.strings.literal(T, "\n//# sourceMappingURL="); const found = bun.strings.indexOfT(T, source, needle) orelse return null; const end = std.mem.indexOfScalarPos(T, source, found + needle.len, '\n') orelse source.len; const url = std.mem.trimRight(T, source[found + needle.len .. end], &.{ ' ', '\r' }); @@ -682,12 +723,15 @@ pub const SourceProviderMap = opaque { source_filename: []const u8, load_hint: SourceMapLoadHint, result: ParseUrlResultHint, + log: ?*Logger.Log, ) ?SourceMap.ParseUrl { var sfb = std.heap.stackFallback(65536, bun.default_allocator); var arena = bun.ArenaAllocator.init(sfb.get()); defer arena.deinit(); const new_load_hint: SourceMapLoadHint, const parsed = parsed: { + var inline_err: ?anyerror = null; + // try to get an inline source map if (load_hint != .is_external_map) try_inline: { const source = ZigSourceProvider__getSourceSlice(provider); @@ -709,13 +753,8 @@ pub const SourceProviderMap = opaque { found_url.slice(), result, ) catch |err| { - bun.Output.warn("Could not decode sourcemap in '{s}': {s}", .{ - source_filename, - @errorName(err), - }); - // Disable the "try using --sourcemap=external" hint - bun.JSC.SavedSourceMap.MissingSourceMapNoteInfo.seen_invalid = true; - return null; + inline_err = err; + break :try_inline; }, }; } @@ -742,10 +781,17 @@ pub const SourceProviderMap = opaque { data, result, ) catch |err| { - bun.Output.warn("Could not decode sourcemap '{s}': {s}", .{ - source_filename, - @errorName(err), - }); + if (log) |l| + l.addWarningFmt( + null, + Logger.Loc.Empty, + bun.default_allocator, + "Could not decode sourcemap '{s}': {s}", + .{ + source_filename, + @errorName(err), + }, + ) catch {}; // Disable the "try using --sourcemap=external" hint bun.JSC.SavedSourceMap.MissingSourceMapNoteInfo.seen_invalid = true; return null; @@ -753,6 +799,23 @@ pub const SourceProviderMap = opaque { }; } + if (inline_err) |err| { + if (log) |l| + l.addWarningFmt( + null, + Logger.Loc.Empty, + bun.default_allocator, + "Could not decode sourcemap in '{s}': {s}", + .{ + source_filename, + @errorName(err), + }, + ) catch {}; + // Disable the "try using --sourcemap=external" hint + bun.JSC.SavedSourceMap.MissingSourceMapNoteInfo.seen_invalid = true; + return null; + } + return null; }; if (parsed.map) |ptr| { diff --git a/test/bundler/bundler_edgecase.test.ts b/test/bundler/bundler_edgecase.test.ts index 56ef384136..78f3120043 100644 --- a/test/bundler/bundler_edgecase.test.ts +++ b/test/bundler/bundler_edgecase.test.ts @@ -1104,6 +1104,38 @@ describe("bundler", () => { }, }, }); + // chunk-concat forgets to de-duplicate source indicies + // chunk-concat ignores all but the first instance of a chunk + itBundled("edgecase/EmitInvalidSourceMap2", { + files: { + "/entry.js": ` + const a = new TextEncoder(); + console.log('hey!') + const d = new TextEncoder(); + + const b = { hello: 'world' }; + + const c = new Set([ + ]); + console.log('hey!') + console.log('hey!') + console.log('hey!') + console.log('hey!') + `, + }, + outdir: "/out", + sourceMap: "external", + minifySyntax: true, + minifyIdentifiers: true, + minifyWhitespace: true, + snapshotSourceMap: { + "entry.js.map": { + files: ["../entry.js"], + mappingsExactMatch: + "AACQ,QAAQ,IAAI,MAAM,EAOlB,QAAQ,IAAI,MAAM,EAClB,QAAQ,IAAI,MAAM,EAClB,QAAQ,IAAI,MAAM,EAClB,QAAQ,IAAI,MAAM", + }, + }, + }); // TODO(@paperdave): test every case of this. I had already tested it manually, but it may break later const requireTranspilationListESM = [ diff --git a/test/bundler/expectBundled.ts b/test/bundler/expectBundled.ts index 459ebbea59..028b63b94a 100644 --- a/test/bundler/expectBundled.ts +++ b/test/bundler/expectBundled.ts @@ -275,9 +275,9 @@ export interface SourceMapTests { * for byte snapshots will not be sustainable. Instead, we will sample a few mappings to make sure * the map is correct. This can be used to test for a single mapping. */ - mappings: MappingSnapshot[]; + mappings?: MappingSnapshot[]; /** For small files it is acceptable to inline all of the mappings. */ - mappingsExactMatch: string; + mappingsExactMatch?: string; } /** Keep in mind this is an array/tuple, NOT AN OBJECT. This keeps things more consise */ @@ -1309,7 +1309,7 @@ for (const [key, blob] of build.outputs) { }); const map_tests = snapshotSourceMap?.[path.basename(file)]; if (map_tests) { - expect(parsed.sources.map(a => a.replaceAll("\\", "/"))).toEqual(map_tests.files); + expect(parsed.sources.map((a: string) => a.replaceAll("\\", "/"))).toEqual(map_tests.files); for (let i = 0; i < parsed.sources; i++) { const source = parsed.sources[i]; const sourcemap_content = parsed.sourceContent[i];