Improve the semantics of split_off_query_fragment

* if there is a query or fragment portion always format with the 'prepender' `?`/`#` character, even if the part is empty.
* if there is a query and a fragment section, don't get confused by `?`s in the fragment, these are allowed by the spec.

Furthermore, improve the semantics of `AssetIdent` to both document and enforce these same semantics around the `query` and `fragment` parts.

Author: lukesandberg

turbopack/crates/turbopack-core/src/file_source.rs

@@ -33,6 +33,8 @@ impl FileSource {
         query: ResolvedVc<RcStr>,
     ) -> Result<Vc<Self>> {
         if query.await?.is_empty() {
+            // Delegate to the other factory to ensure there is a single empty ResolvedVc<RcStr> for
+            // the FileSource type.
             Ok(Self::new(*path))
         } else {
             Ok(Self::cell(FileSource { path, query }))

turbopack/crates/turbopack-core/src/ident.rs

@@ -15,10 +15,12 @@ use crate::resolve::ModulePart;
 pub struct AssetIdent {
     /// The primary path of the asset
     pub path: ResolvedVc<FileSystemPath>,
-    /// The query string of the asset (e.g. `?foo=bar`)
+    /// The query string of the asset this is either the empty string or a query string that starts
+    /// with a `?` (e.g. `?foo=bar`)
     pub query: ResolvedVc<RcStr>,
-    /// The fragment of the asset (e.g. `#foo`)
-    pub fragment: Option<ResolvedVc<RcStr>>,
+    /// The fragment of the asset, this is either the empty string or a fragment string that starts
+    /// with a `#` (e.g. `#foo`)
+    pub fragment: ResolvedVc<RcStr>,
     /// The assets that are nested in this asset
     pub assets: Vec<(ResolvedVc<RcStr>, ResolvedVc<AssetIdent>)>,
     /// The modifiers of this asset (e.g. `client chunks`)
@@ -57,14 +59,10 @@ impl ValueToString for AssetIdent {
     async fn to_string(&self) -> Result<Vc<RcStr>> {
         let mut s = self.path.to_string().owned().await?.into_owned();
 
-        let query = self.query.await?;
-        if !query.is_empty() {
-            write!(s, "?{}", &*query)?;
-        }
-
-        if let Some(fragment) = &self.fragment {
-            write!(s, "#{}", fragment.await?)?;
-        }
+        // The query string is either empty or non-empty starting with `?` so we can just concat
+        s.push_str(&self.query.await?);
+        // ditto for fragment
+        s.push_str(&self.fragment.await?);
 
         if !self.assets.is_empty() {
             s.push_str(" {");
@@ -121,8 +119,19 @@ impl ValueToString for AssetIdent {
 #[turbo_tasks::value_impl]
 impl AssetIdent {
     #[turbo_tasks::function]
-    pub fn new(ident: Value<AssetIdent>) -> Vc<Self> {
-        ident.into_value().cell()
+    pub async fn new(ident: Value<AssetIdent>) -> Result<Vc<Self>> {
+        let query = &*ident.query.await?;
+        // TODO(lukesandber); downgrade to debug_assert
+        assert!(
+            query.is_empty() || query.starts_with("?"),
+            "query should be empty or start with a `?`"
+        );
+        let fragment = &*ident.fragment.await?;
+        assert!(
+            fragment.is_empty() || fragment.starts_with("#"),
+            "query should be empty or start with a `?`"
+        );
+        Ok(ident.into_value().cell())
     }
 
     /// Creates an [AssetIdent] from a [Vc<FileSystemPath>]
@@ -131,7 +140,7 @@ impl AssetIdent {
         Self::new(Value::new(AssetIdent {
             path,
             query: ResolvedVc::cell(RcStr::default()),
-            fragment: None,
+            fragment: ResolvedVc::cell(RcStr::default()),
             assets: Vec::new(),
             modifiers: Vec::new(),
             parts: Vec::new(),
@@ -252,9 +261,10 @@ impl AssetIdent {
             query.deterministic_hash(&mut hasher);
             has_hash = true;
         }
-        if let Some(fragment) = fragment {
+        let fragment = fragment.await?;
+        if !fragment.is_empty() {
             1_u8.deterministic_hash(&mut hasher);
-            fragment.await?.deterministic_hash(&mut hasher);
+            fragment.deterministic_hash(&mut hasher);
             has_hash = true;
         }
         for (key, ident) in assets.iter() {

turbopack/crates/turbopack-core/src/resolve/parse.rs

@@ -61,30 +61,32 @@ pub enum Request {
     },
 }
 
-fn split_off_query_fragment(raw: RcStr) -> (Pattern, Vc<RcStr>, Vc<RcStr>) {
-    let Some((raw, query)) = raw.split_once('?') else {
-        if let Some((raw, fragment)) = raw.split_once('#') {
-            return (
-                Pattern::Constant(raw.into()),
-                Vc::<RcStr>::default(),
-                Vc::cell(fragment.into()),
-            );
+/// Splits a string like `foo?bar#baz` into `(Pattern::Constant('foo'), '?bar', '#baz')`
+///
+/// If the hash or query portion are missing they will be empty strings otherwise they will be
+/// non-empty along with their prepender characters
+fn split_off_query_fragment(mut raw: &str) -> (Pattern, RcStr, RcStr) {
+    // Per the URI spec fragments can contain `?` characters, so we should trim it off first
+    // https://datatracker.ietf.org/doc/html/rfc3986#section-3.5
+
+    let hash = match raw.as_bytes().iter().position(|&b| b == b'#') {
+        Some(pos) => {
+            let hash = RcStr::from(&raw[pos..]);
+            raw = &raw[..pos];
+            hash
         }
-
-        return (
-            Pattern::Constant(raw),
-            Vc::<RcStr>::default(),
-            Vc::<RcStr>::default(),
-        );
+        None => RcStr::default(),
     };
 
-    let (query, fragment) = query.split_once('#').unwrap_or((query, ""));
-
-    (
-        Pattern::Constant(raw.into()),
-        Vc::cell(format!("?{query}").into()),
-        Vc::cell(format!("#{fragment}").into()),
-    )
+    let query = match raw.as_bytes().iter().position(|&b| b == b'?') {
+        Some(pos) => {
+            let query = RcStr::from(&raw[pos..]);
+            raw = &raw[..pos];
+            query
+        }
+        None => RcStr::default(),
+    };
+    (Pattern::Constant(RcStr::from(raw)), query, hash)
 }
 
 lazy_static! {
@@ -164,12 +166,12 @@ impl Request {
         }
 
         if r.starts_with('/') {
-            let (path, query, fragment) = split_off_query_fragment(r);
+            let (path, query, fragment) = split_off_query_fragment(&r);
 
             return Ok(Request::ServerRelative {
                 path,
-                query: query.to_resolved().await?,
-                fragment: fragment.to_resolved().await?,
+                query: ResolvedVc::cell(query),
+                fragment: ResolvedVc::cell(fragment),
             });
         }
 
@@ -180,23 +182,23 @@ impl Request {
         }
 
         if r.starts_with("./") || r.starts_with("../") || r == "." || r == ".." {
-            let (path, query, fragment) = split_off_query_fragment(r);
+            let (path, query, fragment) = split_off_query_fragment(&r);
 
             return Ok(Request::Relative {
                 path,
                 force_in_lookup_dir: false,
-                query: query.to_resolved().await?,
-                fragment: fragment.to_resolved().await?,
+                query: ResolvedVc::cell(query),
+                fragment: ResolvedVc::cell(fragment),
             });
         }
 
         if WINDOWS_PATH.is_match(&r) {
-            let (path, query, fragment) = split_off_query_fragment(r);
+            let (path, query, fragment) = split_off_query_fragment(&r);
 
             return Ok(Request::Windows {
                 path,
-                query: query.to_resolved().await?,
-                fragment: fragment.to_resolved().await?,
+                query: ResolvedVc::cell(query),
+                fragment: ResolvedVc::cell(fragment),
             });
         }
 
@@ -227,13 +229,13 @@ impl Request {
             .captures(&r)
             .and_then(|caps| caps.get(1).zip(caps.get(2)))
         {
-            let (path, query, fragment) = split_off_query_fragment(path.as_str().into());
+            let (path, query, fragment) = split_off_query_fragment(path.as_str());
 
             return Ok(Request::Module {
                 module: module.as_str().into(),
                 path,
-                query: query.to_resolved().await?,
-                fragment: fragment.to_resolved().await?,
+                query: ResolvedVc::cell(query),
+                fragment: ResolvedVc::cell(fragment),
             });
         }
 
@@ -817,3 +819,63 @@ pub async fn stringify_data_uri(
         data.await?
     ))
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_split_query_fragment() {
+        assert_eq!(
+            (
+                Pattern::Constant("foo".into()),
+                RcStr::default(),
+                RcStr::default()
+            ),
+            split_off_query_fragment("foo")
+        );
+        // These two cases are a bit odd, but it is important to treat `import './foo?'` differently
+        // from `import './foo'`, ditto for fragments.
+        assert_eq!(
+            (
+                Pattern::Constant("foo".into()),
+                RcStr::from("?"),
+                RcStr::default()
+            ),
+            split_off_query_fragment("foo?")
+        );
+        assert_eq!(
+            (
+                Pattern::Constant("foo".into()),
+                RcStr::default(),
+                RcStr::from("#")
+            ),
+            split_off_query_fragment("foo#")
+        );
+        assert_eq!(
+            (
+                Pattern::Constant("foo".into()),
+                RcStr::from("?bar=baz"),
+                RcStr::default()
+            ),
+            split_off_query_fragment("foo?bar=baz")
+        );
+        assert_eq!(
+            (
+                Pattern::Constant("foo".into()),
+                RcStr::default(),
+                RcStr::from("#stuff?bar=baz")
+            ),
+            split_off_query_fragment("foo#stuff?bar=baz")
+        );
+
+        assert_eq!(
+            (
+                Pattern::Constant("foo".into()),
+                RcStr::from("?bar=baz"),
+                RcStr::from("#stuff")
+            ),
+            split_off_query_fragment("foo?bar=baz#stuff")
+        );
+    }
+}

turbopack/crates/turbopack-css/src/chunk/mod.rs

@@ -194,7 +194,7 @@ impl CssChunk {
                 ServerFileSystem::new().root().to_resolved().await?
             },
             query: ResolvedVc::cell(RcStr::default()),
-            fragment: None,
+            fragment: ResolvedVc::cell(RcStr::default()),
             assets,
             modifiers: Vec::new(),
             parts: Vec::new(),

turbopack/crates/turbopack-ecmascript/src/chunk/mod.rs

@@ -123,7 +123,7 @@ impl Chunk for EcmascriptChunk {
                 ServerFileSystem::new().root().to_resolved().await?
             },
             query: ResolvedVc::cell(RcStr::default()),
-            fragment: None,
+            fragment: ResolvedVc::cell(RcStr::default()),
             assets,
             modifiers: Vec::new(),
             parts: Vec::new(),