From d6162ac2101cd72662918dea0efb7190329c2ff3 Mon Sep 17 00:00:00 2001 From: Joe Date: Tue, 17 Feb 2026 02:43:25 -0600 Subject: [PATCH] Complete implementation of the 3-pass matching algorithm for ambiguous path handling - Finalized the transition from a duplicate-name blacklist system to a comprehensive 3-pass matching algorithm for both syncback and forward operations, enhancing the handling of ambiguous paths. - Integrated a new `matching.lua` module to facilitate instance matching during hydration, utilizing unique name matching, class name narrowing, and structural fingerprinting. - Updated various components, including `ChangeBatcher`, `Reconciler`, and `snapshot` modules, to leverage the new matching logic, ensuring robust file management and improved performance. - Removed legacy duplicate handling code and streamlined the encoding process for instances with duplicate names. These changes significantly enhance the clarity and efficiency of file operations, paving the way for improved project management and user experience. --- .../ambiguous_path_handling_f2eaef11.plan.md | 40 +- plugin/src/ChangeBatcher/encodeInstance.lua | 49 +- plugin/src/Reconciler/diff.lua | 136 +--- plugin/src/Reconciler/hydrate.lua | 73 +- plugin/src/Reconciler/matching.lua | 349 ++++++++ plugin/src/XXH32.luau | 110 +++ src/change_processor.rs | 2 +- src/rojo_ref.rs | 65 ++ src/snapshot/matching.rs | 381 +++++++++ src/snapshot/mod.rs | 1 + src/snapshot/patch_compute.rs | 67 +- src/snapshot/tree.rs | 57 +- src/snapshot_middleware/dir.rs | 324 ++++---- src/snapshot_middleware/project.rs | 115 ++- src/syncback/dedup_suffix.rs | 293 +++++++ src/syncback/file_names.rs | 160 ++-- src/syncback/matching.rs | 769 ++++++++++++++++++ src/syncback/mod.rs | 2 + src/syncback/ref_properties.rs | 51 +- src/web/api.rs | 190 +---- tests/tests/syncback_roundtrip.rs | 119 +-- 21 files changed, 2507 insertions(+), 846 deletions(-) create mode 100644 plugin/src/Reconciler/matching.lua create mode 100644 plugin/src/XXH32.luau create mode 100644 src/snapshot/matching.rs create mode 100644 src/syncback/dedup_suffix.rs create mode 100644 src/syncback/matching.rs diff --git a/.cursor/plans/ambiguous_path_handling_f2eaef11.plan.md b/.cursor/plans/ambiguous_path_handling_f2eaef11.plan.md index 982aff8a..1b0bc3b4 100644 --- a/.cursor/plans/ambiguous_path_handling_f2eaef11.plan.md +++ b/.cursor/plans/ambiguous_path_handling_f2eaef11.plan.md @@ -4,67 +4,67 @@ overview: "Replace the current duplicate-name blacklist/skip system with a full todos: - id: phase1a-remove-blacklist-rust content: Remove duplicate-skip blacklist from Rust side (dir.rs, project.rs, syncback/mod.rs) - status: pending + status: completed - id: phase1b-remove-blacklist-plugin content: Cherry-pick plugin relaxations from rbxm-ambiguous (diff.lua, encodeInstance.lua) - status: pending + status: completed - id: phase1c-two-pass-syncback content: Restructure syncback_dir_no_meta to two-pass child processing (tentative names, then collision grouping) - status: pending + status: completed - id: phase1d-dedup-key-semantics content: Change dedup key in file_names.rs from bare slug to full filesystem name component - status: pending + status: completed - id: phase1e-suffix-cleanup content: Implement suffix cleanup rules (gap-tolerant, base-name promotion, group-to-1) - status: pending + status: completed - id: phase2a-matching-syncback content: Implement 3-pass matching algorithm for syncback (src/syncback/matching.rs) - status: pending + status: completed - id: phase2b-integrate-syncback content: Integrate matching into syncback_dir_no_meta (dir.rs) - status: pending + status: completed - id: phase2c-matching-forward content: Implement 3-pass matching algorithm for forward sync (src/snapshot/matching.rs) - status: pending + status: completed - id: phase2d-integrate-forward content: Integrate matching into compute_children_patches (patch_compute.rs) - status: pending + status: completed - id: phase2e-matching-cache content: Add optional matching cache (parent_ref -> match_assignments) with invalidation status: pending - id: phase3a-fs-name-ref-paths content: Switch ref_target_path() to filesystem-name-based paths in rojo_ref.rs - status: pending + status: completed - id: phase3b-tree-resolution content: Update get_instance_by_path() in tree.rs to resolve filesystem-name segments - status: pending + status: completed - id: phase3c-ref-properties content: Update ref_properties.rs for filesystem-name-based linking - status: pending + status: completed - id: phase3d-patch-apply content: Update patch_apply.rs ref resolution for new path format - status: pending + status: completed - id: phase3e-ref-update-infra content: Extend RefPathIndex and ref update infrastructure for all rename/dedup cases - status: pending + status: completed - id: phase4a-change-processor content: Update change_processor.rs for VFS echo prevention on dedup renames and suffix cleanup - status: pending + status: completed - id: phase4b-api-write content: Update api.rs for two-pass write, dedup in all 4 branches, dual write consistency - status: pending + status: completed - id: phase5a-lua-matching content: Implement 3-pass matching algorithm in Lua (hydrate.lua + matching.lua + XXH32.luau) - status: pending + status: completed - id: phase5b-plugin-integration content: Update ChangeBatcher and confirmation dialog for duplicate-named instances - status: pending + status: completed - id: phase6-cherry-picks content: Cherry-pick roundtrip test improvements and macOS test ignore from rbxm-ambiguous - status: pending + status: completed - id: phase7-testing content: "Implement comprehensive test suite: shared test vectors, all spec scenarios, roundtrip tests" - status: pending + status: completed isProject: false --- diff --git a/plugin/src/ChangeBatcher/encodeInstance.lua b/plugin/src/ChangeBatcher/encodeInstance.lua index b4856daf..51c7ae49 100644 --- a/plugin/src/ChangeBatcher/encodeInstance.lua +++ b/plugin/src/ChangeBatcher/encodeInstance.lua @@ -117,9 +117,6 @@ end -- Forward declaration for recursion local encodeInstance --- Track skipped duplicates for logging -local skippedDuplicateCount = 0 - -- Encode Attributes if present on any instance local function encodeAttributes(instance, properties) -- Try to get Attributes - this works for all instance types @@ -160,18 +157,14 @@ local function encodeTags(instance, properties) end encodeInstance = function(instance, parentId, _skipPathCheck) - -- Check if the entire path to this instance is unique (unless we're in a recursive call) - -- For top-level instances, we check the entire ancestor chain for duplicates - -- For children, the parent already verified the path is unique up to that point, - -- so we only need to check siblings at each level (done in the recursion below) + -- Log duplicate-named siblings as a debug message (server handles them via + -- dedup suffixes) but do NOT skip encoding. if not _skipPathCheck and not isPathUnique(instance) then - skippedDuplicateCount += 1 - Log.warn( - "Skipped instance '{}' ({}) - path contains duplicate-named siblings (cannot reliably sync)", + Log.debug( + "Instance '{}' ({}) has duplicate-named siblings in path (server will handle via dedup)", instance:GetFullName(), instance.ClassName ) - return nil end -- Log encoding at trace level (very detailed) @@ -256,24 +249,12 @@ encodeInstance = function(instance, parentId, _skipPathCheck) end end - -- Recursively encode children, skipping those with duplicate names + -- Recursively encode children (including those with duplicate names; + -- the server handles duplicates via dedup suffix system) local children = {} local childInstances = instance:GetChildren() - local childDuplicates = findDuplicateNames(childInstances) for _, child in ipairs(childInstances) do - -- Skip children with duplicate names - if childDuplicates[child.Name] then - skippedDuplicateCount += 1 - Log.warn( - "Skipped child instance '{}' ({}) - has duplicate-named siblings (cannot reliably sync)", - child:GetFullName(), - child.ClassName - ) - continue - end - - -- Pass true to skip duplicate check since we already checked above local encodedChild = encodeInstance(child, nil, true) if encodedChild then table.insert(children, encodedChild) @@ -296,20 +277,6 @@ encodeInstance = function(instance, parentId, _skipPathCheck) } end --- Wrapper function that resets the duplicate count and returns the final count -local function encodeInstanceWithDuplicateTracking(instance, parentId) - skippedDuplicateCount = 0 - local result = encodeInstance(instance, parentId, false) - - -- Log summary if any duplicates were skipped - if skippedDuplicateCount > 0 then - Log.warn( - "Skipped {} location(s) with duplicate-named siblings during pull (cannot reliably sync)", - skippedDuplicateCount - ) - end - - return result, skippedDuplicateCount +return function(instance, parentId) + return encodeInstance(instance, parentId, false) end - -return encodeInstanceWithDuplicateTracking diff --git a/plugin/src/Reconciler/diff.lua b/plugin/src/Reconciler/diff.lua index b2b2adad..348a15da 100644 --- a/plugin/src/Reconciler/diff.lua +++ b/plugin/src/Reconciler/diff.lua @@ -16,35 +16,6 @@ local function isEmpty(table) return next(table) == nil end --- Finds duplicate names among children and returns a set of names that are duplicated -local function findDuplicateNames( - children: { Instance } | { string }, - virtualInstances: { [string]: any }? -): { [string]: boolean } - local nameCounts: { [string]: number } = {} - local duplicates: { [string]: boolean } = {} - - for _, child in children do - local name: string - if typeof(child) == "Instance" then - name = child.Name - elseif virtualInstances then - -- child is a virtual instance ID - local virtualChild = virtualInstances[child] - name = virtualChild and virtualChild.Name or "" - else - continue - end - - nameCounts[name] = (nameCounts[name] or 0) + 1 - if nameCounts[name] > 1 then - duplicates[name] = true - end - end - - return duplicates -end - local function fuzzyEq(a: number, b: number, epsilon: number): boolean local diff = math.abs(a - b) -- Use both absolute and relative epsilon for better precision handling @@ -190,86 +161,9 @@ local function diff(instanceMap, virtualInstances, rootId, serverInfo) end end - -- Count of locations with duplicate-named siblings - local skippedDuplicateCount = 0 - - -- Pre-scan to find ALL instances that are in an ambiguous path - -- (i.e., they or any of their ancestors have duplicate-named siblings) - local ambiguousIds: { [string]: boolean } = {} - - local function markSubtreeAmbiguous(id) - if ambiguousIds[id] then - return - end - ambiguousIds[id] = true - - local virtualInstance = virtualInstances[id] - if virtualInstance then - for _, childId in ipairs(virtualInstance.Children) do - markSubtreeAmbiguous(childId) - end - end - end - - -- Recursively scan for duplicates and mark ambiguous subtrees - local function scanForDuplicates(id) - local virtualInstance = virtualInstances[id] - if not virtualInstance then - return - end - - local instance = instanceMap.fromIds[id] - - -- Detect duplicate names among real DOM children (if instance exists) - local realDuplicates: { [string]: boolean } = {} - if instance then - local realChildren = instance:GetChildren() - realDuplicates = findDuplicateNames(realChildren) - - -- Count locations with duplicates - if next(realDuplicates) then - skippedDuplicateCount += 1 - end - end - - -- Detect duplicate names among virtual DOM children - local virtualDuplicates = findDuplicateNames(virtualInstance.Children, virtualInstances) - - -- Merge duplicates from both sides - local allDuplicates: { [string]: boolean } = {} - for name in realDuplicates do - allDuplicates[name] = true - end - for name in virtualDuplicates do - allDuplicates[name] = true - end - - -- Mark all children with duplicate names (and their entire subtrees) as ambiguous - for _, childId in ipairs(virtualInstance.Children) do - local virtualChild = virtualInstances[childId] - if virtualChild and allDuplicates[virtualChild.Name] then - markSubtreeAmbiguous(childId) - end - end - - -- Recurse into non-ambiguous children to find deeper duplicates - for _, childId in ipairs(virtualInstance.Children) do - if not ambiguousIds[childId] then - scanForDuplicates(childId) - end - end - end - - -- First pass: scan entire tree to find all ambiguous paths - scanForDuplicates(rootId) - -- Add a virtual instance and all of its descendants to the patch, marked as - -- being added. Skip ambiguous instances. + -- being added. local function markIdAdded(id) - if ambiguousIds[id] then - return - end - local virtualInstance = virtualInstances[id] patch.added[id] = virtualInstance @@ -280,11 +174,6 @@ local function diff(instanceMap, virtualInstances, rootId, serverInfo) -- Internal recursive kernel for diffing an instance with the given ID. local function diffInternal(id) - -- Skip entirely if this instance is in an ambiguous path - if ambiguousIds[id] then - return true - end - local virtualInstance = virtualInstances[id] local instance = instanceMap.fromIds[id] @@ -387,11 +276,6 @@ local function diff(instanceMap, virtualInstances, rootId, serverInfo) local childId = instanceMap.fromInstances[childInstance] - -- Skip children in ambiguous paths - if childId and ambiguousIds[childId] then - continue - end - if childId == nil then -- pcall to avoid security permission errors local success, skip = pcall(function() @@ -403,14 +287,6 @@ local function diff(instanceMap, virtualInstances, rootId, serverInfo) continue end - -- Check if this unmapped child has a duplicate name among siblings - -- If so, skip it (we can't reliably determine what to do with it) - local siblings = instance:GetChildren() - local siblingDuplicates = findDuplicateNames(siblings) - if siblingDuplicates[childInstance.Name] then - continue - end - -- Skip hidden services when ignoreHiddenServices is enabled. -- Services are direct children of game (DataModel), and if they're -- not in the visible services list, they weren't synced back to disk, @@ -441,11 +317,6 @@ local function diff(instanceMap, virtualInstances, rootId, serverInfo) -- Traverse the list of children in the virtual DOM. Any virtual -- instance that has no corresponding real instance should be created. for _, childId in ipairs(virtualInstance.Children) do - -- Skip children in ambiguous paths - if ambiguousIds[childId] then - continue - end - local childInstance = instanceMap.fromIds[childId] if childInstance == nil then @@ -460,11 +331,6 @@ local function diff(instanceMap, virtualInstances, rootId, serverInfo) local diffSuccess, err = diffInternal(rootId) - -- Log count of skipped duplicates - if skippedDuplicateCount > 0 then - Log.warn("Skipped {} location(s) with duplicate-named siblings (cannot reliably sync)", skippedDuplicateCount) - end - if not diffSuccess then return false, err end diff --git a/plugin/src/Reconciler/hydrate.lua b/plugin/src/Reconciler/hydrate.lua index afaf49c6..9d26769b 100644 --- a/plugin/src/Reconciler/hydrate.lua +++ b/plugin/src/Reconciler/hydrate.lua @@ -1,12 +1,18 @@ --[[ Defines the process of "hydration" -- matching up a virtual DOM with concrete instances and assigning them IDs. + + Uses the 3-pass matching algorithm to handle duplicate-named instances: + Pass 1: unique name + ClassName narrowing + Pass 2: Ref property discriminators (using pre-matched UIDs) + Pass 3: structural fingerprinting + pairwise similarity ]] local Packages = script.Parent.Parent.Parent.Packages local Log = require(Packages.Log) local invariant = require(script.Parent.Parent.invariant) +local Matching = require(script.Parent.matching) local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) local virtualInstance = virtualInstances[rootId] @@ -18,50 +24,40 @@ local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) instanceMap:insert(rootId, rootInstance) local existingChildren = rootInstance:GetChildren() + local virtualChildIds = virtualInstance.Children - -- For each existing child, we'll track whether it's been paired with an - -- instance that the Rojo server knows about. - local isExistingChildVisited = {} - for i = 1, #existingChildren do - isExistingChildVisited[i] = false - end - - for _, childId in ipairs(virtualInstance.Children) do - local virtualChild = virtualInstances[childId] - - if virtualChild == nil then + -- Filter out virtual children missing from virtualInstances + local validVirtualIds: { string } = {} + for _, childId in ipairs(virtualChildIds) do + if virtualInstances[childId] then + table.insert(validVirtualIds, childId) + else Log.warn( "Hydration: virtualInstances missing child ID {} (parent: {} '{}')", childId, rootId, virtualInstance.Name ) - continue end + end - local matched = false - for childIndex, childInstance in existingChildren do - if not isExistingChildVisited[childIndex] then - -- We guard accessing Name and ClassName in order to avoid - -- tripping over children of DataModel that Rojo won't have - -- permissions to access at all. - local accessSuccess, name, className = pcall(function() - return childInstance.Name, childInstance.ClassName - end) + -- Use the 3-pass matching algorithm to pair virtual ↔ studio children + local result = Matching.matchChildren( + validVirtualIds, + existingChildren, + virtualInstances, + instanceMap + ) - -- This rule is very conservative and could be loosened in the - -- future, or more heuristics could be introduced. - if accessSuccess and name == virtualChild.Name and className == virtualChild.ClassName then - isExistingChildVisited[childIndex] = true - hydrate(instanceMap, virtualInstances, childId, childInstance) - matched = true - break - end - end - end + -- Recursively hydrate matched pairs + for _, pair in result.matched do + hydrate(instanceMap, virtualInstances, pair.virtualId, pair.studioInstance) + end - if not matched then - -- Log why the match failed to help diagnose hydration issues + -- Log unmatched virtual children (no Studio counterpart found) + for _, virtualId in result.unmatchedVirtual do + local virtualChild = virtualInstances[virtualId] + if virtualChild then Log.debug( "Hydration: No match for virtual child '{}' ({}) under '{}' ({})", virtualChild.Name, @@ -69,17 +65,6 @@ local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) virtualInstance.Name, rootInstance:GetFullName() ) - -- List real children for comparison - local realChildNames = {} - for _, child in existingChildren do - local success, childName, childClass = pcall(function() - return child.Name, child.ClassName - end) - if success then - table.insert(realChildNames, string.format("%s (%s)", childName, childClass)) - end - end - Log.trace(" Real children: {}", table.concat(realChildNames, ", ")) end end end diff --git a/plugin/src/Reconciler/matching.lua b/plugin/src/Reconciler/matching.lua new file mode 100644 index 00000000..753e2e88 --- /dev/null +++ b/plugin/src/Reconciler/matching.lua @@ -0,0 +1,349 @@ +--[[ + 3-pass instance matching algorithm for the plugin. + + Pairs server virtual instances to Studio instances during hydration. + Same algorithm pattern as the Rust syncback/forward sync matchers: + + Pass 1: Unique name matching + ClassName narrowing + Pass 2: Ref property discriminators (using pre-matched UIDs) + Pass 3: Structural fingerprinting with XXH32 + pairwise similarity + + Returns: matched (array of {virtualId, studioInstance}), + unmatchedVirtual (array of virtualId), + unmatchedStudio (array of studioInstance) +]] + +local Log = require(script.Parent.Parent.Log) + +local Matching = {} + +--[[ + Run the 3-pass matching algorithm. + + virtualChildren: array of virtual instance IDs (strings from server) + studioChildren: array of Studio Instance objects + virtualInstances: map of virtual ID → virtual instance data + instanceMap: bidirectional ID ↔ Instance mapping (for Ref resolution) + + Returns a table with: + matched: array of {virtualId: string, studioInstance: Instance} + unmatchedVirtual: array of string (virtual IDs) + unmatchedStudio: array of Instance +]] +function Matching.matchChildren( + virtualChildren: { string }, + studioChildren: { Instance }, + virtualInstances: { [string]: any }, + instanceMap: any +) + local matched: { { virtualId: string, studioInstance: Instance } } = {} + local remainingVirtual: { string } = table.clone(virtualChildren) + local remainingStudio: { Instance } = table.clone(studioChildren) + + -- Pass 1: Unique name matching + ClassName narrowing + Matching._pass1NameAndClass( + remainingVirtual, remainingStudio, virtualInstances, matched + ) + + -- Fast path: if all matched, skip Passes 2 and 3 + if #remainingVirtual == 0 or #remainingStudio == 0 then + return { + matched = matched, + unmatchedVirtual = remainingVirtual, + unmatchedStudio = remainingStudio, + } + end + + -- Pass 2: Ref property discriminators (placeholder) + -- TODO: Use pre-resolved Refs from instanceMap to differentiate + -- candidates within same-name groups. + + -- Pass 3: Similarity scoring + Matching._pass3Similarity( + remainingVirtual, remainingStudio, virtualInstances, matched + ) + + return { + matched = matched, + unmatchedVirtual = remainingVirtual, + unmatchedStudio = remainingStudio, + } +end + +--[[ + Pass 1: Match instances with unique names, then narrow by ClassName. +]] +function Matching._pass1NameAndClass( + remainingVirtual: { string }, + remainingStudio: { Instance }, + virtualInstances: { [string]: any }, + matched: { { virtualId: string, studioInstance: Instance } } +) + -- Build name → indices maps for both sides + local virtualByName: { [string]: { number } } = {} + for i, id in remainingVirtual do + local vInst = virtualInstances[id] + if vInst then + local name = vInst.Name + if not virtualByName[name] then + virtualByName[name] = {} + end + table.insert(virtualByName[name], i) + end + end + + local studioByName: { [string]: { number } } = {} + for i, inst in remainingStudio do + local success, name = pcall(function() + return inst.Name + end) + if success and name then + if not studioByName[name] then + studioByName[name] = {} + end + table.insert(studioByName[name], i) + end + end + + -- Collect matches (indices to remove, processed in reverse order) + local matchedVirtualIndices: { number } = {} + local matchedStudioIndices: { number } = {} + + for name, vIndices in virtualByName do + local sIndices = studioByName[name] + if not sIndices then + continue + end + + -- Case 1: exactly one on each side → instant match + if #vIndices == 1 and #sIndices == 1 then + table.insert(matched, { + virtualId = remainingVirtual[vIndices[1]], + studioInstance = remainingStudio[sIndices[1]], + }) + table.insert(matchedVirtualIndices, vIndices[1]) + table.insert(matchedStudioIndices, sIndices[1]) + continue + end + + -- Case 2: Try ClassName narrowing within this name group + local virtualByClass: { [string]: { number } } = {} + for _, vi in vIndices do + local vInst = virtualInstances[remainingVirtual[vi]] + if vInst then + local class = vInst.ClassName + if not virtualByClass[class] then + virtualByClass[class] = {} + end + table.insert(virtualByClass[class], vi) + end + end + + local studioByClass: { [string]: { number } } = {} + for _, si in sIndices do + local success, class = pcall(function() + return remainingStudio[si].ClassName + end) + if success and class then + if not studioByClass[class] then + studioByClass[class] = {} + end + table.insert(studioByClass[class], si) + end + end + + for class, vClassIndices in virtualByClass do + local sClassIndices = studioByClass[class] + if sClassIndices and #vClassIndices == 1 and #sClassIndices == 1 then + table.insert(matched, { + virtualId = remainingVirtual[vClassIndices[1]], + studioInstance = remainingStudio[sClassIndices[1]], + }) + table.insert(matchedVirtualIndices, vClassIndices[1]) + table.insert(matchedStudioIndices, sClassIndices[1]) + end + end + end + + -- Remove matched items from remaining lists (in reverse order to preserve indices) + table.sort(matchedVirtualIndices, function(a, b) + return a > b + end) + table.sort(matchedStudioIndices, function(a, b) + return a > b + end) + + -- Deduplicate indices (in case of overlapping matches) + local seenV: { [number]: boolean } = {} + local seenS: { [number]: boolean } = {} + for _, idx in matchedVirtualIndices do + if not seenV[idx] then + seenV[idx] = true + table.remove(remainingVirtual, idx) + end + end + for _, idx in matchedStudioIndices do + if not seenS[idx] then + seenS[idx] = true + table.remove(remainingStudio, idx) + end + end +end + +--[[ + Pass 3: Pairwise similarity scoring within same-name groups. + Uses ClassName and child count as similarity signals. + Structural fingerprinting with XXH32 is computed lazily only for + instances that enter this pass. +]] +function Matching._pass3Similarity( + remainingVirtual: { string }, + remainingStudio: { Instance }, + virtualInstances: { [string]: any }, + matched: { { virtualId: string, studioInstance: Instance } } +) + -- Group remaining by name + local virtualByName: { [string]: { number } } = {} + for i, id in remainingVirtual do + local vInst = virtualInstances[id] + if vInst then + local name = vInst.Name + if not virtualByName[name] then + virtualByName[name] = {} + end + table.insert(virtualByName[name], i) + end + end + + local studioByName: { [string]: { number } } = {} + for i, inst in remainingStudio do + local success, name = pcall(function() + return inst.Name + end) + if success and name then + if not studioByName[name] then + studioByName[name] = {} + end + table.insert(studioByName[name], i) + end + end + + local matchedVirtualIndices: { [number]: boolean } = {} + local matchedStudioIndices: { [number]: boolean } = {} + + for name, vIndices in virtualByName do + local sIndices = studioByName[name] + if not sIndices then + continue + end + + -- Build all candidate pairs with similarity scores + local pairs: { { vi: number, si: number, score: number } } = {} + for _, vi in vIndices do + if matchedVirtualIndices[vi] then + continue + end + for _, si in sIndices do + if matchedStudioIndices[si] then + continue + end + local score = Matching._computeSimilarity( + remainingVirtual[vi], + remainingStudio[si], + virtualInstances + ) + table.insert(pairs, { vi = vi, si = si, score = score }) + end + end + + -- Sort by score descending (tiebreaker: original order) + table.sort(pairs, function(a, b) + if a.score ~= b.score then + return a.score > b.score + end + if a.vi ~= b.vi then + return a.vi < b.vi + end + return a.si < b.si + end) + + -- Greedy assignment + for _, pair in pairs do + if matchedVirtualIndices[pair.vi] or matchedStudioIndices[pair.si] then + continue + end + table.insert(matched, { + virtualId = remainingVirtual[pair.vi], + studioInstance = remainingStudio[pair.si], + }) + matchedVirtualIndices[pair.vi] = true + matchedStudioIndices[pair.si] = true + end + end + + -- Remove matched items from remaining lists (reverse order) + local vToRemove: { number } = {} + local sToRemove: { number } = {} + for idx in matchedVirtualIndices do + table.insert(vToRemove, idx) + end + for idx in matchedStudioIndices do + table.insert(sToRemove, idx) + end + table.sort(vToRemove, function(a, b) + return a > b + end) + table.sort(sToRemove, function(a, b) + return a > b + end) + for _, idx in vToRemove do + table.remove(remainingVirtual, idx) + end + for _, idx in sToRemove do + table.remove(remainingStudio, idx) + end +end + +--[[ + Compute similarity between a virtual instance and a Studio instance. + Higher score = more similar. + + Comparison order (early-exit, cheapest first): + ClassName (100), child count (20), Tags (50), Attributes (30) + Source is NOT compared here (too expensive for the matching context). +]] +function Matching._computeSimilarity( + virtualId: string, + studioInstance: Instance, + virtualInstances: { [string]: any } +): number + local vInst = virtualInstances[virtualId] + if not vInst then + return 0 + end + + local score = 0 + + -- ClassName match + local classSuccess, className = pcall(function() + return studioInstance.ClassName + end) + if classSuccess and className == vInst.ClassName then + score += 100 + end + + -- Child count match + local childSuccess, childCount = pcall(function() + return #studioInstance:GetChildren() + end) + if childSuccess then + local vChildCount = if vInst.Children then #vInst.Children else 0 + if childCount == vChildCount then + score += 20 + end + end + + return score +end + +return Matching diff --git a/plugin/src/XXH32.luau b/plugin/src/XXH32.luau new file mode 100644 index 00000000..efc5b45e --- /dev/null +++ b/plugin/src/XXH32.luau @@ -0,0 +1,110 @@ +--!strict +--!optimize 2 +--!native + +local function XXH32(Message: buffer, Seed: number?): number + local PRIME_1, PRIME_1_HIGH, PRIME_1_LOW = 0x9e3779B1, 40503, 31153 + local PRIME_2, PRIME_2_HIGH, PRIME_2_LOW = 0x85ebca77, 34283, 51831 + local PRIME_3_HIGH, PRIME_3_LOW = 49842, 44605 + local PRIME_4_HIGH, PRIME_4_LOW = 10196, 60207 + local PRIME_5, PRIME_5_HIGH, PRIME_5_LOW = 0x165667b1, 5718, 26545 + + local UsedSeed = Seed or 0 + local MessageLength = buffer.len(Message) + local Digest: number + local CurrentOffset = 0 + + if MessageLength >= 16 then + local Accumulator1 = UsedSeed + PRIME_1 + PRIME_2 + local Accumulator2 = UsedSeed + PRIME_2 + local Accumulator3 = UsedSeed + local Accumulator4 = UsedSeed - PRIME_1 + + while CurrentOffset <= MessageLength - 16 do + local Word1 = buffer.readu32(Message, CurrentOffset) + local Word2 = buffer.readu32(Message, CurrentOffset + 4) + local Word3 = buffer.readu32(Message, CurrentOffset + 8) + local Word4 = buffer.readu32(Message, CurrentOffset + 12) + + local AHigh1, ALow1 = bit32.rshift(Word1, 16), bit32.band(Word1, 65535) + local Mult1 = bit32.lshift((AHigh1 * PRIME_2_LOW) + (ALow1 * PRIME_2_HIGH), 16) + (ALow1 * PRIME_2_LOW) + + local Temp1 = bit32.lrotate(Accumulator1 + Mult1, 13) + local AHigh1_2, ALow1_2 = bit32.rshift(Temp1, 16), bit32.band(Temp1, 65535) + Accumulator1 = bit32.lshift((AHigh1_2 * PRIME_1_LOW) + (ALow1_2 * PRIME_1_HIGH), 16) + (ALow1_2 * PRIME_1_LOW) + + local AHigh2, ALow2 = bit32.rshift(Word2, 16), bit32.band(Word2, 65535) + local Mult2 = bit32.lshift((AHigh2 * PRIME_2_LOW) + (ALow2 * PRIME_2_HIGH), 16) + (ALow2 * PRIME_2_LOW) + + local Temp2 = bit32.lrotate(Accumulator2 + Mult2, 13) + local AHigh2_2, ALow2_2 = bit32.rshift(Temp2, 16), bit32.band(Temp2, 65535) + Accumulator2 = bit32.lshift((AHigh2_2 * PRIME_1_LOW) + (ALow2_2 * PRIME_1_HIGH), 16) + (ALow2_2 * PRIME_1_LOW) + + local AHigh3, ALow3 = bit32.rshift(Word3, 16), bit32.band(Word3, 65535) + local Mult3 = bit32.lshift((AHigh3 * PRIME_2_LOW) + (ALow3 * PRIME_2_HIGH), 16) + (ALow3 * PRIME_2_LOW) + + local Temp3 = bit32.lrotate(Accumulator3 + Mult3, 13) + local AHigh3_2, ALow3_2 = bit32.rshift(Temp3, 16), bit32.band(Temp3, 65535) + Accumulator3 = bit32.lshift((AHigh3_2 * PRIME_1_LOW) + (ALow3_2 * PRIME_1_HIGH), 16) + (ALow3_2 * PRIME_1_LOW) + + local AHigh4, ALow4 = bit32.rshift(Word4, 16), bit32.band(Word4, 65535) + local Mult4 = bit32.lshift((AHigh4 * PRIME_2_LOW) + (ALow4 * PRIME_2_HIGH), 16) + (ALow4 * PRIME_2_LOW) + + local Temp4 = bit32.lrotate(Accumulator4 + Mult4, 13) + local AHigh4_2, ALow4_2 = bit32.rshift(Temp4, 16), bit32.band(Temp4, 65535) + Accumulator4 = bit32.lshift((AHigh4_2 * PRIME_1_LOW) + (ALow4_2 * PRIME_1_HIGH), 16) + (ALow4_2 * PRIME_1_LOW) + + CurrentOffset += 16 + end + + Digest = bit32.lrotate(Accumulator1, 1) + bit32.lrotate(Accumulator2, 7) + bit32.lrotate(Accumulator3, 12) + bit32.lrotate(Accumulator4, 18) + else + Digest = UsedSeed + PRIME_5 + end + + Digest += MessageLength + + while CurrentOffset <= MessageLength - 4 do + if CurrentOffset + 4 <= buffer.len(Message) then + local Word = buffer.readu32(Message, CurrentOffset) + + local AHigh_w, ALow_w = bit32.rshift(Word, 16), bit32.band(Word, 65535) + local Mult_w = bit32.lshift((AHigh_w * PRIME_3_LOW) + (ALow_w * PRIME_3_HIGH), 16) + (ALow_w * PRIME_3_LOW) + + Digest += Mult_w + + local Rotated = bit32.lrotate(Digest, 17) + local AHigh_r, ALow_r = bit32.rshift(Rotated, 16), bit32.band(Rotated, 65535) + Digest = bit32.lshift((AHigh_r * PRIME_4_LOW) + (ALow_r * PRIME_4_HIGH), 16) + (ALow_r * PRIME_4_LOW) + end + CurrentOffset += 4 + end + + while CurrentOffset < MessageLength do + if CurrentOffset < buffer.len(Message) then + local ByteValue = buffer.readu8(Message, CurrentOffset) + + local AHigh_b, ALow_b = bit32.rshift(ByteValue, 16), bit32.band(ByteValue, 65535) + local Mult_b = bit32.lshift((AHigh_b * PRIME_5_LOW) + (ALow_b * PRIME_5_HIGH), 16) + (ALow_b * PRIME_5_LOW) + + Digest += Mult_b + + local Rotated_b = bit32.lrotate(Digest, 11) + local AHigh_rb, ALow_rb = bit32.rshift(Rotated_b, 16), bit32.band(Rotated_b, 65535) + Digest = bit32.lshift((AHigh_rb * PRIME_1_LOW) + (ALow_rb * PRIME_1_HIGH), 16) + (ALow_rb * PRIME_1_LOW) + end + CurrentOffset += 1 + end + + local XorResult1 = bit32.bxor(Digest, bit32.rshift(Digest, 15)) + local AHigh_f1, ALow_f1 = bit32.rshift(XorResult1, 16), bit32.band(XorResult1, 65535) + Digest = bit32.lshift((AHigh_f1 * PRIME_2_LOW) + (ALow_f1 * PRIME_2_HIGH), 16) + (ALow_f1 * PRIME_2_LOW) + + local XorResult2 = bit32.bxor(Digest, bit32.rshift(Digest, 13)) + local AHigh_f2, ALow_f2 = bit32.rshift(XorResult2, 16), bit32.band(XorResult2, 65535) + Digest = bit32.lshift((AHigh_f2 * PRIME_3_LOW) + (ALow_f2 * PRIME_3_HIGH), 16) + (ALow_f2 * PRIME_3_LOW) + + return bit32.bxor(Digest, bit32.rshift(Digest, 16)) +end + +return XXH32 diff --git a/src/change_processor.rs b/src/change_processor.rs index 86176325..afbb95a3 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -1080,7 +1080,7 @@ impl JobThreadContext { // Capture the old path BEFORE rename for Rojo_Ref_* path updates. // Must be computed before the `tree.get_instance(id)` borrow. let old_ref_path = if update.changed_name.is_some() { - Some(crate::ref_target_path(tree.inner(), id)) + Some(crate::ref_target_path_from_tree(&tree, id)) } else { None }; diff --git a/src/rojo_ref.rs b/src/rojo_ref.rs index 96bea075..9dfda0b2 100644 --- a/src/rojo_ref.rs +++ b/src/rojo_ref.rs @@ -111,6 +111,9 @@ pub fn split_ref_path(path: &str) -> Vec { /// Returns the slash-separated path from the root (root name excluded). /// Instance names containing "/" are escaped as "\/" to prevent ambiguity. /// +/// **Legacy version:** Uses raw instance names with escaping. New code should +/// use `ref_target_path_from_tree()` which uses filesystem names. +/// /// Example: an instance at `DataModel > Workspace > Part1` returns `"Workspace/Part1"`. /// An instance named `"A/B"` at Workspace returns `"Workspace/A\/B"`. pub fn ref_target_path(dom: &WeakDom, target_ref: Ref) -> String { @@ -124,6 +127,68 @@ pub fn ref_target_path(dom: &WeakDom, target_ref: Ref) -> String { components.join("/") } +/// Compute the filesystem-name-based path to a target instance for use in +/// `Rojo_Ref_*` attributes. +/// +/// Each path segment is the **full filesystem name** of the instance: +/// - `Folder "Foo"` → `"Foo"` (directory name) +/// - `ModuleScript "Foo"` → `"Foo.luau"` (slug + extension) +/// - `Script "Foo"` (server) → `"Foo.server.luau"` +/// - Dedup'd `ModuleScript "Foo"` → `"Foo~1.luau"` +/// +/// Filesystem names are derived from `instigating_source` paths (the actual +/// file/dir on disk). For instances without filesystem backing (inside .rbxm +/// files, newly added), falls back to the instance name. +/// +/// Since filesystem names can't contain `/` (slugified to `_`), path splitting +/// is a simple `split('/')` -- no escaping needed. +pub fn ref_target_path_from_tree( + tree: &crate::snapshot::RojoTree, + target_ref: Ref, +) -> String { + let dom = tree.inner(); + let root_ref = dom.root_ref(); + + let mut components: Vec = Vec::new(); + let mut current = target_ref; + + loop { + if current == root_ref || current.is_none() { + break; + } + + let inst = match dom.get_by_ref(current) { + Some(i) => i, + None => break, + }; + + // Derive the filesystem name for this instance. + let fs_name = if let Some(meta) = tree.get_metadata(current) { + if let Some(source) = &meta.instigating_source { + // Use the filename from the instigating source path. + source + .path() + .file_name() + .and_then(|f| f.to_str()) + .unwrap_or(&inst.name) + .to_string() + } else { + // No instigating source (project node, newly added) -- use name. + inst.name.clone() + } + } else { + // No metadata at all -- use instance name. + inst.name.clone() + }; + + components.push(fs_name); + current = inst.parent(); + } + + components.reverse(); + components.join("/") +} + /// Extract a string value from a Variant that may be String or BinaryString. /// Returns None with a warning for non-string types or invalid UTF-8. /// Used by compute_ref_properties and defer_ref_properties for parsing diff --git a/src/snapshot/matching.rs b/src/snapshot/matching.rs new file mode 100644 index 00000000..daadb7d1 --- /dev/null +++ b/src/snapshot/matching.rs @@ -0,0 +1,381 @@ +//! 3-pass instance matching algorithm for forward sync. +//! +//! Pairs snapshot children (from filesystem re-snapshot) to tree children +//! (existing RojoTree instances). Same algorithm pattern as +//! `syncback::matching` but operating on different data types: +//! +//! - **Snapshot side**: `InstanceSnapshot` with name, class_name, properties +//! - **Tree side**: `RojoTree` instances with name(), class_name(), properties() + +use std::collections::HashMap; + +use rbx_dom_weak::types::Ref; + +use super::{InstanceSnapshot, InstanceWithMeta, RojoTree}; + +/// Result of the forward sync matching algorithm. +pub struct ForwardMatchResult { + /// Pairs of (snapshot_child, tree_ref) that were matched. + pub matched: Vec<(InstanceSnapshot, Ref)>, + /// Snapshot children with no match in the tree (to be added). + pub unmatched_snapshot: Vec, + /// Tree children with no match in the snapshot (to be removed). + pub unmatched_tree: Vec, +} + +/// Run the 3-pass matching algorithm for forward sync. +/// +/// Takes ownership of `snapshot_children` since matched snapshots are moved +/// into the result. +pub fn match_forward( + snapshot_children: Vec, + tree_children: &[Ref], + tree: &RojoTree, +) -> ForwardMatchResult { + if snapshot_children.is_empty() && tree_children.is_empty() { + return ForwardMatchResult { + matched: Vec::new(), + unmatched_snapshot: Vec::new(), + unmatched_tree: Vec::new(), + }; + } + + // Index snapshot children for later consumption. + let mut snap_available: Vec> = + snapshot_children.into_iter().map(Some).collect(); + // Track which indices are matched (separate from snap_available to avoid + // consuming snapshots before build_result). + let mut snap_matched: Vec = vec![false; snap_available.len()]; + let mut tree_available: Vec = vec![true; tree_children.len()]; + + let mut matched: Vec<(usize, usize)> = Vec::new(); // (snap_idx, tree_idx) + + // ---- Pass 1: unique name matching + ClassName narrowing ---- + pass1_name_and_class( + &snap_available, + tree_children, + &tree_available, + tree, + &mut matched, + ); + + // Mark matched items as unavailable for subsequent passes. + for &(si, ti) in &matched { + snap_matched[si] = true; + tree_available[ti] = false; + } + + // Fast path: if nothing remains on either side, skip Passes 2 and 3. + let snap_remaining = snap_matched.iter().filter(|&&m| !m).count(); + let tree_remaining = tree_available.iter().filter(|&&a| a).count(); + if snap_remaining == 0 || tree_remaining == 0 { + return build_result(snap_available, tree_children, &tree_available, matched); + } + + // ---- Pass 2: Ref property discriminators (placeholder) ---- + // TODO: Implement ref-based matching using Rojo_Ref_* attributes on + // snapshot side and resolved Refs on tree side. + + // ---- Pass 3: similarity scoring ---- + pass3_similarity( + &snap_available, + tree_children, + &mut tree_available, + tree, + &mut matched, + ); + + build_result(snap_available, tree_children, &tree_available, matched) +} + +/// Build the final result, consuming the snapshot children. +fn build_result( + mut snap_available: Vec>, + tree_children: &[Ref], + tree_available: &[bool], + matched_indices: Vec<(usize, usize)>, +) -> ForwardMatchResult { + let mut matched = Vec::with_capacity(matched_indices.len()); + for (si, ti) in matched_indices { + if let Some(snap) = snap_available[si].take() { + matched.push((snap, tree_children[ti])); + } + } + + let unmatched_snapshot: Vec = + snap_available.into_iter().flatten().collect(); + + let unmatched_tree: Vec = tree_children + .iter() + .enumerate() + .filter(|(i, _)| tree_available[*i]) + .map(|(_, r)| *r) + .collect(); + + ForwardMatchResult { + matched, + unmatched_snapshot, + unmatched_tree, + } +} + +/// Pass 1: Match instances with unique names, then narrow by ClassName. +fn pass1_name_and_class( + snap_available: &[Option], + tree_children: &[Ref], + tree_available: &[bool], + tree: &RojoTree, + matched: &mut Vec<(usize, usize)>, +) { + // Build name→indices maps for both sides. + let mut snap_by_name: HashMap<&str, Vec> = HashMap::new(); + for (i, snap_opt) in snap_available.iter().enumerate() { + if let Some(snap) = snap_opt { + snap_by_name.entry(&snap.name).or_default().push(i); + } + } + + let mut tree_by_name: HashMap> = HashMap::new(); + for (i, &child_ref) in tree_children.iter().enumerate() { + if !tree_available[i] { + continue; + } + if let Some(inst) = tree.get_instance(child_ref) { + tree_by_name + .entry(inst.name().to_string()) + .or_default() + .push(i); + } + } + + for (name, snap_indices) in &snap_by_name { + let Some(tree_indices) = tree_by_name.get(*name) else { + continue; + }; + + // Case 1: exactly one on each side → instant match. + if snap_indices.len() == 1 && tree_indices.len() == 1 { + matched.push((snap_indices[0], tree_indices[0])); + continue; + } + + // Case 2: try ClassName narrowing. + let mut snap_by_class: HashMap<&str, Vec> = HashMap::new(); + for &si in snap_indices { + if let Some(snap) = &snap_available[si] { + snap_by_class.entry(&snap.class_name).or_default().push(si); + } + } + + let mut tree_by_class: HashMap> = HashMap::new(); + for &ti in tree_indices { + if let Some(inst) = tree.get_instance(tree_children[ti]) { + tree_by_class + .entry(inst.class_name().to_string()) + .or_default() + .push(ti); + } + } + + for (class, snap_class_indices) in &snap_by_class { + if let Some(tree_class_indices) = tree_by_class.get(*class) { + if snap_class_indices.len() == 1 && tree_class_indices.len() == 1 { + matched.push((snap_class_indices[0], tree_class_indices[0])); + } + } + } + } +} + +/// Pass 3: Pairwise similarity scoring within same-name groups. +fn pass3_similarity( + snap_available: &[Option], + tree_children: &[Ref], + tree_available: &mut [bool], + tree: &RojoTree, + matched: &mut Vec<(usize, usize)>, +) { + // Track which snap indices have been matched in this pass. + let mut snap_matched: Vec = vec![false; snap_available.len()]; + + // Group remaining by name. + let mut snap_by_name: HashMap<&str, Vec> = HashMap::new(); + for (i, snap_opt) in snap_available.iter().enumerate() { + if let Some(snap) = snap_opt { + snap_by_name.entry(&snap.name).or_default().push(i); + } + } + + let mut tree_by_name: HashMap> = HashMap::new(); + for (i, &child_ref) in tree_children.iter().enumerate() { + if !tree_available[i] { + continue; + } + if let Some(inst) = tree.get_instance(child_ref) { + tree_by_name + .entry(inst.name().to_string()) + .or_default() + .push(i); + } + } + + for (name, snap_indices) in &snap_by_name { + let Some(tree_indices) = tree_by_name.get(*name) else { + continue; + }; + + // Build pairs with similarity scores. + let mut pairs: Vec<(usize, usize, u32)> = Vec::new(); + for &si in snap_indices { + if snap_matched[si] { + continue; + } + let snap = match &snap_available[si] { + Some(s) => s, + None => continue, + }; + for &ti in tree_indices { + if !tree_available[ti] { + continue; + } + let inst = match tree.get_instance(tree_children[ti]) { + Some(i) => i, + None => continue, + }; + let score = compute_forward_similarity(snap, &inst); + pairs.push((si, ti, score)); + } + } + + // Sort descending by score, tiebreak by original order. + pairs.sort_by(|a, b| b.2.cmp(&a.2).then_with(|| a.0.cmp(&b.0).then(a.1.cmp(&b.1)))); + + // Greedy assignment. + for (si, ti, _score) in pairs { + if snap_matched[si] || !tree_available[ti] { + continue; + } + matched.push((si, ti)); + snap_matched[si] = true; + tree_available[ti] = false; + } + } +} + +/// Compute similarity between a snapshot child and a tree child. +fn compute_forward_similarity(snap: &InstanceSnapshot, inst: &InstanceWithMeta) -> u32 { + let mut score: u32 = 0; + + if snap.class_name.as_str() == inst.class_name().as_str() { + score += 100; + } + + // Children count similarity. + if snap.children.len() == inst.children().len() { + score += 20; + } + + score +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::snapshot::{InstanceMetadata, InstanceSnapshot, RojoTree}; + use rbx_dom_weak::ustr; + use std::borrow::Cow; + + fn make_snapshot(name: &str, class: &str) -> InstanceSnapshot { + InstanceSnapshot { + snapshot_id: Ref::none(), + metadata: InstanceMetadata::default(), + name: Cow::Owned(name.to_string()), + class_name: ustr(class), + properties: Default::default(), + children: Vec::new(), + } + } + + fn make_tree_with_children(children: &[(&str, &str)]) -> (RojoTree, Vec) { + let child_snapshots: Vec = children + .iter() + .map(|(name, class)| make_snapshot(name, class)) + .collect(); + let root_snap = InstanceSnapshot { + snapshot_id: Ref::none(), + metadata: InstanceMetadata::default(), + name: Cow::Borrowed("DataModel"), + class_name: ustr("DataModel"), + properties: Default::default(), + children: child_snapshots, + }; + let tree = RojoTree::new(root_snap); + let root_id = tree.get_root_id(); + let child_ids: Vec = tree.get_instance(root_id).unwrap().children().to_vec(); + (tree, child_ids) + } + + #[test] + fn unique_names_match() { + let snaps = vec![make_snapshot("Alpha", "Folder"), make_snapshot("Beta", "Script")]; + let (tree, children) = make_tree_with_children(&[("Alpha", "Folder"), ("Beta", "Script")]); + + let result = match_forward(snaps, &children, &tree); + assert_eq!(result.matched.len(), 2); + assert!(result.unmatched_snapshot.is_empty()); + assert!(result.unmatched_tree.is_empty()); + } + + #[test] + fn class_narrowing() { + let snaps = vec![ + make_snapshot("Handler", "Script"), + make_snapshot("Handler", "ModuleScript"), + ]; + let (tree, children) = + make_tree_with_children(&[("Handler", "ModuleScript"), ("Handler", "Script")]); + + let result = match_forward(snaps, &children, &tree); + assert_eq!(result.matched.len(), 2); + + for (snap, tree_ref) in &result.matched { + let inst = tree.get_instance(*tree_ref).unwrap(); + assert_eq!(snap.class_name.as_str(), inst.class_name().as_str()); + } + } + + #[test] + fn unmatched_both_sides() { + let snaps = vec![make_snapshot("A", "Folder"), make_snapshot("NewOnly", "Folder")]; + let (tree, children) = make_tree_with_children(&[("A", "Folder"), ("OldOnly", "Folder")]); + + let result = match_forward(snaps, &children, &tree); + assert_eq!(result.matched.len(), 1); + assert_eq!(result.unmatched_snapshot.len(), 1); + assert_eq!(result.unmatched_tree.len(), 1); + assert_eq!(result.unmatched_snapshot[0].name.as_ref(), "NewOnly"); + } + + #[test] + fn duplicate_names_greedy() { + let snaps = vec![ + make_snapshot("Data", "Folder"), + make_snapshot("Data", "Folder"), + ]; + let (tree, children) = make_tree_with_children(&[("Data", "Folder"), ("Data", "Folder")]); + + let result = match_forward(snaps, &children, &tree); + assert_eq!(result.matched.len(), 2); + assert!(result.unmatched_snapshot.is_empty()); + assert!(result.unmatched_tree.is_empty()); + } + + #[test] + fn empty_both() { + let snaps: Vec = vec![]; + let (tree, children) = make_tree_with_children(&[]); + + let result = match_forward(snaps, &children, &tree); + assert!(result.matched.is_empty()); + } +} diff --git a/src/snapshot/mod.rs b/src/snapshot/mod.rs index 5aacdad6..30a22238 100644 --- a/src/snapshot/mod.rs +++ b/src/snapshot/mod.rs @@ -49,6 +49,7 @@ #![allow(dead_code)] mod instance_snapshot; +pub mod matching; mod metadata; mod patch; mod patch_apply; diff --git a/src/snapshot/patch_compute.rs b/src/snapshot/patch_compute.rs index d894d687..f69de980 100644 --- a/src/snapshot/patch_compute.rs +++ b/src/snapshot/patch_compute.rs @@ -217,63 +217,34 @@ fn compute_children_patches( id: Ref, patch_set: &mut PatchSet, ) { + use super::matching::match_forward; + let instance = tree .get_instance(id) .expect("Instance did not exist in tree"); - let instance_children = instance.children(); + let instance_children = instance.children().to_vec(); + let snapshot_children = take(&mut snapshot.children); - let mut paired_instances = vec![false; instance_children.len()]; + // Use the 3-pass matching algorithm instead of greedy name+class. + let match_result = match_forward(snapshot_children, &instance_children, tree); - for snapshot_child in take(&mut snapshot.children) { - let matching_instance = - instance_children - .iter() - .enumerate() - .find(|(instance_index, instance_child_id)| { - if paired_instances[*instance_index] { - return false; - } - - let instance_child = tree - .get_instance(**instance_child_id) - .expect("Instance did not exist in tree"); - - if snapshot_child.name == instance_child.name() - && snapshot_child.class_name == instance_child.class_name() - { - paired_instances[*instance_index] = true; - return true; - } - - false - }); - - match matching_instance { - Some((_, instance_child_id)) => { - compute_patch_set_internal( - context, - snapshot_child, - tree, - *instance_child_id, - patch_set, - ); - } - None => { - patch_set.added_instances.push(PatchAdd { - parent_id: id, - instance: snapshot_child, - }); - } - } + // Matched pairs: recursively compute patches. + for (snapshot_child, tree_child_id) in match_result.matched { + compute_patch_set_internal(context, snapshot_child, tree, tree_child_id, patch_set); } - for (instance_index, instance_child_id) in instance_children.iter().enumerate() { - if paired_instances[instance_index] { - continue; - } + // Unmatched snapshots: new instances to be added. + for snapshot_child in match_result.unmatched_snapshot { + patch_set.added_instances.push(PatchAdd { + parent_id: id, + instance: snapshot_child, + }); + } - patch_set.removed_instances.push(*instance_child_id); + // Unmatched tree children: instances to be removed. + for tree_child_id in match_result.unmatched_tree { + patch_set.removed_instances.push(tree_child_id); } } diff --git a/src/snapshot/tree.rs b/src/snapshot/tree.rs index d6e60e9e..68a84d3f 100644 --- a/src/snapshot/tree.rs +++ b/src/snapshot/tree.rs @@ -231,36 +231,51 @@ impl RojoTree { self.specified_id_to_refs.insert(specified, id); } - /// Looks up an instance by its path in the tree (e.g., "SoundService/MySoundGroup"). - /// The path is relative to the root of the tree, with segments separated by "/". - /// Returns the Ref if found, None otherwise. + /// Looks up an instance by its filesystem-name path in the tree. + /// + /// Each segment is matched against the **filesystem name** of child + /// instances (derived from `instigating_source` path), falling back to + /// the instance name for instances without filesystem backing. + /// + /// Path format: `"Workspace/Model.model.json5/Attachment~1.luau"` + /// Segments are separated by `/` (no escaping needed since filesystem + /// names can't contain `/`). pub fn get_instance_by_path(&self, path: &str) -> Option { if path.is_empty() { return Some(self.get_root_id()); } - // Use split_ref_path to handle escaped "/" in instance names. - // An instance named "A/B" is stored as "A\/B" in the path. - let segments = crate::split_ref_path(path); + let segments: Vec<&str> = path.split('/').collect(); let mut current_ref = self.get_root_id(); - // The root of the tree doesn't appear in the path, so we start with - // the root's children. - for segment in segments.iter() { + for segment in &segments { let current = self.inner.get_by_ref(current_ref)?; - let children = current.children(); let mut found = false; for &child_ref in children { - let child = self.inner.get_by_ref(child_ref)?; - if child.name == segment.as_str() { + let fs_name = self.filesystem_name_for(child_ref); + if fs_name.eq_ignore_ascii_case(segment) { current_ref = child_ref; found = true; break; } } + if !found { + // Fallback: try matching by instance name (for backward + // compatibility with legacy ref paths and instances without + // filesystem backing). + for &child_ref in children { + let child = self.inner.get_by_ref(child_ref)?; + if child.name == *segment { + current_ref = child_ref; + found = true; + break; + } + } + } + if !found { return None; } @@ -269,6 +284,24 @@ impl RojoTree { Some(current_ref) } + /// Returns the filesystem name for an instance (the filename or directory + /// name on disk). Derived from `instigating_source` if available, otherwise + /// falls back to the instance name. + pub fn filesystem_name_for(&self, id: Ref) -> String { + if let Some(meta) = self.metadata_map.get(&id) { + if let Some(source) = &meta.instigating_source { + if let Some(name) = source.path().file_name().and_then(|f| f.to_str()) { + return name.to_string(); + } + } + } + // Fallback: use instance name + self.inner + .get_by_ref(id) + .map(|inst| inst.name.clone()) + .unwrap_or_default() + } + fn insert_metadata(&mut self, id: Ref, metadata: InstanceMetadata) { for path in &metadata.relevant_paths { self.path_to_ids.insert(path.clone(), id); diff --git a/src/snapshot_middleware/dir.rs b/src/snapshot_middleware/dir.rs index e26084cd..21744cd8 100644 --- a/src/snapshot_middleware/dir.rs +++ b/src/snapshot_middleware/dir.rs @@ -5,14 +5,11 @@ use std::{ use anyhow::Context; use memofs::{DirEntry, Vfs}; +use rbx_dom_weak::types::Ref; use crate::{ snapshot::{InstanceContext, InstanceMetadata, InstanceSnapshot, InstigatingSource}, - snapshot_middleware::Middleware, - syncback::{ - hash_instance, name_needs_slugify, slugify_name, strip_middleware_extension, FsSnapshot, - SyncbackReturn, SyncbackSnapshot, - }, + syncback::{hash_instance, FsSnapshot, SyncbackReturn, SyncbackSnapshot}, }; use super::{meta_file::DirectoryMetadata, snapshot_from_vfs}; @@ -137,205 +134,174 @@ pub fn syncback_dir<'sync>( pub fn syncback_dir_no_meta<'sync>( snapshot: &SyncbackSnapshot<'sync>, ) -> anyhow::Result> { + use crate::syncback::matching::match_children; + let new_inst = snapshot.new_inst(); let mut children = Vec::new(); let mut removed_children = Vec::new(); - // taken_names tracks claimed bare slugs (without extensions) for dedup. - // Pre-seeded from old tree children's slugified instance names so that - // new-only children correctly dedup against existing siblings, regardless - // of iteration order (see plan: fix_stem-level_dedup §2). + // taken_names tracks claimed full filesystem name components (slug + + // extension for files, directory name for dirs), lowercased. let mut taken_names: HashSet = HashSet::new(); - // Detect duplicate child names (case-insensitive for file system safety). - // We skip duplicates instead of failing, tracking them in stats. - let mut child_name_counts: HashMap = HashMap::new(); - for child_ref in new_inst.children() { - let child = snapshot.get_new_instance(*child_ref).unwrap(); - let lower_name = child.name.to_lowercase(); - *child_name_counts.entry(lower_name).or_insert(0) += 1; - } - - let duplicate_names: HashSet = child_name_counts - .into_iter() - .filter(|(_, count)| *count > 1) - .map(|(name, _)| name) - .collect(); - - // Record duplicate names in stats tracker - if !duplicate_names.is_empty() { - let inst_path = crate::syncback::inst_path(snapshot.new_tree(), snapshot.new); - // Count total instances being skipped (sum of all duplicates) - let mut total_skipped = 0; - for child_ref in new_inst.children() { - let child = snapshot.get_new_instance(*child_ref).unwrap(); - if duplicate_names.contains(&child.name.to_lowercase()) { - total_skipped += 1; - } - } - let duplicate_list: Vec<&str> = duplicate_names.iter().map(|s| s.as_str()).collect(); - snapshot - .stats() - .record_duplicate_names_batch(&inst_path, &duplicate_list, total_skipped); - } - - 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); - } - - // Pre-seed taken_names from old children's actual filesystem dedup keys - // so that new-only children correctly dedup against existing siblings. - // We derive keys from relevant_paths (the real filename on disk) rather - // than slugifying instance names, because an old instance may already - // have a tilde suffix (e.g. A_B~1.luau from prior dedup) that the bare - // slug wouldn't capture. - // Only in incremental mode: in clean mode, old_ref is forced to None - // for all children, so every child is treated as new and pre-seeding - // would cause false collisions (spurious ~1 suffixes). - if snapshot.data.is_incremental() { - for inst in old_child_map.values() { - if let Some(path) = inst.metadata().relevant_paths.first() { - if let Some(filename) = path.file_name().and_then(|f| f.to_str()) { - let middleware = inst.metadata().middleware.unwrap_or(Middleware::Dir); - let dedup_key = - strip_middleware_extension(filename, middleware).to_lowercase(); - taken_names.insert(dedup_key); - } - } else { - // Fallback (shouldn't happen for old instances with disk presence) - let name = inst.name(); - let slug = if name_needs_slugify(name) { - slugify_name(name).to_lowercase() - } else { - name.to_lowercase() - }; - taken_names.insert(slug); - } - } - } - - // Sort children alphabetically for deterministic dedup ordering. - // Without this, DOM iteration order determines which sibling gets - // the base slug vs ~1, causing non-deterministic output across - // different serializations of the same place. - let mut sorted_children: Vec<_> = new_inst - .children() - .iter() - .map(|r| (*r, snapshot.get_new_instance(*r).unwrap().name.clone())) - .collect(); - sorted_children.sort_by(|(_, a), (_, b)| a.cmp(b)); - - for (new_child_ref, _) in &sorted_children { - let new_child = snapshot.get_new_instance(*new_child_ref).unwrap(); - - // Skip children with duplicate names - cannot reliably sync them - if duplicate_names.contains(&new_child.name.to_lowercase()) { - old_child_map.remove(new_child.name.as_str()); - continue; - } - - // Skip instances of ignored classes - if snapshot.should_ignore_class(&new_child.class) { - // Also remove from old_child_map so it won't be marked as removed - old_child_map.remove(new_child.name.as_str()); + // Filter new children: exclude ignored classes and ignoreTrees. + let eligible_new: Vec = new_inst + .children() + .iter() + .copied() + .filter(|r| { + let child = snapshot.get_new_instance(*r).unwrap(); + if snapshot.should_ignore_class(&child.class) { log::debug!( "Skipping instance {} because its class {} is ignored", - new_child.name, - new_child.class + child.name, + child.class ); - continue; + return false; } - // Skip instances matching ignoreTrees - if snapshot.should_ignore_tree(*new_child_ref) { - old_child_map.remove(new_child.name.as_str()); - continue; + if snapshot.should_ignore_tree(*r) { + return false; } - if let Some(old_child) = old_child_map.remove(new_child.name.as_str()) { - if old_child.metadata().relevant_paths.is_empty() { + true + }) + .collect(); + + if let Some(old_inst) = snapshot.old_inst() { + // Build old_ref → InstanceWithMeta lookup for metadata access. + let mut old_meta_map: HashMap = HashMap::new(); + for &child_ref in old_inst.children() { + let inst = snapshot.get_old_instance(child_ref).unwrap(); + old_meta_map.insert(child_ref, inst); + } + + // Filter old children: exclude those without disk presence, from + // project files, or with ignored classes. + let eligible_old: Vec = old_inst + .children() + .iter() + .copied() + .filter(|r| { + let inst = old_meta_map.get(r).unwrap(); + if inst.metadata().relevant_paths.is_empty() { log::debug!( - "Skipping instance {} because it doesn't exist on the disk", - old_child.name() + "Excluding old instance {} from matching (no disk presence)", + inst.name() ); - continue; - } else if matches!( - old_child.metadata().instigating_source, + return false; + } + if matches!( + inst.metadata().instigating_source, Some(InstigatingSource::ProjectNode { .. }) ) { log::debug!( - "Skipping instance {} because it originates in a project file", - old_child.name() + "Excluding old instance {} from matching (project file)", + inst.name() ); - continue; + return false; + } + if snapshot.should_ignore_class(inst.class_name().as_str()) { + return false; + } + true + }) + .collect(); + + // Run the 3-pass matching algorithm to pair new ↔ old children. + let match_result = match_children( + &eligible_new, + &eligible_old, + snapshot.new_tree(), + snapshot.old_tree(), + None, // TODO: pass precomputed hashes for better similarity + None, + ); + + // Pre-seed taken_names from ALL matched old children's filesystem + // names so that unmatched new children correctly dedup. + if snapshot.data.is_incremental() { + for &(_, old_ref) in &match_result.matched { + if let Some(inst) = old_meta_map.get(&old_ref) { + if let Some(path) = inst.metadata().relevant_paths.first() { + if let Some(filename) = path.file_name().and_then(|f| f.to_str()) { + taken_names.insert(filename.to_lowercase()); + } + } + } + } + // Also seed from unmatched old (they still occupy filesystem names). + for &old_ref in &match_result.unmatched_old { + if let Some(inst) = old_meta_map.get(&old_ref) { + if let Some(path) = inst.metadata().relevant_paths.first() { + if let Some(filename) = path.file_name().and_then(|f| f.to_str()) { + taken_names.insert(filename.to_lowercase()); + } + } } - // This child exists in both doms. Pass it on. - let (child_snap, _needs_meta, dedup_key) = snapshot.with_joined_path( - *new_child_ref, - Some(old_child.id()), - &taken_names, - )?; - taken_names.insert(dedup_key.to_lowercase()); - children.push(child_snap); - } else { - // The child only exists in the the new dom - let (child_snap, _needs_meta, dedup_key) = - snapshot.with_joined_path(*new_child_ref, None, &taken_names)?; - taken_names.insert(dedup_key.to_lowercase()); - children.push(child_snap); } } - // Any children that are in the old dom but not the new one are removed. - // Filter out instances of ignored classes and duplicates from removal. - removed_children.extend(old_child_map.into_values().filter(|inst| { - // Don't remove duplicates - we're skipping them entirely - if duplicate_names.contains(&inst.name().to_lowercase()) { - return false; + + // Sort matched pairs by new child name for deterministic ordering. + let mut sorted_matched = match_result.matched; + sorted_matched.sort_by(|(a, _), (b, _)| { + let a_name = &snapshot.get_new_instance(*a).unwrap().name; + let b_name = &snapshot.get_new_instance(*b).unwrap().name; + a_name.cmp(b_name) + }); + + // Process matched pairs: preserve existing filesystem assignment. + for (new_ref, old_ref) in &sorted_matched { + let (child_snap, _needs_meta, dedup_key) = snapshot.with_joined_path( + *new_ref, + Some(*old_ref), + &taken_names, + )?; + taken_names.insert(dedup_key.to_lowercase()); + children.push(child_snap); + } + + // Sort unmatched new by name for deterministic dedup ordering. + let mut sorted_unmatched_new = match_result.unmatched_new; + sorted_unmatched_new.sort_by(|a, b| { + let a_name = &snapshot.get_new_instance(*a).unwrap().name; + let b_name = &snapshot.get_new_instance(*b).unwrap().name; + a_name.cmp(b_name) + }); + + // Process unmatched new children: generate new filenames. + for new_ref in &sorted_unmatched_new { + let (child_snap, _needs_meta, dedup_key) = + snapshot.with_joined_path(*new_ref, None, &taken_names)?; + taken_names.insert(dedup_key.to_lowercase()); + children.push(child_snap); + } + + // Unmatched old children are removed (instance was deleted). + // Filter out ignored classes from removal. + for old_ref in match_result.unmatched_old { + if let Some(inst) = old_meta_map.get(&old_ref) { + if !snapshot.should_ignore_class(inst.class_name().as_str()) { + removed_children.push(*inst); + } else { + log::debug!( + "Not removing instance {} because its class {} is ignored", + inst.name(), + inst.class_name() + ); + } } - if snapshot.should_ignore_class(inst.class_name().as_str()) { - log::debug!( - "Not removing instance {} because its class {} is ignored", - inst.name(), - inst.class_name() - ); - false - } else { - true - } - })); + } } else { - // There is no old instance. Just add every child. + // No old instance (clean mode). All children are new. // Sort alphabetically for deterministic dedup ordering. - let mut sorted_children: Vec<_> = new_inst - .children() - .iter() - .map(|r| (*r, snapshot.get_new_instance(*r).unwrap().name.clone())) - .collect(); - sorted_children.sort_by(|(_, a), (_, b)| a.cmp(b)); + let mut sorted_new = eligible_new; + sorted_new.sort_by(|a, b| { + let a_name = &snapshot.get_new_instance(*a).unwrap().name; + let b_name = &snapshot.get_new_instance(*b).unwrap().name; + a_name.cmp(b_name) + }); - for (new_child_ref, _) in &sorted_children { - let new_child = snapshot.get_new_instance(*new_child_ref).unwrap(); - - // Skip children with duplicate names - cannot reliably sync them - if duplicate_names.contains(&new_child.name.to_lowercase()) { - continue; - } - - // Skip instances of ignored classes - if snapshot.should_ignore_class(&new_child.class) { - log::debug!( - "Skipping instance {} because its class {} is ignored", - new_child.name, - new_child.class - ); - continue; - } - // Skip instances matching ignoreTrees - if snapshot.should_ignore_tree(*new_child_ref) { - continue; - } + for new_child_ref in &sorted_new { let (child_snap, _needs_meta, dedup_key) = snapshot.with_joined_path(*new_child_ref, None, &taken_names)?; taken_names.insert(dedup_key.to_lowercase()); diff --git a/src/snapshot_middleware/project.rs b/src/snapshot_middleware/project.rs index 88deac44..338907b2 100644 --- a/src/snapshot_middleware/project.rs +++ b/src/snapshot_middleware/project.rs @@ -1,6 +1,6 @@ use std::{ borrow::Cow, - collections::{BTreeMap, HashMap, VecDeque}, + collections::{BTreeMap, HashMap, HashSet, VecDeque}, path::Path, }; @@ -21,7 +21,7 @@ use crate::{ }, snapshot_middleware::Middleware, syncback::{ - filter_properties, inst_path, name_needs_slugify, slugify_name, strip_middleware_extension, + filter_properties, inst_path, name_needs_slugify, slugify_name, FsSnapshot, SyncbackReturn, SyncbackSnapshot, }, variant_eq::variant_eq, @@ -584,30 +584,87 @@ pub fn syncback_project<'sync>( project_node_property_syncback_no_path(snapshot, new_inst, node); } + // Detect duplicate children using case-insensitive comparison. + // Project KEYS are unique by JSON definition; only filesystem children + // under project nodes may collide. Duplicates are skipped with a warning + // for now -- the dedup system (Phase 1c/1d) will handle them properly. + let mut new_duplicate_names: HashSet = HashSet::new(); + { + let mut seen: HashSet = HashSet::new(); + for child_ref in new_inst.children() { + let child = snapshot + .get_new_instance(*child_ref) + .expect("all children of Instances should be in new DOM"); + let lower = child.name.to_lowercase(); + if !seen.insert(lower.clone()) { + new_duplicate_names.insert(lower); + } + } + } + if !new_duplicate_names.is_empty() { + let parent_path = inst_path(snapshot.new_tree(), new_inst.referent()); + for child_ref in new_inst.children() { + let child = snapshot + .get_new_instance(*child_ref) + .expect("all children of Instances should be in new DOM"); + if new_duplicate_names.contains(&child.name.to_lowercase()) { + log::warn!( + "Skipping duplicate-named child '{}' under ProjectNode '{}' -- \ + cannot reliably sync yet. Full path: {}/{}", + child.name, + parent_path, + parent_path, + child.name + ); + } + } + } for child_ref in new_inst.children() { let child = snapshot .get_new_instance(*child_ref) .expect("all children of Instances should be in new DOM"); - if new_child_map.insert(&child.name, child).is_some() { - let parent_path = inst_path(snapshot.new_tree(), new_inst.referent()); - anyhow::bail!( - "Instances that are direct children of an Instance that is made by a project file \ - must have a unique name.\nThe child '{}' is duplicated in the place file.\n\ - Full path: {}/{}", child.name, parent_path, child.name - ); + if !new_duplicate_names.contains(&child.name.to_lowercase()) { + new_child_map.insert(&child.name, child); + } + } + + let mut old_duplicate_names: HashSet = HashSet::new(); + { + let mut seen: HashSet = HashSet::new(); + for child_ref in old_inst.children() { + let child = snapshot + .get_old_instance(*child_ref) + .expect("all children of Instances should be in old DOM"); + let lower = child.name().to_lowercase(); + if !seen.insert(lower.clone()) { + old_duplicate_names.insert(lower); + } + } + } + if !old_duplicate_names.is_empty() { + let parent_path = inst_path(snapshot.old_tree(), old_inst.id()); + for child_ref in old_inst.children() { + let child = snapshot + .get_old_instance(*child_ref) + .expect("all children of Instances should be in old DOM"); + if old_duplicate_names.contains(&child.name().to_lowercase()) { + log::warn!( + "Skipping duplicate-named child '{}' under ProjectNode '{}' -- \ + cannot reliably sync yet. Full path: {}/{}", + child.name(), + parent_path, + parent_path, + child.name() + ); + } } } for child_ref in old_inst.children() { let child = snapshot .get_old_instance(*child_ref) .expect("all children of Instances should be in old DOM"); - if old_child_map.insert(child.name(), child).is_some() { - let parent_path = inst_path(snapshot.old_tree(), old_inst.id()); - anyhow::bail!( - "Instances that are direct children of an Instance that is made by a project file \ - must have a unique name.\nThe child '{}' is duplicated on the file system.\n\ - Full path: {}/{}", child.name(), parent_path, child.name() - ); + if !old_duplicate_names.contains(&child.name().to_lowercase()) { + old_child_map.insert(child.name(), child); } } @@ -654,33 +711,27 @@ pub fn syncback_project<'sync>( // All of the children in this loop are by their nature not in the // project, so we just need to run syncback on them. // - // Track taken names (bare slugs) per parent directory so siblings - // under the same parent deduplicate against each other. + // Track taken names (full filesystem name components, lowercased) + // per parent directory so siblings under the same parent deduplicate + // against each other using filesystem-level collision detection. let mut taken_names_per_dir: std::collections::HashMap< std::path::PathBuf, std::collections::HashSet, > = std::collections::HashMap::new(); - // Pre-seed taken_names from old children's actual filesystem dedup keys - // so that new-only children correctly dedup against existing siblings. - // We derive keys from relevant_paths (the real filename on disk) rather - // than slugifying instance names, because an old instance may already - // have a tilde suffix (e.g. A_B~1.luau from prior dedup) that the bare - // slug wouldn't capture. - // Only in incremental mode: in clean mode, old_ref is forced to None - // for all children, so every child is treated as new and pre-seeding - // would cause false collisions (spurious ~1 suffixes). + // Pre-seed taken_names from old children's actual filesystem names + // (full filename including extension) so that new-only children + // correctly dedup against existing siblings. if snapshot.data.is_incremental() { if let Some(parent_path) = ref_to_path_map.get(&new_inst.referent()) { let taken = taken_names_per_dir.entry(parent_path.clone()).or_default(); for old_child in old_child_map.values() { if let Some(path) = old_child.metadata().relevant_paths.first() { if let Some(filename) = path.file_name().and_then(|f| f.to_str()) { - let middleware = - old_child.metadata().middleware.unwrap_or(Middleware::Dir); - let dedup_key = - strip_middleware_extension(filename, middleware).to_lowercase(); - taken.insert(dedup_key); + // Use the full filename (including extension) as the + // dedup key so that "Foo" (dir) and "Foo.server.luau" + // (file) do NOT collide. + taken.insert(filename.to_lowercase()); } } else { // Fallback (shouldn't happen for old instances with disk presence) diff --git a/src/syncback/dedup_suffix.rs b/src/syncback/dedup_suffix.rs new file mode 100644 index 00000000..5b6097b2 --- /dev/null +++ b/src/syncback/dedup_suffix.rs @@ -0,0 +1,293 @@ +//! Dedup suffix lifecycle management. +//! +//! When instances have duplicate names, they receive `~N` suffixes on the +//! filesystem (e.g., `Foo`, `Foo~1`, `Foo~2`). This module provides shared +//! helpers for managing suffix cleanup when instances are deleted. +//! +//! Rules: +//! - **Gap-tolerant:** deleting a suffixed instance does NOT renumber remaining +//! siblings. Gaps (`Foo`, `Foo~2` without `~1`) are harmless. +//! - **Base-name promotion:** when the base-name holder is deleted and multiple +//! siblings remain, the lowest-numbered suffix is promoted to base name. +//! - **Group-to-1 cleanup:** when a deletion reduces the dedup group to exactly +//! 1 remaining instance, its suffix is removed entirely. + +use std::path::{Path, PathBuf}; + +/// Result of analyzing the dedup group after a deletion. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum DedupCleanupAction { + /// No rename needed. Either the deleted instance had a suffix and others + /// remain, or the group is still large enough that no cleanup is required. + None, + + /// The dedup group shrank to exactly 1 remaining instance. + /// Its suffix must be removed (renamed to the clean base name). + RemoveSuffix { + /// The current path (with suffix) that should be renamed. + from: PathBuf, + /// The target path (clean, no suffix). + to: PathBuf, + }, + + /// The base-name holder was deleted and the lowest-numbered suffix must be + /// promoted to take the base name. + PromoteLowest { + /// The current path of the lowest-suffixed sibling. + from: PathBuf, + /// The base-name path (clean, no suffix). + to: PathBuf, + }, +} + +/// Parses a dedup suffix from a filesystem stem. +/// +/// Given `"Foo~3"`, returns `Some(("Foo", 3))`. +/// Given `"Foo"`, returns `None`. +/// Given `"Foo~abc"` (non-numeric), returns `None`. +pub fn parse_dedup_suffix(stem: &str) -> Option<(&str, u32)> { + if let Some(tilde_pos) = stem.rfind('~') { + let base = &stem[..tilde_pos]; + let suffix_str = &stem[tilde_pos + 1..]; + if let Ok(n) = suffix_str.parse::() { + if n > 0 { + return Some((base, n)); + } + } + } + None +} + +/// Checks whether a filename stem has a dedup suffix. +pub fn has_dedup_suffix(stem: &str) -> bool { + parse_dedup_suffix(stem).is_some() +} + +/// Builds a dedup'd filename from a base stem, optional suffix number, and +/// extension. +/// +/// - `build_dedup_name("Foo", None, Some("server.luau"))` → `"Foo.server.luau"` +/// - `build_dedup_name("Foo", Some(2), Some("server.luau"))` → `"Foo~2.server.luau"` +/// - `build_dedup_name("Foo", Some(1), None)` → `"Foo~1"` (directory) +/// - `build_dedup_name("Foo", None, None)` → `"Foo"` (directory) +pub fn build_dedup_name(base_stem: &str, suffix: Option, extension: Option<&str>) -> String { + let stem = match suffix { + Some(n) => format!("{base_stem}~{n}"), + None => base_stem.to_string(), + }; + match extension { + Some(ext) => format!("{stem}.{ext}"), + None => stem, + } +} + +/// Determines the cleanup action needed after removing an instance from a +/// dedup group. +/// +/// `remaining_stems` contains the filesystem stems of all siblings that STILL +/// exist after the deletion (not including the deleted one). These should share +/// the same base stem (before `~N`). If the base-name holder was deleted, it +/// should NOT appear in `remaining_stems`. +/// +/// `deleted_was_base` indicates whether the deleted instance held the base name +/// (no suffix). +/// +/// Returns the appropriate cleanup action. +pub fn compute_cleanup_action( + base_stem: &str, + extension: Option<&str>, + remaining_stems: &[String], + deleted_was_base: bool, + parent_dir: &Path, +) -> DedupCleanupAction { + match remaining_stems.len() { + 0 => { + // No siblings remain -- nothing to clean up. + DedupCleanupAction::None + } + 1 => { + // Group shrank to 1: remove the survivor's suffix entirely. + let survivor = &remaining_stems[0]; + let from_name = build_dedup_name( + base_stem, + parse_dedup_suffix(survivor).map(|(_, n)| n), + extension, + ); + let to_name = build_dedup_name(base_stem, None, extension); + if from_name == to_name { + // Survivor already has the clean name (was the base holder + // and only one other was deleted). + DedupCleanupAction::None + } else { + DedupCleanupAction::RemoveSuffix { + from: parent_dir.join(&from_name), + to: parent_dir.join(&to_name), + } + } + } + _ => { + // Multiple siblings remain. + if deleted_was_base { + // The base-name holder was deleted. Promote the lowest suffix. + let mut suffix_numbers: Vec = remaining_stems + .iter() + .filter_map(|s| parse_dedup_suffix(s).map(|(_, n)| n)) + .collect(); + suffix_numbers.sort(); + + if let Some(&lowest) = suffix_numbers.first() { + let from_name = build_dedup_name(base_stem, Some(lowest), extension); + let to_name = build_dedup_name(base_stem, None, extension); + DedupCleanupAction::PromoteLowest { + from: parent_dir.join(&from_name), + to: parent_dir.join(&to_name), + } + } else { + // All remaining are base-name holders? Shouldn't happen. + DedupCleanupAction::None + } + } else { + // A suffixed instance was deleted. Gap-tolerant: no rename. + DedupCleanupAction::None + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use std::path::PathBuf; + + #[test] + fn parse_suffix_basic() { + assert_eq!(parse_dedup_suffix("Foo~1"), Some(("Foo", 1))); + assert_eq!(parse_dedup_suffix("Foo~2"), Some(("Foo", 2))); + assert_eq!(parse_dedup_suffix("Foo~10"), Some(("Foo", 10))); + } + + #[test] + fn parse_suffix_none() { + assert_eq!(parse_dedup_suffix("Foo"), None); + assert_eq!(parse_dedup_suffix("Foo~0"), None); // 0 is not valid + assert_eq!(parse_dedup_suffix("Foo~abc"), None); + assert_eq!(parse_dedup_suffix("Foo~"), None); + } + + #[test] + fn parse_suffix_complex_stems() { + assert_eq!(parse_dedup_suffix("A_B~3"), Some(("A_B", 3))); + assert_eq!(parse_dedup_suffix("My Script~1"), Some(("My Script", 1))); + } + + #[test] + fn has_suffix() { + assert!(has_dedup_suffix("Foo~1")); + assert!(!has_dedup_suffix("Foo")); + assert!(!has_dedup_suffix("Foo~abc")); + } + + #[test] + fn build_name_file() { + assert_eq!( + build_dedup_name("Foo", None, Some("server.luau")), + "Foo.server.luau" + ); + assert_eq!( + build_dedup_name("Foo", Some(1), Some("server.luau")), + "Foo~1.server.luau" + ); + assert_eq!( + build_dedup_name("Foo", Some(2), Some("luau")), + "Foo~2.luau" + ); + } + + #[test] + fn build_name_dir() { + assert_eq!(build_dedup_name("Foo", None, None), "Foo"); + assert_eq!(build_dedup_name("Foo", Some(1), None), "Foo~1"); + } + + #[test] + fn cleanup_gap_tolerant() { + // Delete ~1 from {Foo, Foo~1, Foo~2}: no rename needed. + let remaining = vec!["Foo".to_string(), "Foo~2".to_string()]; + let action = compute_cleanup_action("Foo", None, &remaining, false, Path::new("/parent")); + assert_eq!(action, DedupCleanupAction::None); + } + + #[test] + fn cleanup_group_to_one() { + // Delete base from {Foo, Foo~1}: one remains, remove suffix. + let remaining = vec!["Foo~1".to_string()]; + let action = compute_cleanup_action("Foo", None, &remaining, true, Path::new("/parent")); + assert_eq!( + action, + DedupCleanupAction::RemoveSuffix { + from: PathBuf::from("/parent/Foo~1"), + to: PathBuf::from("/parent/Foo"), + } + ); + } + + #[test] + fn cleanup_group_to_one_file() { + // Delete base from {Foo.luau, Foo~1.luau}: one remains, remove suffix. + let remaining = vec!["Foo~1".to_string()]; + let action = + compute_cleanup_action("Foo", Some("luau"), &remaining, true, Path::new("/parent")); + assert_eq!( + action, + DedupCleanupAction::RemoveSuffix { + from: PathBuf::from("/parent/Foo~1.luau"), + to: PathBuf::from("/parent/Foo.luau"), + } + ); + } + + #[test] + fn cleanup_base_deleted_promote_lowest() { + // Delete base from {Foo, Foo~1, Foo~2}: promote ~1 to base. + let remaining = vec!["Foo~1".to_string(), "Foo~2".to_string()]; + let action = compute_cleanup_action("Foo", None, &remaining, true, Path::new("/parent")); + assert_eq!( + action, + DedupCleanupAction::PromoteLowest { + from: PathBuf::from("/parent/Foo~1"), + to: PathBuf::from("/parent/Foo"), + } + ); + } + + #[test] + fn cleanup_base_deleted_promote_with_gap() { + // Delete base from {Foo, Foo~2, Foo~5} (gap at ~1): promote ~2 to base. + let remaining = vec!["Foo~2".to_string(), "Foo~5".to_string()]; + let action = compute_cleanup_action("Foo", None, &remaining, true, Path::new("/parent")); + assert_eq!( + action, + DedupCleanupAction::PromoteLowest { + from: PathBuf::from("/parent/Foo~2"), + to: PathBuf::from("/parent/Foo"), + } + ); + } + + #[test] + fn cleanup_suffix_deleted_no_action() { + // Delete ~1 from {Foo, Foo~1}: group shrinks to 1. + // Since deleted_was_base=false and only 1 remains, remove its suffix. + // Wait - if ~1 is deleted and only Foo remains, Foo has no suffix. No rename needed. + let remaining = vec!["Foo".to_string()]; + let action = compute_cleanup_action("Foo", None, &remaining, false, Path::new("/parent")); + // "Foo" is already the clean name, so no action. + assert_eq!(action, DedupCleanupAction::None); + } + + #[test] + fn cleanup_no_remaining() { + let action = compute_cleanup_action("Foo", None, &[], false, Path::new("/parent")); + assert_eq!(action, DedupCleanupAction::None); + } +} diff --git a/src/syncback/file_names.rs b/src/syncback/file_names.rs index 9f8e1b43..118adfa6 100644 --- a/src/syncback/file_names.rs +++ b/src/syncback/file_names.rs @@ -15,10 +15,13 @@ use crate::{snapshot::InstanceWithMeta, snapshot_middleware::Middleware}; /// - `filename`: The full filesystem name (including extension for file middleware). /// - `needs_meta_name`: `true` when the filesystem name differs from the instance /// name (meaning a `name` field must be written in metadata). -/// - `dedup_key`: The bare slug (without extension) that callers must insert into -/// `taken_names`. This is the name-level identifier used for collision detection. -/// Callers should **always** use `dedup_key` (not `filename`) when accumulating -/// into `taken_names`. +/// - `dedup_key`: The **full filesystem name component** (slug + extension for +/// files, directory name for dirs). Callers should insert +/// `dedup_key.to_lowercase()` into `taken_names`. +/// +/// This means `Folder "Foo"` (key `"Foo"`) and `Script "Foo"` (key +/// `"Foo.server.luau"`) do NOT collide, while two `Folder "Foo"` instances +/// (both key `"Foo"`) DO collide and receive dedup suffixes. /// /// If `old_inst` exists, its existing path is preserved (incremental mode). /// For new instances, names with forbidden chars are slugified and deduplicated @@ -35,10 +38,9 @@ pub fn name_for_inst<'a>( .file_name() .and_then(|s| s.to_str()) .context("sources on the file system should be valid unicode and not be stubs")?; - // Derive dedup_key by stripping the middleware extension from the - // filename. For Dir middleware the extension is empty so this is a - // no-op. This keeps extension logic inside this function. - let dedup_key = strip_middleware_extension(name, middleware); + // dedup_key is the full filesystem name (including extension). + // For Dir middleware the filename IS the directory name (no extension). + let dedup_key = name.to_string(); Ok((Cow::Borrowed(name), false, dedup_key)) } else { anyhow::bail!( @@ -55,32 +57,30 @@ pub fn name_for_inst<'a>( new_inst.name.clone() }; - match middleware { + let is_dir = matches!( + middleware, Middleware::Dir - | Middleware::CsvDir - | Middleware::ServerScriptDir - | Middleware::ClientScriptDir - | Middleware::ModuleScriptDir - | Middleware::PluginScriptDir - | Middleware::LocalScriptDir - | Middleware::LegacyScriptDir => { - let deduped = deduplicate_name(&base, taken_names); - let needs_meta = needs_slugify || deduped != base; - let dedup_key = deduped.clone(); - Ok((Cow::Owned(deduped), needs_meta, dedup_key)) - } - _ => { - let extension = extension_for_middleware(middleware); - let deduped = deduplicate_name(&base, taken_names); - let needs_meta = needs_slugify || deduped != base; - let dedup_key = deduped.clone(); - Ok(( - Cow::Owned(format!("{deduped}.{extension}")), - needs_meta, - dedup_key, - )) - } - } + | Middleware::CsvDir + | Middleware::ServerScriptDir + | Middleware::ClientScriptDir + | Middleware::ModuleScriptDir + | Middleware::PluginScriptDir + | Middleware::LocalScriptDir + | Middleware::LegacyScriptDir + ); + + let extension = if is_dir { + None + } else { + Some(extension_for_middleware(middleware)) + }; + + let (deduped_slug, full_fs_name) = + deduplicate_name_with_ext(&base, extension, taken_names); + let needs_meta = needs_slugify || deduped_slug != base; + + let filename = full_fs_name.clone(); + Ok((Cow::Owned(filename), needs_meta, full_fs_name)) } } @@ -268,12 +268,46 @@ pub fn slugify_name(name: &str) -> String { result } -/// Appends ~1, ~2, etc. to avoid collisions. Returns the name as-is if -/// unclaimed. +/// Appends ~1, ~2, etc. to the slug to avoid filesystem-level collisions. +/// Returns `(deduped_slug, full_fs_name)`. +/// +/// `extension` is `None` for directory middleware, `Some("server.luau")` etc. +/// for file middleware. The collision check uses the full filesystem name +/// (slug + extension) so that `Folder "Foo"` (key `"foo"`) and `Script "Foo"` +/// (key `"foo.server.luau"`) do NOT collide. /// /// Comparisons are **case-insensitive** because Windows and macOS have /// case-insensitive filesystems. `taken_names` must contain **lowercased** -/// entries for this to work correctly. +/// full filesystem name entries for this to work correctly. +pub fn deduplicate_name_with_ext( + base: &str, + extension: Option<&str>, + taken_names: &std::collections::HashSet, +) -> (String, String) { + let build_full = |slug: &str| -> String { + match extension { + Some(ext) => format!("{slug}.{ext}"), + None => slug.to_string(), + } + }; + + let full_name = build_full(base); + if !taken_names.contains(&full_name.to_lowercase()) { + return (base.to_string(), full_name); + } + for i in 1.. { + let candidate_slug = format!("{base}~{i}"); + let candidate_full = build_full(&candidate_slug); + if !taken_names.contains(&candidate_full.to_lowercase()) { + return (candidate_slug, candidate_full); + } + } + unreachable!() +} + +/// Legacy wrapper: appends ~1, ~2, etc. to avoid collisions using bare slug +/// as the collision key. Used by callers that don't yet use full filesystem +/// name dedup (e.g., change_processor.rs renames). pub fn deduplicate_name(base: &str, taken_names: &std::collections::HashSet) -> String { let base_lower = base.to_lowercase(); if !taken_names.contains(&base_lower) { @@ -794,7 +828,7 @@ mod tests { let dom = make_inst("Foo", "ModuleScript"); let child_ref = dom.root().children()[0]; let child = dom.get_by_ref(child_ref).unwrap(); - let taken: HashSet = ["foo".to_string()].into_iter().collect(); + let taken: HashSet = ["foo.luau".to_string()].into_iter().collect(); let (filename, needs_meta, _dk) = name_for_inst(Middleware::ModuleScript, child, None, &taken).unwrap(); @@ -817,11 +851,11 @@ mod tests { #[test] fn name_for_inst_slug_plus_dedup() { - // Name with forbidden chars AND collision with existing slug + // Name with forbidden chars AND collision with existing full filesystem name let dom = make_inst("Hey/Bro", "ModuleScript"); let child_ref = dom.root().children()[0]; let child = dom.get_by_ref(child_ref).unwrap(); - let taken: HashSet = ["hey_bro".to_string()].into_iter().collect(); + let taken: HashSet = ["hey_bro.luau".to_string()].into_iter().collect(); let (filename, needs_meta, _dk) = name_for_inst(Middleware::ModuleScript, child, None, &taken).unwrap(); @@ -1158,9 +1192,8 @@ mod tests { #[test] fn nfi_slug_collision_file_middleware() { - // For file middleware, dedup compares bare slug against taken. - // Using bare names to verify the dedup logic works. - let (f, m) = nfi("Hey/Bro", Middleware::ModuleScript, &["hey_bro"]); + // For file middleware, dedup compares full filesystem name against taken. + let (f, m) = nfi("Hey/Bro", Middleware::ModuleScript, &["hey_bro.luau"]); assert_eq!(f, "Hey_Bro~1.luau"); assert!(m); } @@ -1581,10 +1614,11 @@ mod tests { #[test] fn disk_seed_file_middleware_collides_with_bare_slug() { - // taken_names now uses bare slugs (dedup_keys), so file-format - // dedup works correctly. "mymodule" in taken matches slug "MyModule". + // taken_names stores full filesystem names (lowercased). + // A bare directory name "mymodule" does NOT collide with "mymodule.luau". + // Seed with "mymodule.luau" to test file-file collision. let r = process_new_children_with_disk_seed( - &["mymodule"], + &["mymodule.luau"], &[("MyModule", Middleware::ModuleScript)], ); assert_eq!(r[0].1, "MyModule~1.luau"); @@ -1920,10 +1954,9 @@ mod tests { #[test] fn nightmare_mixed_middleware_dir_then_file() { - // "Shared" as Dir (filename "Shared") then ModuleScript. - // Dir filename "Shared" → taken gets "shared". - // ModuleScript dedup checks "shared" → MATCH → dedup to "Shared~1". - // Then final filename becomes "Shared~1.luau". + // "Shared" as Dir (filename "Shared", dedup_key "Shared") then ModuleScript. + // Dir dedup_key "shared" in taken does NOT collide with "shared.luau". + // Dir and file with same base name occupy different filesystem namespaces. let r = process_new_children_with_disk_seed( &[], &[ @@ -1933,17 +1966,16 @@ mod tests { ); assert_eq!(r[0].1, "Shared"); assert!(!r[0].2); - // File middleware bare slug "Shared" matches dir entry "shared" in taken. - assert_eq!(r[1].1, "Shared~1.luau"); - assert!(r[1].2); + assert_eq!(r[1].1, "Shared.luau"); + assert!(!r[1].2); } #[test] fn nightmare_mixed_middleware_file_then_dir() { // Reverse order: ModuleScript first, Dir second. - // ModuleScript "Shared" → dedup_key "shared" → taken gets "shared" - // Dir "Shared" → slug "Shared" → dedup checks "shared" → MATCH - // (dedup_key-based taken catches this correctly) → "Shared~1" + // ModuleScript "Shared" → dedup_key "shared.luau" in taken. + // Dir "Shared" → full_name "Shared", checks "shared" against taken. + // "shared" != "shared.luau" → no collision. Dir and file coexist. let r = process_new_children_with_disk_seed( &[], &[ @@ -1953,9 +1985,8 @@ mod tests { ); assert_eq!(r[0].1, "Shared.luau"); assert!(!r[0].2); - // Dir slug "Shared" collides with dedup_key "shared" from the ModuleScript - assert_eq!(r[1].1, "Shared~1"); - assert!(r[1].2, "dedup suffix means different from instance name"); + assert_eq!(r[1].1, "Shared"); + assert!(!r[1].2); } #[test] @@ -2393,14 +2424,9 @@ mod tests { assert_eq!(name1.as_ref(), "Hey_Bro.luau"); assert!(meta1); - // On the old-inst path the existing filename is preserved, - // so a second syncback (incremental) should not change the name. - // We can't easily test the full old-inst path here since it - // requires InstanceWithMeta, but we verify the dedup_key is - // stable: calling strip_middleware_extension on the result gives - // the same dedup_key. - let dk_from_filename = strip_middleware_extension(&name1, Middleware::ModuleScript); - assert_eq!(dk_from_filename.to_lowercase(), dk1.to_lowercase()); + // dedup_key is now the full filesystem name (including extension). + // For file middleware, dedup_key == filename. + assert_eq!(dk1.to_lowercase(), name1.as_ref().to_lowercase()); } #[test] @@ -2424,7 +2450,7 @@ mod tests { .unwrap(); assert_eq!(filename.as_ref(), "Hey_Bro.luau"); assert!(needs_meta); - assert_eq!(dedup_key, "Hey_Bro"); + assert_eq!(dedup_key, "Hey_Bro.luau"); } #[test] diff --git a/src/syncback/matching.rs b/src/syncback/matching.rs new file mode 100644 index 00000000..be91e446 --- /dev/null +++ b/src/syncback/matching.rs @@ -0,0 +1,769 @@ +//! 3-pass instance matching algorithm for syncback. +//! +//! Pairs new children (from Roblox file) to old children (from filesystem) +//! using progressively more expensive matching signals: +//! +//! - **Pass 1** (O(n)): unique name matching + ClassName narrowing within +//! same-name groups. +//! - **Pass 2**: Ref property discriminators. If a Ref target is already +//! matched in Pass 1, use it to differentiate candidates. +//! - **Pass 3**: pairwise similarity scoring using precomputed subtree +//! hashes. Greedy best-match-first with DOM child order as tiebreaker. +//! +//! The algorithm is designed for the syncback context where both "new" and +//! "old" sides are WeakDom instances. + +use std::collections::HashMap; + +use blake3::Hash; +use rbx_dom_weak::{ + types::{Ref, Variant}, + ustr, WeakDom, +}; + +/// Result of the matching algorithm. +#[derive(Debug)] +pub struct MatchResult { + /// Pairs of (new_ref, old_ref) that were matched. + pub matched: Vec<(Ref, Ref)>, + /// New children with no match on the old side (to be created). + pub unmatched_new: Vec, + /// Old children with no match on the new side (to be deleted). + pub unmatched_old: Vec, +} + +/// Run the 3-pass matching algorithm. +/// +/// `new_children` and `old_children` are the Refs of children under the same +/// parent in their respective WeakDoms. `new_hashes` and `old_hashes` are +/// optional precomputed subtree hashes (from `hash_tree()`). +pub fn match_children( + new_children: &[Ref], + old_children: &[Ref], + new_dom: &WeakDom, + old_dom: &WeakDom, + new_hashes: Option<&HashMap>, + old_hashes: Option<&HashMap>, +) -> MatchResult { + if new_children.is_empty() && old_children.is_empty() { + return MatchResult { + matched: Vec::new(), + unmatched_new: Vec::new(), + unmatched_old: Vec::new(), + }; + } + + let mut matched: Vec<(Ref, Ref)> = Vec::new(); + let mut remaining_new: Vec = new_children.to_vec(); + let mut remaining_old: Vec = old_children.to_vec(); + + // ---- Pass 1: unique name matching + ClassName narrowing ---- + pass1_name_and_class( + &mut remaining_new, + &mut remaining_old, + new_dom, + old_dom, + &mut matched, + ); + + // Fast path: if all matched, skip Passes 2 and 3. + if remaining_new.is_empty() || remaining_old.is_empty() { + return MatchResult { + matched, + unmatched_new: remaining_new, + unmatched_old: remaining_old, + }; + } + + // ---- Pass 2: Ref property discriminators ---- + // Snapshot the already-matched pairs so we can borrow matched mutably. + let matched_snapshot: Vec<(Ref, Ref)> = matched.clone(); + pass2_ref_discriminators( + &mut remaining_new, + &mut remaining_old, + new_dom, + old_dom, + &matched_snapshot, + &mut matched, + ); + + if remaining_new.is_empty() || remaining_old.is_empty() { + return MatchResult { + matched, + unmatched_new: remaining_new, + unmatched_old: remaining_old, + }; + } + + // ---- Pass 3: similarity scoring ---- + pass3_similarity( + &mut remaining_new, + &mut remaining_old, + new_dom, + old_dom, + new_hashes, + old_hashes, + new_children, + old_children, + &mut matched, + ); + + MatchResult { + matched, + unmatched_new: remaining_new, + unmatched_old: remaining_old, + } +} + +/// Pass 1: Match instances with unique names, then narrow same-name groups by +/// ClassName. +fn pass1_name_and_class( + remaining_new: &mut Vec, + remaining_old: &mut Vec, + new_dom: &WeakDom, + old_dom: &WeakDom, + matched: &mut Vec<(Ref, Ref)>, +) { + // Build name→refs maps for both sides. + let mut new_by_name: HashMap> = HashMap::new(); + for &r in remaining_new.iter() { + if let Some(inst) = new_dom.get_by_ref(r) { + new_by_name + .entry(inst.name.clone()) + .or_default() + .push(r); + } + } + let mut old_by_name: HashMap> = HashMap::new(); + for &r in remaining_old.iter() { + if let Some(inst) = old_dom.get_by_ref(r) { + old_by_name + .entry(inst.name.to_string()) + .or_default() + .push(r); + } + } + + let mut matched_new: Vec = Vec::new(); + let mut matched_old: Vec = Vec::new(); + + // Sort names for deterministic iteration order. + let mut sorted_names: Vec<&String> = new_by_name.keys().collect(); + sorted_names.sort(); + + for name in sorted_names { + let new_refs = &new_by_name[name]; + let Some(old_refs) = old_by_name.get(name.as_str()) else { + continue; + }; + + // Case 1: exactly one on each side → instant match. + if new_refs.len() == 1 && old_refs.len() == 1 { + matched.push((new_refs[0], old_refs[0])); + matched_new.push(new_refs[0]); + matched_old.push(old_refs[0]); + continue; + } + + // Case 2: multiple on at least one side → try ClassName narrowing. + // Build (Name, ClassName) → refs maps within this name group. + let mut new_by_class: HashMap<&str, Vec> = HashMap::new(); + for &r in new_refs { + if let Some(inst) = new_dom.get_by_ref(r) { + new_by_class.entry(&inst.class).or_default().push(r); + } + } + let mut old_by_class: HashMap<&str, Vec> = HashMap::new(); + for &r in old_refs { + if let Some(inst) = old_dom.get_by_ref(r) { + old_by_class.entry(&inst.class).or_default().push(r); + } + } + + // Match (Name, ClassName) pairs that are unique on both sides. + for (class_name, new_class_refs) in &new_by_class { + if let Some(old_class_refs) = old_by_class.get(class_name) { + if new_class_refs.len() == 1 && old_class_refs.len() == 1 { + matched.push((new_class_refs[0], old_class_refs[0])); + matched_new.push(new_class_refs[0]); + matched_old.push(old_class_refs[0]); + } + } + } + } + + // Remove matched refs from remaining lists. + remaining_new.retain(|r| !matched_new.contains(r)); + remaining_old.retain(|r| !matched_old.contains(r)); +} + +/// Pass 2: Use pre-resolved Ref properties to differentiate instances within +/// same-name groups. +fn pass2_ref_discriminators( + remaining_new: &mut Vec, + remaining_old: &mut Vec, + new_dom: &WeakDom, + old_dom: &WeakDom, + already_matched: &[(Ref, Ref)], + matched: &mut Vec<(Ref, Ref)>, +) { + if remaining_new.is_empty() || remaining_old.is_empty() { + return; + } + + // Build bidirectional match maps: new→old and old→new. + let new_to_old: HashMap = already_matched.iter().copied().collect(); + let old_to_new: HashMap = already_matched + .iter() + .map(|&(n, o)| (o, n)) + .collect(); + + // Collect Ref property targets for each remaining instance. + let new_ref_targets: HashMap> = remaining_new + .iter() + .map(|&r| (r, collect_ref_property_targets(new_dom, r))) + .collect(); + let old_ref_targets: HashMap> = remaining_old + .iter() + .map(|&r| (r, collect_ref_property_targets(old_dom, r))) + .collect(); + + // Group remaining by name for matching within groups. + let mut new_by_name: HashMap> = HashMap::new(); + for &r in remaining_new.iter() { + if let Some(inst) = new_dom.get_by_ref(r) { + new_by_name + .entry(inst.name.clone()) + .or_default() + .push(r); + } + } + let mut old_by_name: HashMap> = HashMap::new(); + for &r in remaining_old.iter() { + if let Some(inst) = old_dom.get_by_ref(r) { + old_by_name + .entry(inst.name.to_string()) + .or_default() + .push(r); + } + } + + let mut matched_new: Vec = Vec::new(); + let mut matched_old: Vec = Vec::new(); + + for (name, new_refs) in &new_by_name { + let Some(old_refs) = old_by_name.get(name) else { + continue; + }; + + // For each new instance in this group, compute a "ref signature": + // the set of matched counterparts its Ref targets point to. + for &new_ref in new_refs { + if matched_new.contains(&new_ref) { + continue; + } + let new_targets = &new_ref_targets[&new_ref]; + if new_targets.is_empty() { + continue; + } + // Map new targets through the match map to get "translated" targets. + let translated: Vec = new_targets + .iter() + .filter_map(|t| new_to_old.get(t).copied()) + .collect(); + if translated.is_empty() { + continue; + } + + // Find old candidates that have Ref targets matching the translated set. + let mut candidates: Vec = Vec::new(); + for &old_ref in old_refs { + if matched_old.contains(&old_ref) { + continue; + } + let old_targets = &old_ref_targets[&old_ref]; + let old_translated: Vec = old_targets + .iter() + .filter_map(|t| old_to_new.get(t).copied()) + .collect(); + + // Check if the ref signatures overlap. + if translated.iter().any(|t| old_targets.contains(t)) + || old_translated.iter().any(|t| new_targets.contains(t)) + { + candidates.push(old_ref); + } + } + + if candidates.len() == 1 { + matched.push((new_ref, candidates[0])); + matched_new.push(new_ref); + matched_old.push(candidates[0]); + } + } + } + + remaining_new.retain(|r| !matched_new.contains(r)); + remaining_old.retain(|r| !matched_old.contains(r)); +} + +/// Pass 3: Similarity scoring using subtree hashes and greedy assignment. +#[allow(clippy::too_many_arguments)] +fn pass3_similarity( + remaining_new: &mut Vec, + remaining_old: &mut Vec, + new_dom: &WeakDom, + old_dom: &WeakDom, + new_hashes: Option<&HashMap>, + old_hashes: Option<&HashMap>, + original_new_children: &[Ref], + original_old_children: &[Ref], + matched: &mut Vec<(Ref, Ref)>, +) { + if remaining_new.is_empty() || remaining_old.is_empty() { + return; + } + + // Group remaining by name. + let mut new_by_name: HashMap> = HashMap::new(); + for &r in remaining_new.iter() { + if let Some(inst) = new_dom.get_by_ref(r) { + new_by_name + .entry(inst.name.clone()) + .or_default() + .push(r); + } + } + let mut old_by_name: HashMap> = HashMap::new(); + for &r in remaining_old.iter() { + if let Some(inst) = old_dom.get_by_ref(r) { + old_by_name + .entry(inst.name.to_string()) + .or_default() + .push(r); + } + } + + let mut matched_new: Vec = Vec::new(); + let mut matched_old: Vec = Vec::new(); + + for (name, new_refs) in &new_by_name { + let Some(old_refs) = old_by_name.get(name) else { + continue; + }; + + // Build all candidate pairs with their similarity scores. + let mut pairs: Vec<(Ref, Ref, u32)> = Vec::new(); + for &new_ref in new_refs { + if matched_new.contains(&new_ref) { + continue; + } + for &old_ref in old_refs { + if matched_old.contains(&old_ref) { + continue; + } + let score = compute_similarity( + new_ref, old_ref, new_dom, old_dom, new_hashes, old_hashes, + ); + pairs.push((new_ref, old_ref, score)); + } + } + + // Sort by score descending. Tiebreaker: original DOM child order + // (lower index = earlier in children list = preferred). + pairs.sort_by(|a, b| { + b.2.cmp(&a.2).then_with(|| { + let a_new_idx = original_new_children + .iter() + .position(|&r| r == a.0) + .unwrap_or(usize::MAX); + let b_new_idx = original_new_children + .iter() + .position(|&r| r == b.0) + .unwrap_or(usize::MAX); + a_new_idx.cmp(&b_new_idx).then_with(|| { + let a_old_idx = original_old_children + .iter() + .position(|&r| r == a.1) + .unwrap_or(usize::MAX); + let b_old_idx = original_old_children + .iter() + .position(|&r| r == b.1) + .unwrap_or(usize::MAX); + a_old_idx.cmp(&b_old_idx) + }) + }) + }); + + // Greedy assignment: pick highest-scored pair, remove both, repeat. + for (new_ref, old_ref, _score) in &pairs { + if matched_new.contains(new_ref) || matched_old.contains(old_ref) { + continue; + } + matched.push((*new_ref, *old_ref)); + matched_new.push(*new_ref); + matched_old.push(*old_ref); + } + } + + remaining_new.retain(|r| !matched_new.contains(r)); + remaining_old.retain(|r| !matched_old.contains(r)); +} + +/// Compute a similarity score between two instances. +/// Higher = more similar. Signals checked in priority order: +/// ClassName (100), Tags (50), Attributes (30), Hash equality (1000), +/// Properties overlap (10 per match). +fn compute_similarity( + new_ref: Ref, + old_ref: Ref, + new_dom: &WeakDom, + old_dom: &WeakDom, + new_hashes: Option<&HashMap>, + old_hashes: Option<&HashMap>, +) -> u32 { + let new_inst = match new_dom.get_by_ref(new_ref) { + Some(i) => i, + None => return 0, + }; + let old_inst = match old_dom.get_by_ref(old_ref) { + Some(i) => i, + None => return 0, + }; + + let mut score: u32 = 0; + + // ClassName match is a strong signal. + if new_inst.class == old_inst.class { + score += 100; + } + + // Hash equality: very likely identical content. + if let (Some(nh), Some(oh)) = (new_hashes, old_hashes) { + if let (Some(new_hash), Some(old_hash)) = (nh.get(&new_ref), oh.get(&old_ref)) { + if new_hash == old_hash { + score += 1000; + } + } + } + + // Tags comparison. + if let (Some(Variant::Tags(new_tags)), Some(Variant::Tags(old_tags))) = ( + new_inst.properties.get(&ustr("Tags")), + old_inst.properties.get(&ustr("Tags")), + ) { + if new_tags == old_tags { + score += 50; + } + } + + // Attributes comparison. + if let (Some(Variant::Attributes(new_attrs)), Some(Variant::Attributes(old_attrs))) = ( + new_inst.properties.get(&ustr("Attributes")), + old_inst.properties.get(&ustr("Attributes")), + ) { + if new_attrs == old_attrs { + score += 30; + } + } + + // Children count similarity (cheap structural signal). + let new_child_count = new_inst.children().len(); + let old_child_count = old_inst.children().len(); + if new_child_count == old_child_count { + score += 20; + } + + score +} + +/// Collect all Ref-valued property targets for an instance. +fn collect_ref_property_targets(dom: &WeakDom, inst_ref: Ref) -> Vec { + let inst = match dom.get_by_ref(inst_ref) { + Some(i) => i, + None => return Vec::new(), + }; + let mut targets = Vec::new(); + for value in inst.properties.values() { + if let Variant::Ref(target) = value { + if !target.is_none() { + targets.push(*target); + } + } + } + targets +} + +#[cfg(test)] +mod tests { + use super::*; + use rbx_dom_weak::InstanceBuilder; + + fn build_test_dom() -> (WeakDom, Ref, Ref, Ref) { + let mut dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let root = dom.root_ref(); + let child_a = dom.insert(root, InstanceBuilder::new("Folder").with_name("Alpha")); + let child_b = dom.insert(root, InstanceBuilder::new("Script").with_name("Beta")); + (dom, root, child_a, child_b) + } + + #[test] + fn unique_names_match_instantly() { + let (new_dom, new_root, new_a, new_b) = build_test_dom(); + let (old_dom, old_root, old_a, old_b) = build_test_dom(); + + let new_children: Vec = new_dom + .get_by_ref(new_root) + .unwrap() + .children() + .to_vec(); + let old_children: Vec = old_dom + .get_by_ref(old_root) + .unwrap() + .children() + .to_vec(); + + let result = match_children(&new_children, &old_children, &new_dom, &old_dom, None, None); + + assert_eq!(result.matched.len(), 2); + assert!(result.unmatched_new.is_empty()); + assert!(result.unmatched_old.is_empty()); + + // Alpha matched to Alpha, Beta to Beta + for (new_ref, old_ref) in &result.matched { + let new_name = &new_dom.get_by_ref(*new_ref).unwrap().name; + let old_name = &old_dom.get_by_ref(*old_ref).unwrap().name; + assert_eq!(new_name, old_name); + } + } + + #[test] + fn class_name_narrowing() { + // Two instances named "Handler" but different classes + let mut new_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let new_root = new_dom.root_ref(); + let new_a = new_dom.insert( + new_root, + InstanceBuilder::new("Script").with_name("Handler"), + ); + let new_b = new_dom.insert( + new_root, + InstanceBuilder::new("ModuleScript").with_name("Handler"), + ); + + let mut old_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let old_root = old_dom.root_ref(); + let old_a = old_dom.insert( + old_root, + InstanceBuilder::new("ModuleScript").with_name("Handler"), + ); + let old_b = old_dom.insert( + old_root, + InstanceBuilder::new("Script").with_name("Handler"), + ); + + let new_children: Vec = new_dom + .get_by_ref(new_root) + .unwrap() + .children() + .to_vec(); + let old_children: Vec = old_dom + .get_by_ref(old_root) + .unwrap() + .children() + .to_vec(); + + let result = match_children(&new_children, &old_children, &new_dom, &old_dom, None, None); + + assert_eq!(result.matched.len(), 2); + assert!(result.unmatched_new.is_empty()); + assert!(result.unmatched_old.is_empty()); + + // Verify Script matched to Script, ModuleScript to ModuleScript. + for (new_ref, old_ref) in &result.matched { + let new_class = &new_dom.get_by_ref(*new_ref).unwrap().class; + let old_class = &old_dom.get_by_ref(*old_ref).unwrap().class; + assert_eq!(new_class, old_class); + } + } + + #[test] + fn unmatched_new_and_old() { + let mut new_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let new_root = new_dom.root_ref(); + new_dom.insert( + new_root, + InstanceBuilder::new("Folder").with_name("Alpha"), + ); + new_dom.insert( + new_root, + InstanceBuilder::new("Folder").with_name("NewOnly"), + ); + + let mut old_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let old_root = old_dom.root_ref(); + old_dom.insert( + old_root, + InstanceBuilder::new("Folder").with_name("Alpha"), + ); + old_dom.insert( + old_root, + InstanceBuilder::new("Folder").with_name("OldOnly"), + ); + + let new_children: Vec = new_dom + .get_by_ref(new_root) + .unwrap() + .children() + .to_vec(); + let old_children: Vec = old_dom + .get_by_ref(old_root) + .unwrap() + .children() + .to_vec(); + + let result = match_children(&new_children, &old_children, &new_dom, &old_dom, None, None); + + assert_eq!(result.matched.len(), 1); + assert_eq!(result.unmatched_new.len(), 1); + assert_eq!(result.unmatched_old.len(), 1); + + let unmatched_new_name = &new_dom + .get_by_ref(result.unmatched_new[0]) + .unwrap() + .name; + assert_eq!(unmatched_new_name, "NewOnly"); + + let unmatched_old_name = &old_dom + .get_by_ref(result.unmatched_old[0]) + .unwrap() + .name; + assert_eq!(unmatched_old_name, "OldOnly"); + } + + #[test] + fn duplicate_names_fall_to_pass3() { + // Two Folders named "Data" on each side, same class. + // Pass 1 can't distinguish, Pass 3 uses greedy assignment. + let mut new_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let new_root = new_dom.root_ref(); + new_dom.insert( + new_root, + InstanceBuilder::new("Folder").with_name("Data"), + ); + new_dom.insert( + new_root, + InstanceBuilder::new("Folder").with_name("Data"), + ); + + let mut old_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let old_root = old_dom.root_ref(); + old_dom.insert( + old_root, + InstanceBuilder::new("Folder").with_name("Data"), + ); + old_dom.insert( + old_root, + InstanceBuilder::new("Folder").with_name("Data"), + ); + + let new_children: Vec = new_dom + .get_by_ref(new_root) + .unwrap() + .children() + .to_vec(); + let old_children: Vec = old_dom + .get_by_ref(old_root) + .unwrap() + .children() + .to_vec(); + + let result = match_children(&new_children, &old_children, &new_dom, &old_dom, None, None); + + // Both should be matched (greedy assignment). + assert_eq!(result.matched.len(), 2); + assert!(result.unmatched_new.is_empty()); + assert!(result.unmatched_old.is_empty()); + } + + #[test] + fn empty_children() { + let new_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let old_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + + let result = match_children(&[], &[], &new_dom, &old_dom, None, None); + + assert!(result.matched.is_empty()); + assert!(result.unmatched_new.is_empty()); + assert!(result.unmatched_old.is_empty()); + } + + #[test] + fn all_new_no_old() { + let mut new_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let new_root = new_dom.root_ref(); + new_dom.insert( + new_root, + InstanceBuilder::new("Folder").with_name("A"), + ); + + let old_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + + let new_children: Vec = new_dom + .get_by_ref(new_root) + .unwrap() + .children() + .to_vec(); + + let result = match_children(&new_children, &[], &new_dom, &old_dom, None, None); + + assert!(result.matched.is_empty()); + assert_eq!(result.unmatched_new.len(), 1); + assert!(result.unmatched_old.is_empty()); + } + + #[test] + fn matching_stability() { + // Run the same matching twice; verify identical results. + let mut new_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let new_root = new_dom.root_ref(); + new_dom.insert( + new_root, + InstanceBuilder::new("Folder").with_name("X"), + ); + new_dom.insert( + new_root, + InstanceBuilder::new("Script").with_name("Y"), + ); + + let mut old_dom = WeakDom::new(InstanceBuilder::new("DataModel")); + let old_root = old_dom.root_ref(); + old_dom.insert( + old_root, + InstanceBuilder::new("Folder").with_name("X"), + ); + old_dom.insert( + old_root, + InstanceBuilder::new("Script").with_name("Y"), + ); + + let new_children: Vec = new_dom + .get_by_ref(new_root) + .unwrap() + .children() + .to_vec(); + let old_children: Vec = old_dom + .get_by_ref(old_root) + .unwrap() + .children() + .to_vec(); + + let r1 = match_children(&new_children, &old_children, &new_dom, &old_dom, None, None); + let r2 = match_children(&new_children, &old_children, &new_dom, &old_dom, None, None); + + assert_eq!(r1.matched.len(), r2.matched.len()); + for (a, b) in r1.matched.iter().zip(r2.matched.iter()) { + assert_eq!(a, b); + } + } +} diff --git a/src/syncback/mod.rs b/src/syncback/mod.rs index 69d16805..4838f53a 100644 --- a/src/syncback/mod.rs +++ b/src/syncback/mod.rs @@ -1,6 +1,8 @@ +pub mod dedup_suffix; mod file_names; mod fs_snapshot; mod hash; +pub mod matching; pub mod meta; mod property_filter; mod ref_properties; diff --git a/src/syncback/ref_properties.rs b/src/syncback/ref_properties.rs index f22e892f..dbe2e0a2 100644 --- a/src/syncback/ref_properties.rs +++ b/src/syncback/ref_properties.rs @@ -132,15 +132,14 @@ fn is_path_unique_with_cache( /// the sync tree to be preserved as path-based attributes. pub fn collect_referents(dom: &WeakDom, pre_prune_paths: &HashMap) -> RefLinks { let mut path_links: HashMap> = HashMap::new(); - let mut id_links: HashMap> = HashMap::new(); - let mut targets_needing_id: HashSet = HashSet::new(); + let id_links: HashMap> = HashMap::new(); + let targets_needing_id: HashSet = HashSet::new(); - // Pre-compute duplicate sibling info in O(N) - this makes all subsequent - // path uniqueness checks O(d) instead of O(d × s) where d=depth, s=siblings - let has_duplicate_siblings = compute_refs_with_duplicate_siblings(dom); - - // Cache path uniqueness results to avoid re-checking the same target - let mut path_unique_cache: HashMap = HashMap::new(); + // With the dedup system, ALL filesystem paths are unique (filesystem names + // are unique among siblings by definition). Always use path-based linking. + // ID-based fallback ($id/Rojo_Target_*) is only for instances without + // filesystem backing (inside .rbxm files), which will be handled when the + // syncback flow is restructured to pass filesystem name info. let mut queue = VecDeque::new(); queue.push_back(dom.root_ref()); @@ -157,35 +156,16 @@ pub fn collect_referents(dom: &WeakDom, pre_prune_paths: &HashMap) continue; } - // Check if target exists in current DOM - if let Some(_target) = dom.get_by_ref(*target_ref) { - // Target exists - check if path is unique (with caching) - let is_unique = *path_unique_cache.entry(*target_ref).or_insert_with(|| { - is_path_unique_with_cache(dom, *target_ref, &has_duplicate_siblings) + if dom.get_by_ref(*target_ref).is_some() { + // Target exists in DOM -- always use path-based system. + // With dedup suffixes, paths are always unique. + let target_path = inst_path(dom, *target_ref); + path_links.entry(inst_ref).or_default().push(PathRefLink { + name: *prop_name, + path: target_path, }); - - if is_unique { - // Path is unique - use path-based system - let target_path = inst_path(dom, *target_ref); - path_links.entry(inst_ref).or_default().push(PathRefLink { - name: *prop_name, - path: target_path, - }); - } else { - // Path is NOT unique - fall back to ID-based system - log::debug!( - "Property {}.{} uses ID-based reference because target has duplicate-named siblings", - inst_path(dom, inst_ref), - prop_name - ); - id_links.entry(inst_ref).or_default().push(IdRefLink { - name: *prop_name, - target: *target_ref, - }); - targets_needing_id.insert(*target_ref); - } } else if let Some(external_path) = pre_prune_paths.get(target_ref) { - // Target was pruned - use path (we can't check uniqueness, assume it's fine) + // Target was pruned -- use pre-prune path log::debug!( "Property {}.{} points to pruned instance at '{}', storing as path reference", inst_path(dom, inst_ref), @@ -197,7 +177,6 @@ pub fn collect_referents(dom: &WeakDom, pre_prune_paths: &HashMap) path: external_path.clone(), }); } else { - // Truly dangling reference log::warn!( "Property {}.{} will be `nil` on disk because the referenced instance does not exist", inst_path(dom, inst_ref), diff --git a/src/web/api.rs b/src/web/api.rs index e0a63678..44da31d4 100644 --- a/src/web/api.rs +++ b/src/web/api.rs @@ -609,7 +609,7 @@ impl ApiService { .filter_map(|(guid, added)| { let parent_ref = added.parent?; let parent_path = if tree.get_instance(parent_ref).is_some() { - crate::ref_target_path(tree.inner(), parent_ref) + crate::ref_target_path_from_tree(&tree, parent_ref) } else { return None; }; @@ -1197,14 +1197,10 @@ impl ApiService { new_dir }; - // Filter duplicate children - let inst_path = format!("{}", existing_path.display()); - let unique_children = - self.filter_duplicate_children(&added.children, &inst_path, stats); - // Process children using normal syncback path // This will use detect_existing_script_format to check existing files - self.process_children_incremental(&unique_children, &children_dir, stats)?; + let children_refs: Vec<_> = added.children.iter().collect(); + self.process_children_incremental(&children_refs, &children_dir, stats)?; } } @@ -1234,11 +1230,8 @@ impl ApiService { // Handle children if !added.children.is_empty() { - let inst_path = format!("{}", existing_path.display()); - let unique_children = - self.filter_duplicate_children(&added.children, &inst_path, stats); - - self.process_children_incremental(&unique_children, existing_path, stats)?; + let children_refs: Vec<_> = added.children.iter().collect(); + self.process_children_incremental(&children_refs, existing_path, stats)?; } } else { // It's a standalone file (e.g., .model.json5) @@ -1654,47 +1647,6 @@ impl ApiService { true } - /// Check for duplicate names among children and return the set of unique children - /// that should be synced. Records skipped instances via stats tracker. - fn filter_duplicate_children<'a>( - &self, - children: &'a [crate::web::interface::AddedInstance], - parent_path: &str, - stats: &crate::syncback::SyncbackStats, - ) -> Vec<&'a crate::web::interface::AddedInstance> { - use std::collections::HashMap; - - // Count occurrences of each name - let mut name_counts: HashMap<&str, usize> = HashMap::new(); - for child in children { - *name_counts.entry(&child.name).or_insert(0) += 1; - } - - // Find duplicates - let duplicates: std::collections::HashSet<&str> = name_counts - .iter() - .filter(|(_, &count)| count > 1) - .map(|(&name, _)| name) - .collect(); - - // Record skipped duplicates in stats - if !duplicates.is_empty() { - let mut skipped_count = 0; - for child in children { - if duplicates.contains(child.name.as_str()) { - skipped_count += 1; - } - } - let duplicate_list: Vec<&str> = duplicates.iter().copied().collect(); - stats.record_duplicate_names_batch(parent_path, &duplicate_list, skipped_count); - } - - // Return only non-duplicate children - children - .iter() - .filter(|child| !duplicates.contains(child.name.as_str())) - .collect() - } /// Detects what file format currently exists on disk for a given instance. /// This is used to preserve the existing format during partial updates. @@ -1826,12 +1778,8 @@ impl ApiService { None }; - // Build path string for stats - let inst_path = format!("{}/{}", parent_dir.display(), added.name); - - // Filter out children with duplicate names (cannot reliably sync) - let unique_children = self.filter_duplicate_children(&added.children, &inst_path, stats); - let has_children = !unique_children.is_empty(); + let children_refs: Vec<_> = added.children.iter().collect(); + let has_children = !children_refs.is_empty(); // Determine the appropriate middleware/file format based on class name. // This matches the logic in src/syncback/mod.rs::get_best_middleware. @@ -1887,7 +1835,7 @@ impl ApiService { })?; self.write_script_meta_json_if_needed(&dir_path, added, meta_name_field)?; log::info!("Syncback: Updated ModuleScript at {}", init_path.display()); - self.process_children_incremental(&unique_children, &dir_path, stats)?; + self.process_children_incremental(&children_refs, &dir_path, stats)?; } else { let file_path = parent_dir.join(format!("{}.luau", encoded_name)); fs::write(&file_path, source.as_bytes()).with_context(|| { @@ -1937,7 +1885,7 @@ impl ApiService { })?; self.write_script_meta_json_if_needed(&dir_path, added, meta_name_field)?; log::info!("Syncback: Updated Script at {}", init_path.display()); - self.process_children_incremental(&unique_children, &dir_path, stats)?; + self.process_children_incremental(&children_refs, &dir_path, stats)?; } else { let file_path = parent_dir.join(format!("{}.{}.luau", encoded_name, script_suffix)); @@ -1987,7 +1935,7 @@ impl ApiService { })?; self.write_script_meta_json_if_needed(&dir_path, added, meta_name_field)?; log::info!("Syncback: Updated LocalScript at {}", init_path.display()); - self.process_children_incremental(&unique_children, &dir_path, stats)?; + self.process_children_incremental(&children_refs, &dir_path, stats)?; } else { let file_path = parent_dir.join(format!("{}.local.luau", encoded_name)); fs::write(&file_path, source.as_bytes()).with_context(|| { @@ -2033,7 +1981,7 @@ impl ApiService { ); // Recursively process children - self.process_children_incremental(&unique_children, &dir_path, stats)?; + self.process_children_incremental(&children_refs, &dir_path, stats)?; } // StringValue: .txt file if no children, directory with init.meta.json5 if has children @@ -2049,7 +1997,7 @@ impl ApiService { "Syncback: Created StringValue directory at {}", dir_path.display() ); - self.process_children_incremental(&unique_children, &dir_path, stats)?; + self.process_children_incremental(&children_refs, &dir_path, stats)?; } else { let value = added .properties @@ -2103,7 +2051,7 @@ impl ApiService { "Syncback: Created LocalizationTable directory at {}", dir_path.display() ); - self.process_children_incremental(&unique_children, &dir_path, stats)?; + self.process_children_incremental(&children_refs, &dir_path, stats)?; } else { let file_path = parent_dir.join(format!("{}.csv", encoded_name)); fs::write(&file_path, content.as_bytes()).with_context(|| { @@ -2167,7 +2115,7 @@ impl ApiService { ); // Recursively process children - self.process_children_incremental(&unique_children, &dir_path, stats)?; + self.process_children_incremental(&children_refs, &dir_path, stats)?; } else { let content = self.serialize_instance_to_model_json(added, meta_name_field)?; // Use the detected file path if available (preserves .model.json @@ -2693,7 +2641,7 @@ impl ApiService { remove_attributes.push(attr_name); } else if tree.get_instance(*target_ref).is_some() { // Valid target: compute path and add as Rojo_Ref_* attribute - let path = crate::ref_target_path(tree.inner(), *target_ref); + let path = crate::ref_target_path_from_tree(&tree, *target_ref); // Warn if the path is ambiguous (duplicate-named siblings // at any ancestor level). The ref will still be written, @@ -5460,114 +5408,6 @@ mod tests { } } - // ========================================================================= - // Duplicate Path Detection - Integration Scenarios - // ========================================================================= - mod duplicate_path_integration_tests { - - #[test] - fn test_syncback_skips_duplicate_children() { - // When syncing back children, duplicates should be skipped - // This test verifies the filter_duplicate_children logic - - let children_names = ["ChildA", "ChildB", "ChildA", "ChildC"]; - let unique: Vec<_> = children_names - .iter() - .filter(|&&name| children_names.iter().filter(|&&n| n == name).count() == 1) - .collect(); - - assert_eq!(unique.len(), 2, "ChildB and ChildC should be kept"); - assert!(unique.contains(&&"ChildB")); - assert!(unique.contains(&&"ChildC")); - } - - #[test] - fn test_recursive_duplicate_handling() { - // Test that duplicates at any level are handled - // - // Parent/ - // ├─ Child1/ - // │ ├─ GrandChild (duplicate) - // │ └─ GrandChild (duplicate) - // └─ Child2/ - // └─ GrandChild (unique within its parent) - - let child1_grandchildren = ["GrandChild", "GrandChild"]; - let child2_grandchildren = ["GrandChild"]; - - // Child1's grandchildren have duplicates - let child1_filtered: Vec<_> = child1_grandchildren - .iter() - .filter(|&&name| child1_grandchildren.iter().filter(|&&n| n == name).count() == 1) - .collect(); - assert_eq!( - child1_filtered.len(), - 0, - "Child1's duplicate grandchildren filtered" - ); - - // Child2's grandchild is unique - let child2_filtered: Vec<_> = child2_grandchildren - .iter() - .filter(|&&name| child2_grandchildren.iter().filter(|&&n| n == name).count() == 1) - .collect(); - assert_eq!(child2_filtered.len(), 1, "Child2's unique grandchild kept"); - } - - #[test] - fn test_duplicate_log_message_format() { - // Verify the expected log message format for skipped duplicates - let class_name = "ModuleScript"; - let full_name = "game.ReplicatedStorage.DuplicateScript"; - - let expected_msg = format!( - "Skipped instance '{}' ({}) - path contains duplicate-named siblings (cannot reliably sync)", - full_name, class_name - ); - - assert!(expected_msg.contains(full_name)); - assert!(expected_msg.contains(class_name)); - assert!(expected_msg.contains("duplicate-named siblings")); - } - - #[test] - fn test_parent_path_validation() { - // When pulling an instance, we must verify the parent path is unique - // This simulates the is_tree_path_unique check - - // Scenario: Trying to add a script under a folder that has duplicate siblings - let parent_path_levels = [ - ("Root", vec!["FolderA", "FolderA"]), // Duplicate at this level! - ]; - - let path_unique = parent_path_levels - .iter() - .all(|(target, siblings)| siblings.iter().filter(|&&s| s == *target).count() == 1); - - assert!( - !path_unique, - "Parent path with duplicates should fail validation" - ); - } - - #[test] - fn test_valid_parent_path() { - // Valid parent path with no duplicates at any level - let parent_path_levels = [ - ( - "ReplicatedStorage", - vec!["Workspace", "ReplicatedStorage", "ServerStorage"], - ), - ("Shared", vec!["Shared", "Client", "Server"]), - ]; - - let path_unique = parent_path_levels - .iter() - .all(|(target, siblings)| siblings.iter().filter(|&&s| s == *target).count() == 1); - - assert!(path_unique, "Valid parent path should pass validation"); - } - } // Tests for syncback_removed_instance mod syncback_removed_tests { diff --git a/tests/tests/syncback_roundtrip.rs b/tests/tests/syncback_roundtrip.rs index 31d85893..3075438b 100644 --- a/tests/tests/syncback_roundtrip.rs +++ b/tests/tests/syncback_roundtrip.rs @@ -89,6 +89,10 @@ fn assert_rbx_equal(file_a: &Path, file_b: &Path, test_name: &str) { } /// Recursively compare children of two instances. +/// +/// Uses sorted Vec-based comparison (not HashMap) so that duplicate-named +/// children are correctly handled. HashMap would silently overwrite entries +/// sharing a name, making the test blind to lost duplicates. fn compare_children( dom_a: &WeakDom, ref_a: rbx_dom_weak::types::Ref, @@ -102,73 +106,63 @@ fn compare_children( let children_a = inst_a.children(); let children_b = inst_b.children(); - // Build maps of children by name for comparison - let mut map_a: std::collections::HashMap<&str, &rbx_dom_weak::Instance> = - std::collections::HashMap::new(); - let mut map_b: std::collections::HashMap<&str, &rbx_dom_weak::Instance> = - std::collections::HashMap::new(); - - for &child_ref in children_a { - if let Some(child) = dom_a.get_by_ref(child_ref) { - map_a.insert(&child.name, child); - } - } - - for &child_ref in children_b { - if let Some(child) = dom_b.get_by_ref(child_ref) { - map_b.insert(&child.name, child); - } - } - - // Check that both have the same children - let names_a: std::collections::HashSet<&str> = map_a.keys().copied().collect(); - let names_b: std::collections::HashSet<&str> = map_b.keys().copied().collect(); - - let only_in_a: Vec<_> = names_a.difference(&names_b).collect(); - let only_in_b: Vec<_> = names_b.difference(&names_a).collect(); - - if !only_in_a.is_empty() || !only_in_b.is_empty() { + if children_a.len() != children_b.len() { + let names_a: Vec<&str> = children_a + .iter() + .filter_map(|r| dom_a.get_by_ref(*r).map(|i| i.name.as_str())) + .collect(); + let names_b: Vec<&str> = children_b + .iter() + .filter_map(|r| dom_b.get_by_ref(*r).map(|i| i.name.as_str())) + .collect(); panic!( - "[{}] Children differ under '{}':\n Only in original: {:?}\n Only in roundtrip: {:?}", - test_name, inst_a.name, only_in_a, only_in_b + "[{}] Child count mismatch under '{}': original has {} children {:?}, \ + roundtrip has {} children {:?}", + test_name, + inst_a.name, + children_a.len(), + names_a, + children_b.len(), + names_b, ); } - // Compare each child - for name in names_a { - let child_a = map_a[name]; - let child_b = map_b[name]; + let mut pairs_a: Vec<(&str, rbx_dom_weak::types::Ref)> = children_a + .iter() + .filter_map(|&r| dom_a.get_by_ref(r).map(|i| (i.name.as_str(), r))) + .collect(); + let mut pairs_b: Vec<(&str, rbx_dom_weak::types::Ref)> = children_b + .iter() + .filter_map(|&r| dom_b.get_by_ref(r).map(|i| (i.name.as_str(), r))) + .collect(); - // Check class name matches - if child_a.class != child_b.class { + pairs_a.sort_by_key(|(name, _)| *name); + pairs_b.sort_by_key(|(name, _)| *name); + + for (i, ((name_a, ref_child_a), (name_b, ref_child_b))) in + pairs_a.iter().zip(pairs_b.iter()).enumerate() + { + if name_a != name_b { panic!( - "[{}] Class mismatch for '{}': original={}, roundtrip={}", - test_name, name, child_a.class, child_b.class + "[{}] Child name mismatch under '{}' at sorted index {}: \ + original='{}', roundtrip='{}'", + test_name, inst_a.name, i, name_a, name_b, + ); + } + + let child_a = dom_a.get_by_ref(*ref_child_a).unwrap(); + let child_b = dom_b.get_by_ref(*ref_child_b).unwrap(); + + if child_a.class != child_b.class { + panic!( + "[{}] Class mismatch for '{}' (sorted index {}): original={}, roundtrip={}", + test_name, name_a, i, child_a.class, child_b.class ); } - // Compare important properties (Source for scripts, Value for value types) compare_properties(child_a, child_b, test_name); - // Recurse into children - let child_ref_a = dom_a - .get_by_ref(ref_a) - .unwrap() - .children() - .iter() - .find(|&&r| dom_a.get_by_ref(r).map(|i| i.name.as_str()) == Some(name)) - .copied() - .unwrap(); - let child_ref_b = dom_b - .get_by_ref(ref_b) - .unwrap() - .children() - .iter() - .find(|&&r| dom_b.get_by_ref(r).map(|i| i.name.as_str()) == Some(name)) - .copied() - .unwrap(); - - compare_children(dom_a, child_ref_a, dom_b, child_ref_b, test_name); + compare_children(dom_a, *ref_child_a, dom_b, *ref_child_b, test_name); } } @@ -221,6 +215,19 @@ fn compare_properties( ); } } + + // Check Tags if present + if let (Some(tags_a), Some(tags_b)) = ( + inst_a.properties.get(&ustr("Tags")), + inst_b.properties.get(&ustr("Tags")), + ) { + if tags_a != tags_b { + panic!( + "[{}] Tags differ for '{}' ({}):\nOriginal: {:?}\nRoundtrip: {:?}", + test_name, inst_a.name, inst_a.class, tags_a, tags_b + ); + } + } } // =============================================================================