Skip to content

Conversation

@kdy1
Copy link
Contributor

@kdy1 kdy1 commented Dec 4, 2025

Summary

  • Integrate changes from upstream getsentry/rust-sourcemap v9.3.0 release
  • Update CHANGELOG.md with upstream changes
  • Merge improvements to decoder, encoder, and type definitions
  • Update test suite with upstream enhancements

Conflict Resolution

Conflicts were resolved by keeping swc-project fork modifications:

  • Cargo.toml: Maintained package name as swc_sourcemap and current version
  • src/sourceview.rs: Kept BytesStr-based implementation instead of Arc

Test plan

  • Verify all existing tests pass
  • Check that the package name remains swc_sourcemap
  • Confirm BytesStr implementation still works correctly

🤖 Generated with Claude Code

kdy1 and others added 13 commits June 5, 2025 15:31
In case of most transforms (for ECMAScript), the original input source
map is used only to adjust mappings, and other fields are kept intact.

The _lazy_ variant source maps are source maps used only to call
`adjust_mappings`. It does not deserialize everything. Instead, it only
deserializes the fields required to calculate the new mappings.

This is done in the name of the performance, and the justification for
introducing a new type is at
vercel/next.js#80177 (comment)
<img width="881" alt="image"
src="https://github.com/user-attachments/assets/7ca4b084-ba46-4384-ac25-fefeae0e2513"
/>
I added some `skip_serializing_if` to `RawSourceMap`
`BytesStr` is a more efficient variant of `Arc<str>`

Since swc-project/swc#10580, SWC also uses it to store the source map strings
cherry pick getsentry#130

Co-authored-by: Sebastian Zivota <[email protected]>
cherry-pick getsentry#132

---------

Co-authored-by: Sebastian Zivota <[email protected]>
Caused invalid source content:
<img width="2018" height="721" alt="image"
src="https://github.com/user-attachments/assets/dca54abd-fec5-4f4e-88ac-67e2d255dbfd"
/>
Integrate changes from upstream getsentry/rust-sourcemap v9.3.0 release:
- Update CHANGELOG.md with upstream changes
- Merge improvements to decoder, encoder, and type definitions
- Update test suite with upstream enhancements

Conflicts resolved by keeping swc-project fork modifications:
- Cargo.toml: Maintain package name as 'swc_sourcemap' and current version
- src/sourceview.rs: Keep BytesStr-based implementation

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <[email protected]>
Copilot AI review requested due to automatic review settings December 4, 2025 10:47
Comment on lines +214 to +216
lines.push(unsafe {
str::from_utf8_unchecked(slice::from_raw_parts(rv.as_ptr(), rv.len()))
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Lifetime violation: &'static str in cache points to non-static data.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The SourceView struct stores &'static str references in its lines: Mutex<Vec<&'static str>> cache. These references actually point to data owned by self.source, which does not have a 'static lifetime. This creates a lifetime violation where the cached &str pointers can become dangling if self.source is dropped or re-initialized while the cached lines are still in use, for example, when a SourceView instance is cloned and processed_until resets.

💡 Suggested Fix

Change the lines cache to store BytesStr or String instead of &'static str to ensure owned data or correctly managed lifetimes. Alternatively, ensure the SourceView struct itself holds ownership of the data for the lifetime of the cached references.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/sourceview.rs#L214-L216

Potential issue: The `SourceView` struct stores `&'static str` references in its `lines:
Mutex<Vec<&'static str>>` cache. These references actually point to data owned by
`self.source`, which does not have a `'static` lifetime. This creates a lifetime
violation where the cached `&str` pointers can become dangling if `self.source` is
dropped or re-initialized while the cached lines are still in use, for example, when a
`SourceView` instance is cloned and `processed_until` resets.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 5461981

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR merges upstream getsentry/rust-sourcemap v9.3.0 into the swc-project fork, bringing improvements to decoder, encoder, and type definitions while maintaining fork-specific modifications. The merge involves significant type system changes from Arc<str> to BytesStr and adds a new lazy decoding module for better performance.

Key changes:

  • Replaced Arc<str> with BytesStr throughout the codebase for string storage
  • Added new lazy module for lazy sourcemap decoding with minimal field deserialization
  • Implemented adjust_mappings_from_multiple function for combining multiple sourcemaps
  • Refactored SourceView internal line storage mechanism using atomic operations

Critical Issues Found:

  1. Lifetime soundness bug in SourceView where &'static str references are unsafely created from non-static data
  2. Rust edition incompatibility with use<'_, 'a> syntax requiring Rust 2024 but project uses edition 2018
  3. Invalid API call to .advance() method that doesn't exist on BytesStr

Reviewed changes

Copilot reviewed 30 out of 33 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/*.rs Updated imports from sourcemap to swc_sourcemap and adjusted test assertions for BytesStr API changes
tests/fixtures/adjust_mappings_from_multiple/* Added test fixtures for new multi-sourcemap adjustment feature
src/types.rs Refactored to use BytesStr, added adjust_mappings_from_multiple function, updated all string APIs
src/sourceview.rs Refactored line caching with atomic operations (contains critical lifetime bug)
src/lazy/mod.rs New module for lazy sourcemap decoding (contains Rust 2024 syntax issue)
src/builder.rs Updated to use BytesStr APIs (contains invalid method call)
src/*.rs (other) Updated imports and type signatures to use BytesStr
examples/*.rs Updated imports to swc_sourcemap
Cargo.toml Updated package metadata, version, and added bytes-str dependency
README.md Updated example code to use swc_sourcemap
.github/workflows/ci.yml Updated branch names and workflow triggers

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}
if source.starts_with(&prefix) {
*source = source[prefix.len()..].into();
source.advance(prefix.len());
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The advance method is being called on BytesStr, but this method may not exist in the bytes-str crate version 0.2.4. This appears to be an incorrect API usage. Consider replacing this with: *source = source[prefix.len()..].into(); or using the appropriate BytesStr API to skip a prefix.

Suggested change
source.advance(prefix.len());
*source = source[prefix.len()..].into();

Copilot uses AI. Check for mistakes.

#[derive(Debug, Clone, Copy, Serialize)]
#[serde(untagged)]
pub(crate) enum MaybeRawValue<'a, T> {
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] MaybeRawValue is declared as pub(crate) in the lazy module, which means it should be accessible within the crate, including in tests within src/types.rs. However, to ensure proper encapsulation, consider whether this type should be made fully public (pub) if it's needed in tests, or if the test should be restructured to avoid using it directly.

Suggested change
pub(crate) enum MaybeRawValue<'a, T> {
pub enum MaybeRawValue<'a, T> {

Copilot uses AI. Check for mistakes.
line_end_offsets: Mutex<Vec<LineEndOffset>>,
pub(crate) source: BytesStr,
processed_until: AtomicUsize,
lines: Mutex<Vec<&'static str>>,
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lines field stores &'static str references, but the get_line method creates these references from the BytesStr source using unsafe code (lines 214-216). This is unsound because the references don't actually have a static lifetime - they're tied to the lifetime of the BytesStr in self.source. If the SourceView is dropped, the BytesStr is dropped, but the &'static str references in lines would become dangling pointers.

This should be changed to store references with a lifetime tied to self, such as lines: Mutex<Vec<&str>> with appropriate lifetime annotations on the struct, or use indices into the source instead of raw pointers.

Suggested change
lines: Mutex<Vec<&'static str>>,
lines: Mutex<Vec<(usize, usize)>>,

Copilot uses AI. Check for mistakes.
self.file.as_mut().map(|f| &*f.as_data())
}

pub fn sources(&mut self) -> impl Iterator<Item = &'_ BytesStr> + use<'_, 'a> {
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use<'_, 'a> syntax in the return type is a Rust 2024 edition feature that may not be compatible with the edition specified in Cargo.toml (edition = "2018"). This will cause compilation errors. Consider either updating the edition to "2024" or using a different return type like impl Iterator<Item = &'_ BytesStr> + '_.

Suggested change
pub fn sources(&mut self) -> impl Iterator<Item = &'_ BytesStr> + use<'_, 'a> {
pub fn sources(&mut self) -> impl Iterator<Item = &'_ BytesStr> + '_ {

Copilot uses AI. Check for mistakes.
return fn();
} //# sourceMappingURL=sourcemapped.js.map
;
runExternalSourceMapped();
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Callee is not a function: it has type undefined.

Suggested change
runExternalSourceMapped();
runExternalSourceMapped(() => {});

Copilot uses AI. Check for mistakes.
@loewenheim
Copy link
Contributor

Sorry, what exactly is this meant to do? master is already at 9.3.0.

@kdy1
Copy link
Contributor Author

kdy1 commented Dec 4, 2025

Oh sorry... This is the same as #135 (PR created towards wrong repository by Claude Code).

I missed this PR. My bad. Sorry

@kdy1 kdy1 closed this Dec 4, 2025
@loewenheim
Copy link
Contributor

No worries!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants