From 152dc8dea851dba40b38b1d58f04f20c28447cf9 Mon Sep 17 00:00:00 2001 From: Will Binns-Smith Date: Wed, 16 Oct 2024 15:15:44 -0700 Subject: [PATCH] Remove SourceMap wrapper providing Send+Sync Previously, the sourcemap crate did not implement Send+Sync on `DecodedMap`, so we created unsafe wrappers around it and avoided unsafe behavior. Since https://github.com/getsentry/rust-sourcemap/pull/80, this is no longer necessary. Test Plan: CI --- Cargo.lock | 3 +- Cargo.toml | 3 + .../turbopack-core/src/source_map/mod.rs | 117 ++++-------------- .../crates/turbopack-ecmascript/src/parse.rs | 4 +- 4 files changed, 32 insertions(+), 95 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b5b756b0077f8d..5d7440a06afabf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6147,8 +6147,7 @@ dependencies = [ [[package]] name = "sourcemap" version = "9.0.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dab08a862c70980b8e23698b507e272317ae52a608a164a844111f5372374f1f" +source = "git+https://github.com/wbinnssmith/rust-sourcemap?branch=wbinnssmith%2Fdecoded-serde#bc90cfd4c30a12cb901c1e03221e399c33d85d87" dependencies = [ "base64-simd", "bitvec", diff --git a/Cargo.toml b/Cargo.toml index 45c5e6ba9e284a..9635478a3724f9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -216,3 +216,6 @@ unsize = "1.1.0" url = "2.2.2" urlencoding = "2.1.2" webbrowser = "0.8.7" + +[patch.crates-io] +sourcemap = { git = "https://github.com/wbinnssmith/rust-sourcemap", branch = "wbinnssmith/decoded-serde" } diff --git a/turbopack/crates/turbopack-core/src/source_map/mod.rs b/turbopack/crates/turbopack-core/src/source_map/mod.rs index 322980c8b9d1d0..e75c870cc26422 100644 --- a/turbopack/crates/turbopack-core/src/source_map/mod.rs +++ b/turbopack/crates/turbopack-core/src/source_map/mod.rs @@ -2,9 +2,8 @@ use std::{borrow::Cow, io::Write, ops::Deref, sync::Arc}; use anyhow::Result; use once_cell::sync::Lazy; -use ref_cast::RefCast; use regex::Regex; -use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use serde::{Deserialize, Serialize}; use sourcemap::{DecodedMap, SourceMap as RegularMap, SourceMapBuilder, SourceMapIndex}; use turbo_tasks::{RcStr, TryJoinIterExt, ValueToString, Vc}; use turbo_tasks_fs::{ @@ -217,7 +216,7 @@ impl SourceMap { } impl SourceMap { - pub async fn to_source_map(&self) -> Result> { + pub async fn to_source_map(&self) -> Result> { Ok(match self { Self::Decoded(m) => m.map.clone(), Self::Sectioned(m) => { @@ -229,13 +228,11 @@ impl SourceMap { sourcemap::SourceMapSection::new( (s.offset.line as u32, s.offset.column as u32), None, - Some(s.map.0.clone()), + Some((*s.map.clone()).clone()), ) }) .collect::>(); - Arc::new(CrateMapWrapper(DecodedMap::Index(SourceMapIndex::new( - None, sections, - )))) + Arc::new(DecodedMap::Index(SourceMapIndex::new(None, sections))) } }) } @@ -261,7 +258,7 @@ impl SourceMap { let rope = match &*this { SourceMap::Decoded(r) => { let mut bytes = vec![]; - r.0.to_writer(&mut bytes)?; + r.to_writer(&mut bytes)?; Rope::from(bytes) } @@ -391,10 +388,9 @@ impl SourceMap { ) } async fn regular_map_with_resolved_sources( - map: &RegularMapWrapper, + map: &RegularMap, origin: Vc, ) -> Result { - let map = &map.0; let file = map.get_file().map(Arc::::from); let tokens = map.tokens().map(|t| t.get_raw_token()).collect(); let names = map.names().map(Arc::::from).collect(); @@ -420,12 +416,11 @@ impl SourceMap { )) } async fn decoded_map_with_resolved_sources( - map: &CrateMapWrapper, + map: &DecodedMap, origin: Vc, - ) -> Result { - Ok(CrateMapWrapper(match &map.0 { + ) -> Result { + Ok(match &map { DecodedMap::Regular(map) => { - let map = RegularMapWrapper::ref_cast(map); DecodedMap::Regular(regular_map_with_resolved_sources(map, origin).await?) } DecodedMap::Index(map) => { @@ -434,9 +429,7 @@ impl SourceMap { let sections = map .sections() .filter_map(|section| { - section - .get_sourcemap() - .map(|s| (section.get_offset(), CrateMapWrapper::ref_cast(s))) + section.get_sourcemap().map(|s| (section.get_offset(), s)) }) .collect::>(); let sections = sections @@ -455,7 +448,7 @@ impl SourceMap { offset, // Urls are deprecated and we don't accept them None, - Some(map.0), + Some(map), )); } DecodedMap::Index(SourceMapIndex::new(file, new_sections)) @@ -463,12 +456,12 @@ impl SourceMap { DecodedMap::Hermes(_) => { todo!("hermes source maps are not implemented"); } - })) + }) } Ok(match &*self.await? { Self::Decoded(m) => { let map = Box::pin(decoded_map_with_resolved_sources(&m.map, origin)).await?; - Self::Decoded(InnerSourceMap::new(map.0)) + Self::Decoded(InnerSourceMap::new(map)) } Self::Sectioned(m) => { let mut sections = Vec::with_capacity(m.sections.len()); @@ -511,7 +504,7 @@ impl SourceMap { .. }) = &mut token { - if let DecodedMap::Regular(map) = &map.map.0 { + if let DecodedMap::Regular(map) = &*map.map { if map.get_source_count() == 1 { let source = map.sources().next().unwrap(); *guessed_original_file = Some(source.to_string()); @@ -520,7 +513,7 @@ impl SourceMap { } if need_source_content && content.is_none() { - if let Some(map) = map.map.as_regular_source_map() { + if let Some(map) = as_regular_source_map(&map.map) { content = tok.and_then(|tok| { let src_id = tok.get_src_id(); @@ -606,22 +599,21 @@ impl GenerateSourceMap for SourceMap { } } -/// A regular source map covers an entire file. +/// A regular source map covers an entire file. This wraps the sourcemap crate's struct to provide +/// Eq, PartialEq, and references via an Arc. #[derive(Clone, Debug, Serialize, Deserialize)] pub struct InnerSourceMap { - map: Arc, + map: Arc, } impl InnerSourceMap { pub fn new(map: DecodedMap) -> Self { - InnerSourceMap { - map: Arc::new(CrateMapWrapper(map)), - } + InnerSourceMap { map: Arc::new(map) } } } impl Deref for InnerSourceMap { - type Target = Arc; + type Target = Arc; fn deref(&self) -> &Self::Target { &self.map @@ -635,34 +627,6 @@ impl PartialEq for InnerSourceMap { } } -/// Wraps the DecodedMap struct so that it is Sync and Send. -/// -/// # Safety -/// -/// Must not use per line access to the SourceMap, as it is not thread safe. -#[derive(Debug, RefCast)] -#[repr(transparent)] -pub struct CrateMapWrapper(DecodedMap); - -// Safety: DecodedMap contains a raw pointer, which isn't Send, which is -// required to cache in a Vc. So, we have wrap it in 4 layers of cruft to do it. -unsafe impl Send for CrateMapWrapper {} -unsafe impl Sync for CrateMapWrapper {} - -/// Wraps the RegularMap struct so that it is Sync and Send. -/// -/// # Safety -/// -/// Must not use per line access to the SourceMap, as it is not thread safe. -#[derive(Debug, RefCast)] -#[repr(transparent)] -pub struct RegularMapWrapper(RegularMap); - -// Safety: RegularMap contains a raw pointer, which isn't Send, which is -// required to cache in a Vc. So, we have wrap it in 4 layers of cruft to do it. -unsafe impl Send for RegularMapWrapper {} -unsafe impl Sync for RegularMapWrapper {} - #[derive(Debug)] pub struct CrateIndexWrapper { pub sections: Vec, @@ -671,45 +635,16 @@ pub struct CrateIndexWrapper { #[derive(Debug)] pub struct CrateSectionWrapper { pub offset: SourcePos, - pub map: Arc, + pub map: Arc, } -impl CrateMapWrapper { - pub fn as_regular_source_map(&self) -> Option> { - match &self.0 { - DecodedMap::Regular(m) => Some(Cow::Borrowed(m)), - DecodedMap::Index(m) => m.flatten().map(Cow::Owned).ok(), - _ => None, - } +pub fn as_regular_source_map(decoded_map: &DecodedMap) -> Option> { + match decoded_map { + DecodedMap::Regular(m) => Some(Cow::Borrowed(m)), + DecodedMap::Index(m) => m.flatten().map(Cow::Owned).ok(), + _ => None, } } - -impl Deref for CrateMapWrapper { - type Target = DecodedMap; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl Serialize for CrateMapWrapper { - fn serialize(&self, serializer: S) -> Result { - use serde::ser::Error; - let mut bytes = vec![]; - self.0.to_writer(&mut bytes).map_err(Error::custom)?; - serializer.serialize_bytes(bytes.as_slice()) - } -} - -impl<'de> Deserialize<'de> for CrateMapWrapper { - fn deserialize>(deserializer: D) -> Result { - use serde::de::Error; - let bytes = <&[u8]>::deserialize(deserializer)?; - let map = DecodedMap::from_reader(bytes).map_err(Error::custom)?; - Ok(CrateMapWrapper(map)) - } -} - /// A sectioned source map contains many (possibly recursive) maps covering /// different regions of the file. #[derive(Eq, PartialEq, Debug, Serialize, Deserialize)] diff --git a/turbopack/crates/turbopack-ecmascript/src/parse.rs b/turbopack/crates/turbopack-ecmascript/src/parse.rs index 1c99733c878b80..a50ad4034a0b5d 100644 --- a/turbopack/crates/turbopack-ecmascript/src/parse.rs +++ b/turbopack/crates/turbopack-ecmascript/src/parse.rs @@ -29,7 +29,7 @@ use turbopack_core::{ error::PrettyPrintError, issue::{Issue, IssueExt, IssueSeverity, IssueStage, OptionStyledString, StyledString}, source::Source, - source_map::{GenerateSourceMap, OptionSourceMap, SourceMap}, + source_map::{as_regular_source_map, GenerateSourceMap, OptionSourceMap, SourceMap}, SOURCE_MAP_PREFIX, }; use turbopack_swc_utils::emitter::IssueEmitter; @@ -119,7 +119,7 @@ impl GenerateSourceMap for ParseResultSourceMap { None }; let input_map = if let Some(map) = original_src_map.as_ref() { - map.as_regular_source_map() + as_regular_source_map(map) } else { None };