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 + ); + } + } } // =============================================================================