Repository navigation
sourcemap: ignore a <file>.map whose debugId is not the debug id of the file
#47628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
robobun
wants to merge
9
commits into
main
Choose a base branch
from
robobun/fba4fb53/map-debug-id
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
3493ac1
sourcemap: do not use a `<file>.map` whose debugId is not the debug i…
robobun 34172e1
Merge remote-tracking branch 'origin/main' into robobun/fba4fb53/map-…
robobun 89a1045
sourcemap: scope the docs sentence to the first read and to maps that…
robobun a3e0215
test: read a quoted path in a warning the same way when its backslash…
robobun 08f409e
Merge remote-tracking branch 'origin/main' into robobun/fba4fb53/map-…
robobun f653a70
test: a debug id line under a `#!` line, and a linked build with a pu…
robobun 3461927
sourcemap: compare a debug id that is not a UUID as it is
robobun c4c7751
test: hold the new debug id tests in the AOT runner suites
robobun 29fd0e9
ci: retrigger
robobun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,122 @@ | ||
| //! The debug id of a bundle: `bun build` writes it into the file (`//# debugId=<id>`) and into its map (`"debugId"`). | ||
|
|
||
| use bun_core::strings::{self, PathByte}; | ||
| use bun_core::{Utf8Bytes, fmt}; | ||
| use core::fmt::{Display, Formatter}; | ||
|
|
||
| pub(crate) struct Check<'a> { | ||
| pub(crate) file_id: &'a [u8], | ||
| pub(crate) source_filename: &'a [u8], | ||
| } | ||
|
|
||
| impl Check<'_> { | ||
| /// A UUID is 32 hex digits, written with or without dashes and in either case. Any other id is compared as it is. | ||
| pub(crate) fn is_id_of_file(&self, map_id: &[u8]) -> bool { | ||
| fn digits(id: &[u8]) -> impl Iterator<Item = u8> + '_ { | ||
| id.iter() | ||
| .filter(|unit| **unit != b'-') | ||
| .map(u8::to_ascii_lowercase) | ||
| } | ||
| let is_uuid = |id: &[u8]| { | ||
| digits(id).count() == 32 && digits(id).all(|digit| digit.is_ascii_hexdigit()) | ||
| }; | ||
| self.file_id == map_id || (is_uuid(self.file_id) && digits(self.file_id).eq(digits(map_id))) | ||
| } | ||
|
|
||
| #[cold] | ||
| pub(crate) fn warn(&self, map_id: &[u8]) { | ||
| let map_path = [self.source_filename, b".map"].concat(); | ||
| bun_core::warn!( | ||
| "Ignoring sourcemap {}: its debugId {} is not the debugId {} that {} had when it was last loaded", | ||
| fmt::EscapeControlChars(fmt::quote(&map_path)), | ||
| Shown(map_id), | ||
| Shown(self.file_id), | ||
| fmt::EscapeControlChars(fmt::quote(self.source_filename)), | ||
| ); | ||
| bun_core::note!( | ||
| "positions in this file stay unmapped until Bun loads it again beside a map with the same debugId" | ||
| ); | ||
| // `note!` does not flush, and a Worker can end with no other flush. | ||
| bun_core::output::flush(); | ||
| crate::SavedSourceMap::MissingSourceMapNoteInfo::set_seen_invalid(true); | ||
| } | ||
| } | ||
|
|
||
| struct Shown<'a>(&'a [u8]); | ||
|
|
||
| impl Shown<'_> { | ||
| const MAX: usize = 64; | ||
| } | ||
|
|
||
| impl Display for Shown<'_> { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> core::fmt::Result { | ||
| let shown = &self.0[..self.0.len().min(Self::MAX)]; | ||
| write!(f, "{}", fmt::EscapeControlChars(fmt::quote(shown)))?; | ||
| if shown.len() < self.0.len() { | ||
| f.write_str("...")?; | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
|
|
||
| /// The id on the `//# debugId=` line of a `// @bun` file: the last line, or the line above a last `//# sourceMappingURL=` line. | ||
| pub(crate) fn of_file(source: &bun_core::String) -> Option<Utf8Bytes<'_>> { | ||
| let id = if source.is_8bit() { | ||
| Utf8Bytes::Borrowed(value_of_id_line(source.latin1())?) | ||
| } else { | ||
| Utf8Bytes::Owned(strings::to_utf8_alloc(value_of_id_line(source.utf16())?)) | ||
| }; | ||
| // An id is one run of ASCII with no blank and no quote: V8 reads the value of a magic comment this way. | ||
| let is_id = !id.is_empty() | ||
| && id | ||
| .iter() | ||
| .all(|unit| unit.is_ascii_graphic() && !matches!(*unit, b'"' | b'\'')); | ||
| is_id.then_some(id) | ||
| } | ||
|
|
||
| fn value_of_id_line<T: PathByte>(text: &[T]) -> Option<&[T]> { | ||
| // Text that does not start as the file does is not the file: for a replaced `Module.wrapper` the loader swaps the first line of a CommonJS file and cuts its end. | ||
| let first_line = match strip_prefix(text, b"#!") { | ||
| Some(_) => &text[T::index_of_unit(text, T::from_u8(b'\n'))? + 1..], | ||
| None => text, | ||
| }; | ||
| strip_prefix(first_line, b"// @bun")?; | ||
|
|
||
| let (above, last) = split_last_line(text); | ||
| let line = match strip_prefix(last, b"//# sourceMappingURL=") { | ||
| Some(_) => split_last_line(above).1, | ||
| None => last, | ||
| }; | ||
| let mut value = strip_prefix(line, b"//# debugId=")?; | ||
| let blank = [T::from_u8(b' '), T::from_u8(b'\t')]; | ||
| while let [first, rest @ ..] = value { | ||
| if !blank.contains(first) { | ||
| break; | ||
| } | ||
| value = rest; | ||
| } | ||
| Some(value) | ||
| } | ||
|
|
||
| fn split_last_line<T: PathByte>(mut text: &[T]) -> (&[T], &[T]) { | ||
| let blank = (*b"\n\r \t").map(T::from_u8); | ||
| while let [rest @ .., last] = text { | ||
| if !blank.contains(last) { | ||
| break; | ||
| } | ||
| text = rest; | ||
| } | ||
| match strings::last_index_of_char_t(text, T::from_u8(b'\n')) { | ||
| Some(line_end) => (&text[..line_end], &text[line_end + 1..]), | ||
| None => (&text[..0], text), | ||
| } | ||
| } | ||
|
|
||
| fn strip_prefix<'a, T: PathByte>(text: &'a [T], prefix: &[u8]) -> Option<&'a [T]> { | ||
| let (start, rest) = text.split_at_checked(prefix.len())?; | ||
| start | ||
| .iter() | ||
| .zip(prefix) | ||
| .all(|(unit, byte)| *unit == T::from_u8(*byte)) | ||
| .then_some(rest) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 (optional) Developers running
bun build --watch --outfilewithbun --hotnow get a spuriouswarn: Ignoring sourcemapplus unmapped stack traces on some reloads.--outfilewrites the bundle before its.map(build_command.rs:1190 walks output_files, where maps follow chunks), so a reload that errors in that window reads the previous map beside the new bundle and src/sourcemap/lib.rs:543 refuses it for the whole load. Fix: make the bundle and its sidecar converge for a hot reloader: write the.mapbefore the bundle in the--outfilewriter, as writeOutputFilesToDisk.rs:325 does for--outdir, or re-read a refused map once when it changes on disk. The PR calls this accepted (41 of 519 rebuilds); on the base branch the old map applied silently.Why this was flagged
Trigger:
bun build --target=bun --sourcemap=external --watch --outfile=out.jsrunning besidebun --hot out.js, and the reloaded code throws (or readserror.stack) before the watcher's second write lands. In--outfilemode the in-memory build path fills output_files with chunk entries at indices below index_for_sourcemaps_and_bytecode, and build_command.rs:1190 writes them in that order, so out.js lands before out.js.map. The hot reload registers the new provider, and the first stack lookup reads the still-old out.js.map; parse_json_impl at src/sourcemap/lib.rs:923-934 finds the old debugId, and lib.rs:543 returns None, after which SavedSourceMap.rs:329 removes the entry so that load never maps again. On the base branch the same reload applied the old map (wrong positions, no message); after this change it prints the warning and shows generated positions until the next reload. The--outdirwriter writes the map first (writeOutputFilesToDisk.rs:325) and does not hit this.Verification:
--outfilewithout--outdirleavesoutput_dirempty (build_command.rs:332-335), so generateChunksInParallel.rs:862-871 takes the in-memory branch; build_command.rs:1190-1191 writes them in that order: bundle first,.mapsecond. src/sourcemap/lib.rs:539-543:Err(crate::Error::DebugIdMismatch) => return None. src/jsc/SavedSourceMap.rs:327-338: onNonethe provider entry is removed, so the map is not re-read for that load.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed, and it is in the Notes under "Who loses a map that main applies". An inotify trace shows the order:
--outfilewritesout.jsand thenout.js.map, and--outdirwrites the map first.I did not change the writer in this PR. The order of the
--outfilewrites is a producer bug by itself: on main the same reload maps the new bundle with the old map and prints nothing. I passed it on as its own change (write the map before the bundle in the--outfileloop ofbuild_command.rs). #41879 moves--outfileto the writer that--outdiruses, which has that order.I did not add a second read of a refused map. A lookup that fails is final for that load on main too (
SavedSourceMapremoves the entry), and the next reload reads the map again, which a test here covers. I leave this thread open for the maintainer decision.