Merge branch 'feature/init-name-resolution'

src/snapshot_middleware/dir.rs

@@ -8,7 +8,7 @@ use memofs::{DirEntry, Vfs};
 
 use crate::{
     snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource},
-    syncback::{hash_instance, FsSnapshot, SyncbackReturn, SyncbackSnapshot},
+    syncback::{hash_instance, slugify_name, FsSnapshot, SyncbackReturn, SyncbackSnapshot},
 };
 
 use super::{meta_file::DirectoryMetadata, snapshot_from_vfs};
@@ -134,12 +134,39 @@ pub fn syncback_dir_no_meta<'sync>(
     let mut children = Vec::new();
     let mut removed_children = Vec::new();
 
-    // We have to enforce unique child names for the file system.
-    let mut child_names = HashSet::with_capacity(new_inst.children().len());
+    // Build the old child map early so it can be used for deduplication below.
+    let mut old_child_map = HashMap::new();
+    if let Some(old_inst) = snapshot.old_inst() {
+        for child in old_inst.children() {
+            let inst = snapshot.get_old_instance(*child).unwrap();
+            old_child_map.insert(inst.name(), inst);
+        }
+    }
+
+    // Enforce unique filesystem names. Uses actual on-disk names for existing
+    // children and resolved names (with init-prefix) for new ones.
+    let mut fs_child_names = HashSet::with_capacity(new_inst.children().len());
     let mut duplicate_set = HashSet::new();
     for child_ref in new_inst.children() {
         let child = snapshot.get_new_instance(*child_ref).unwrap();
-        if !child_names.insert(child.name.to_lowercase()) {
+        let fs_name = old_child_map
+            .get(child.name.as_str())
+            .and_then(|old| old.metadata().relevant_paths.first())
+            .and_then(|p| p.file_name())
+            .and_then(|n| n.to_str())
+            .map(|s| s.to_lowercase())
+            .unwrap_or_else(|| {
+                let slug = slugify_name(&child.name);
+                let slug_lower = slug.to_lowercase();
+                // Mirror name_for_inst's init-prefix.
+                if slug_lower == "init" {
+                    format!("_{slug_lower}")
+                } else {
+                    slug_lower
+                }
+            });
+
+        if !fs_child_names.insert(fs_name) {
             duplicate_set.insert(child.name.as_str());
         }
     }
@@ -153,13 +180,7 @@ pub fn syncback_dir_no_meta<'sync>(
         anyhow::bail!("Instance has more than 25 children with duplicate names");
     }
 
-    if let Some(old_inst) = snapshot.old_inst() {
-        let mut old_child_map = HashMap::with_capacity(old_inst.children().len());
-        for child in old_inst.children() {
-            let inst = snapshot.get_old_instance(*child).unwrap();
-            old_child_map.insert(inst.name(), inst);
-        }
-
+    if snapshot.old_inst().is_some() {
         for new_child_ref in new_inst.children() {
             let new_child = snapshot.get_new_instance(*new_child_ref).unwrap();
             if let Some(old_child) = old_child_map.remove(new_child.name.as_str()) {
@@ -225,6 +246,12 @@ pub fn syncback_dir_no_meta<'sync>(
 mod test {
     use super::*;
 
+    use std::path::PathBuf;
+
+    use crate::{
+        snapshot::{InstanceMetadata, InstanceSnapshot},
+        Project, RojoTree, SyncbackData, SyncbackSnapshot,
+    };
     use memofs::{InMemoryFs, VfsSnapshot};
 
     #[test]
@@ -261,4 +288,237 @@ mod test {
 
         insta::assert_yaml_snapshot!(instance_snapshot);
     }
+
+    fn make_project() -> Project {
+        serde_json::from_str(r#"{"tree": {"$className": "DataModel"}}"#).unwrap()
+    }
+
+    fn make_vfs() -> Vfs {
+        let mut imfs = InMemoryFs::new();
+        imfs.load_snapshot("/root", VfsSnapshot::empty_dir()).unwrap();
+        Vfs::new(imfs)
+    }
+
+    /// Two children whose Roblox names are identical when lowercased ("Alpha"
+    /// and "alpha") but live at different filesystem paths because of the
+    /// `name` property ("Beta/" and "Alpha/" respectively). The dedup check
+    /// must use the actual filesystem paths, not the raw Roblox names, to
+    /// avoid a false-positive duplicate error.
+    #[test]
+    fn syncback_no_false_duplicate_with_name_prop() {
+        use rbx_dom_weak::{InstanceBuilder, WeakDom};
+
+        // Old child A: Roblox name "Alpha", on disk at "/root/Beta"
+        //   (name property maps "Alpha" → "Beta" on the filesystem)
+        let old_child_a = InstanceSnapshot::new()
+            .name("Alpha")
+            .class_name("Folder")
+            .metadata(
+                InstanceMetadata::new()
+                    .instigating_source(PathBuf::from("/root/Beta"))
+                    .relevant_paths(vec![PathBuf::from("/root/Beta")]),
+            );
+        // Old child B: Roblox name "alpha", on disk at "/root/Alpha"
+        let old_child_b = InstanceSnapshot::new()
+            .name("alpha")
+            .class_name("Folder")
+            .metadata(
+                InstanceMetadata::new()
+                    .instigating_source(PathBuf::from("/root/Alpha"))
+                    .relevant_paths(vec![PathBuf::from("/root/Alpha")]),
+            );
+        let old_parent = InstanceSnapshot::new()
+            .name("Parent")
+            .class_name("Folder")
+            .children(vec![old_child_a, old_child_b])
+            .metadata(
+                InstanceMetadata::new()
+                    .instigating_source(PathBuf::from("/root"))
+                    .relevant_paths(vec![PathBuf::from("/root")]),
+            );
+        let old_tree = RojoTree::new(old_parent);
+
+        // New state: same two children in Roblox.
+        let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT"));
+        let new_parent = new_tree.insert(
+            new_tree.root_ref(),
+            InstanceBuilder::new("Folder").with_name("Parent"),
+        );
+        new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Alpha"));
+        new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("alpha"));
+
+        let vfs = make_vfs();
+        let project = make_project();
+        let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project);
+        let snapshot = SyncbackSnapshot {
+            data,
+            old: Some(old_tree.get_root_id()),
+            new: new_parent,
+            path: PathBuf::from("/root"),
+            middleware: None,
+        };
+
+        let result = syncback_dir_no_meta(&snapshot);
+        assert!(
+            result.is_ok(),
+            "should not error when two children have the same lowercased Roblox \
+             name but map to distinct filesystem paths: {result:?}",
+        );
+    }
+
+    /// Two completely new children with the same non-init name would produce
+    /// the same filesystem entry and must be detected as a duplicate.
+    #[test]
+    fn syncback_detects_sibling_duplicate_names() {
+        use rbx_dom_weak::{InstanceBuilder, WeakDom};
+
+        let old_parent = InstanceSnapshot::new()
+            .name("Parent")
+            .class_name("Folder")
+            .metadata(
+                InstanceMetadata::new()
+                    .instigating_source(PathBuf::from("/root"))
+                    .relevant_paths(vec![PathBuf::from("/root")]),
+            );
+        let old_tree = RojoTree::new(old_parent);
+
+        let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT"));
+        let new_parent = new_tree.insert(
+            new_tree.root_ref(),
+            InstanceBuilder::new("Folder").with_name("Parent"),
+        );
+        // "Foo" is not a reserved name but two siblings named "Foo" still
+        // collide on disk.
+        new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Foo"));
+        new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Foo"));
+
+        let vfs = make_vfs();
+        let project = make_project();
+        let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project);
+        let snapshot = SyncbackSnapshot {
+            data,
+            old: Some(old_tree.get_root_id()),
+            new: new_parent,
+            path: PathBuf::from("/root"),
+            middleware: None,
+        };
+
+        let result = syncback_dir_no_meta(&snapshot);
+        assert!(
+            result.is_err(),
+            "should error when two new children would produce the same filesystem name",
+        );
+    }
+
+    /// A new child named "Init" (as a ModuleScript) would naively become
+    /// "Init.luau", which case-insensitively matches the parent's reserved
+    /// "init.luau". Syncback must resolve this automatically by prefixing the
+    /// filesystem name with '_' (→ "_Init.luau") rather than erroring.
+    #[test]
+    fn syncback_resolves_init_name_conflict() {
+        use rbx_dom_weak::{InstanceBuilder, WeakDom};
+
+        let old_parent = InstanceSnapshot::new()
+            .name("Parent")
+            .class_name("Folder")
+            .metadata(
+                InstanceMetadata::new()
+                    .instigating_source(PathBuf::from("/root"))
+                    .relevant_paths(vec![PathBuf::from("/root")]),
+            );
+        let old_tree = RojoTree::new(old_parent);
+
+        let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT"));
+        let new_parent = new_tree.insert(
+            new_tree.root_ref(),
+            InstanceBuilder::new("Folder").with_name("Parent"),
+        );
+        new_tree.insert(
+            new_parent,
+            InstanceBuilder::new("ModuleScript").with_name("Init"),
+        );
+
+        let vfs = make_vfs();
+        let project = make_project();
+        let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project);
+        let snapshot = SyncbackSnapshot {
+            data,
+            old: Some(old_tree.get_root_id()),
+            new: new_parent,
+            path: PathBuf::from("/root"),
+            middleware: None,
+        };
+
+        let result = syncback_dir_no_meta(&snapshot);
+        assert!(
+            result.is_ok(),
+            "should resolve init-name conflict by prefixing '_', not error: {result:?}",
+        );
+        // The child should have been placed at "_Init.luau", not "Init.luau".
+        let child_file_name = result
+            .unwrap()
+            .children
+            .into_iter()
+            .next()
+            .and_then(|c| c.path.file_name().map(|n| n.to_string_lossy().into_owned()))
+            .unwrap_or_default();
+        assert!(
+            child_file_name.starts_with('_'),
+            "child filesystem name should start with '_' to avoid init collision, \
+             got: {child_file_name}",
+        );
+    }
+
+    /// A child whose filesystem name is stored with a slugified prefix (e.g.
+    /// "_Init") must NOT be blocked — only the bare "init" stem is reserved.
+    #[test]
+    fn syncback_allows_slugified_init_name() {
+        use rbx_dom_weak::{InstanceBuilder, WeakDom};
+
+        // Existing child: on disk as "_Init" (slugified from a name with an
+        // illegal character), its stem is "_init" which is not reserved.
+        let old_child = InstanceSnapshot::new()
+            .name("Init")
+            .class_name("Folder")
+            .metadata(
+                InstanceMetadata::new()
+                    .instigating_source(PathBuf::from("/root/_Init"))
+                    .relevant_paths(vec![PathBuf::from("/root/_Init")]),
+            );
+        let old_parent = InstanceSnapshot::new()
+            .name("Parent")
+            .class_name("Folder")
+            .children(vec![old_child])
+            .metadata(
+                InstanceMetadata::new()
+                    .instigating_source(PathBuf::from("/root"))
+                    .relevant_paths(vec![PathBuf::from("/root")]),
+            );
+        let old_tree = RojoTree::new(old_parent);
+
+        let mut new_tree = WeakDom::new(InstanceBuilder::new("ROOT"));
+        let new_parent = new_tree.insert(
+            new_tree.root_ref(),
+            InstanceBuilder::new("Folder").with_name("Parent"),
+        );
+        new_tree.insert(new_parent, InstanceBuilder::new("Folder").with_name("Init"));
+
+        let vfs = make_vfs();
+        let project = make_project();
+        let data = SyncbackData::for_test(&vfs, &old_tree, &new_tree, &project);
+        let snapshot = SyncbackSnapshot {
+            data,
+            old: Some(old_tree.get_root_id()),
+            new: new_parent,
+            path: PathBuf::from("/root"),
+            middleware: None,
+        };
+
+        let result = syncback_dir_no_meta(&snapshot);
+        assert!(
+            result.is_ok(),
+            "should allow a child whose filesystem name is slugified away from \
+             the reserved 'init' stem: {result:?}",
+        );
+    }
 }

src/snapshot_middleware/meta_file.rs

@@ -154,11 +154,18 @@ impl AdjacentMetadata {
             .old_inst()
             .and_then(|inst| inst.metadata().specified_name.clone())
             .or_else(|| {
-                // If this is a new instance and its name is invalid for the filesystem,
-                // we need to specify the name in meta.json so it can be preserved
+                // Write name when the filesystem path doesn't match the
+                // instance name (invalid chars or init-prefix).
                 if snapshot.old_inst().is_none() {
                     let instance_name = &snapshot.new_inst().name;
-                    if validate_file_name(instance_name).is_err() {
+                    let fs_stem = path
+                        .file_name()
+                        .and_then(|n| n.to_str())
+                        .map(|s| s.split('.').next().unwrap_or(s))
+                        .unwrap_or("");
+                    if validate_file_name(instance_name).is_err()
+                        || fs_stem != instance_name.as_str()
+                    {
                         Some(instance_name.clone())
                     } else {
                         None
@@ -421,11 +428,17 @@ impl DirectoryMetadata {
             .old_inst()
             .and_then(|inst| inst.metadata().specified_name.clone())
             .or_else(|| {
-                // If this is a new instance and its name is invalid for the filesystem,
-                // we need to specify the name in meta.json so it can be preserved
+                // Write name when the directory name doesn't match the
+                // instance name (invalid chars or init-prefix).
                 if snapshot.old_inst().is_none() {
                     let instance_name = &snapshot.new_inst().name;
-                    if validate_file_name(instance_name).is_err() {
+                    let fs_name = path
+                        .file_name()
+                        .and_then(|n| n.to_str())
+                        .unwrap_or("");
+                    if validate_file_name(instance_name).is_err()
+                        || fs_name != instance_name.as_str()
+                    {
                         Some(instance_name.clone())
                     } else {
                         None

src/syncback/file_names.rs

@@ -36,23 +36,33 @@ pub fn name_for_inst<'a>(
             | Middleware::ServerScriptDir
             | Middleware::ClientScriptDir
             | Middleware::ModuleScriptDir => {
-                if validate_file_name(&new_inst.name).is_err() {
+                let name = if validate_file_name(&new_inst.name).is_err() {
                     Cow::Owned(slugify_name(&new_inst.name))
                 } else {
-                    Cow::Borrowed(&new_inst.name)
+                    Cow::Borrowed(new_inst.name.as_str())
+                };
+                // Prefix "init" to avoid colliding with reserved init files.
+                if name.to_lowercase() == "init" {
+                    Cow::Owned(format!("_{name}"))
+                } else {
+                    name
                 }
             }
             _ => {
                 let extension = extension_for_middleware(middleware);
                 let slugified;
-                let final_name = if validate_file_name(&new_inst.name).is_err() {
+                let stem: &str = if validate_file_name(&new_inst.name).is_err() {
                     slugified = slugify_name(&new_inst.name);
                     &slugified
                 } else {
                     &new_inst.name
                 };
-
-                Cow::Owned(format!("{final_name}.{extension}"))
+                // Prefix "init" stems to avoid colliding with reserved init files.
+                if stem.to_lowercase() == "init" {
+                    Cow::Owned(format!("_{stem}.{extension}"))
+                } else {
+                    Cow::Owned(format!("{stem}.{extension}"))
+                }
             }
         })
     }

src/syncback/snapshot.rs

@@ -238,6 +238,24 @@ pub fn inst_path(dom: &WeakDom, referent: Ref) -> String {
     path.join("/")
 }
 
+impl<'sync> SyncbackData<'sync> {
+    /// Constructs a `SyncbackData` for use in unit tests.
+    #[cfg(test)]
+    pub fn for_test(
+        vfs: &'sync Vfs,
+        old_tree: &'sync RojoTree,
+        new_tree: &'sync WeakDom,
+        project: &'sync Project,
+    ) -> Self {
+        Self {
+            vfs,
+            old_tree,
+            new_tree,
+            project,
+        }
+    }
+}
+
 #[cfg(test)]
 mod test {
     use rbx_dom_weak::{InstanceBuilder, WeakDom};