diff --git a/CHANGELOG.md b/CHANGELOG.md index 02f2c262..65f6ed8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,9 @@ Making a new release? Simply add the new header with the version and date undern * Added a "Forget" option to the sync reminder notification to avoid being reminded for that place in the future ([#1215]) * Improves relative path calculation for sourcemap generation to avoid issues with Windows UNC paths. ([#1217]) * Add support for gitignore-style negation in globIgnorePaths ([#1256]) +* Fixed the sync fallback scrambling sibling order; replacements are now re-parented ancestors-first and in their original child order. ([#1265]) +* Instances that share a name and class are now robustly matched on resync by comparing their properties, instead of relying on child order alone. ([#1266]) +* Rojo now reports a clear error instead of panicking in several cases, including when the `serve` port is already in use, when a synced file is read-only or locked, when the filesystem watcher can't be created, and when the working directory is inaccessible. ([#1267]) [#1176]: https://github.com/rojo-rbx/rojo/pull/1176 [#1179]: https://github.com/rojo-rbx/rojo/pull/1179 @@ -50,6 +53,9 @@ Making a new release? Simply add the new header with the version and date undern [#1215]: https://github.com/rojo-rbx/rojo/pull/1215 [#1217]: https://github.com/rojo-rbx/rojo/pull/1217 [#1256]: https://github.com/rojo-rbx/rojo/pull/1256 +[#1265]: https://github.com/rojo-rbx/rojo/pull/1265 +[#1266]: https://github.com/rojo-rbx/rojo/pull/1266 +[#1267]: https://github.com/rojo-rbx/rojo/pull/1267 ## [7.7.0-rc.1] (November 27th, 2025) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0063e448..bf11ae72 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -13,12 +13,27 @@ Code contributions are welcome for features and bugs that have been reported in You'll want these tools to work on Rojo: -* Latest stable Rust compiler +* Rust 1.88 or newer * Rustfmt and Clippy are used for code formatting and linting. -* Latest stable [Rojo](https://github.com/rojo-rbx/rojo) * [Rokit](https://github.com/rojo-rbx/rokit) * [Luau Language Server](https://github.com/JohnnyMorganz/luau-lsp) (Only needed if working on the Studio plugin.) +Rokit installs the pinned Rojo, Selene, StyLua, Lune, and run-in-roblox versions listed in [`rokit.toml`](rokit.toml): + +```bash +rokit install +``` + +Before opening a pull request, run the relevant checks: + +```bash +cargo test +cargo fmt -- --check +cargo clippy +stylua --check plugin/src +selene plugin/src +``` + When working on the Studio plugin, we recommend using this command to automatically rebuild the plugin when you save a change: *(Make sure you've enabled the Studio setting to reload plugins on file change!)* @@ -29,7 +44,7 @@ bash scripts/watch-build-plugin.sh You can also run the plugin's unit tests with the following: -*(Make sure you have `run-in-roblox` installed first!)* +*(If you are not using Rokit, make sure you have `run-in-roblox` installed first!)* ```bash bash scripts/unit-test-plugin.sh @@ -49,27 +64,27 @@ Please file issues and we'll try to help figure out what the best way forward is ## Local Development Gotchas -If your build fails with "Error: failed to open file `D:\code\rojo\plugin\modules\roact\src`" you need to update your Git submodules. +If your build fails with an error about a missing path under `plugin/Packages`, such as `plugin/Packages/Roact`, you need to update your Git submodules. Run the command and try building again: `git submodule update --init --recursive`. ## Pushing a Rojo Release -The Rojo release process is pretty manual right now. If you need to do it, here's how: +The Rojo release process is driven by the GitHub Actions release workflow. If you need to do it, here's how: 1. Bump server version in [`Cargo.toml`](Cargo.toml) -2. Bump plugin version in [`plugin/src/Config.lua`](plugin/src/Config.lua) -3. Run `cargo test` to update `Cargo.lock` and run tests +2. Bump plugin version in [`plugin/Version.txt`](plugin/Version.txt) + * The build checks that the Cargo and plugin versions match. +3. Run `cargo test` to update `Cargo.lock` after the version bump and run tests 4. Update [`CHANGELOG.md`](CHANGELOG.md) 5. Commit! * `git add . && git commit -m "Release vX.Y.Z"` 6. Tag the commit * `git tag vX.Y.Z` -7. Publish the CLI - * `cargo publish` -8. Publish the Plugin - * `cargo run -- upload plugin --asset_id 6415005344` -9. Push commits and tags +7. Push commits and tags * `git push && git push --tags` +8. Wait for the GitHub Actions release workflow to create the draft release and upload CLI/plugin artifacts +9. Publish the CLI crate + * `cargo publish` 10. Copy GitHub release content from previous release * Update the leading text with a summary about the release * Paste the changelog notes (as-is!) from [`CHANGELOG.md`](CHANGELOG.md) - * Write a small summary of each major feature \ No newline at end of file + * Write a small summary of each major feature diff --git a/README.md b/README.md index c150ecb7..f3d1aeeb 100644 --- a/README.md +++ b/README.md @@ -25,12 +25,11 @@ Rojo enables: * Versioning your game, library, or plugin using Git or another VCS * Streaming `rbxmx` and `rbxm` models into your game in real time * Packaging and deploying your project to Roblox.com from the command line +* Pulling Instances from Roblox place and model files back into an existing Rojo project with `rojo syncback` -In the future, Rojo will be able to: +Rojo also has an optional two-way sync setting in the Studio plugin for syncing supported Studio edits back to the filesystem. -* Sync instances from Roblox Studio to the filesystem -* Automatically convert your existing game to work with Rojo -* Import custom instances like MoonScript code +Some workflows, like fully automatic conversion of every existing game into a Rojo project, are still limited and may require manual project configuration. ## [Documentation](https://rojo.space/docs) Documentation is hosted in the [rojo.space repository](https://github.com/rojo-rbx/rojo.space). diff --git a/crates/memofs/CHANGELOG.md b/crates/memofs/CHANGELOG.md index c35b5399..ec0b71f0 100644 --- a/crates/memofs/CHANGELOG.md +++ b/crates/memofs/CHANGELOG.md @@ -2,6 +2,9 @@ ## Unreleased Changes * Added `Vfs::canonicalize`. [#1201] +* **Breaking:** `StdBackend::new` and `Vfs::new_default` now return `io::Result`, so a failure to create the filesystem watcher is reported as an error instead of panicking. The `Default` implementation for `StdBackend` has been removed as a result. [#1267] + +[#1267]: https://github.com/rojo-rbx/rojo/pull/1267 ## 0.3.1 (2025-11-27) * Added `Vfs::exists`. [#1169] diff --git a/crates/memofs/src/lib.rs b/crates/memofs/src/lib.rs index 41d21100..11acf810 100644 --- a/crates/memofs/src/lib.rs +++ b/crates/memofs/src/lib.rs @@ -255,8 +255,11 @@ pub struct Vfs { impl Vfs { /// Creates a new `Vfs` with the default backend, `StdBackend`. - pub fn new_default() -> Self { - Self::new(StdBackend::new()) + /// + /// Returns an error if the filesystem watcher could not be initialized, + /// which can happen in restricted or sandboxed environments. + pub fn new_default() -> io::Result { + Ok(Self::new(StdBackend::new()?)) } /// Creates a new `Vfs` with the given backend. @@ -639,7 +642,7 @@ mod test { let file_path = dir.path().join("file.txt"); fs_err::write(&file_path, contents.to_string()).unwrap(); - let vfs = Vfs::new(StdBackend::new()); + let vfs = Vfs::new(StdBackend::new().unwrap()); let canonicalized = vfs.canonicalize(&file_path).unwrap(); assert_eq!(canonicalized, file_path.canonicalize().unwrap()); assert_eq!( @@ -653,7 +656,7 @@ mod test { let dir = tempfile::tempdir().unwrap(); let file_path = dir.path().join("test"); - let vfs = Vfs::new(StdBackend::new()); + let vfs = Vfs::new(StdBackend::new().unwrap()); let err = vfs.canonicalize(&file_path).unwrap_err(); assert_eq!(err.kind(), io::ErrorKind::NotFound); } diff --git a/crates/memofs/src/std_backend.rs b/crates/memofs/src/std_backend.rs index a25fa39d..d684bde5 100644 --- a/crates/memofs/src/std_backend.rs +++ b/crates/memofs/src/std_backend.rs @@ -17,9 +17,9 @@ pub struct StdBackend { } impl StdBackend { - pub fn new() -> StdBackend { + pub fn new() -> io::Result { let (notify_tx, notify_rx) = mpsc::channel(); - let watcher = watcher(notify_tx, Duration::from_millis(50)).unwrap(); + let watcher = watcher(notify_tx, Duration::from_millis(50)).map_err(io::Error::other)?; let (tx, rx) = crossbeam_channel::unbounded(); @@ -46,11 +46,11 @@ impl StdBackend { Result::<(), crossbeam_channel::SendError>::Ok(()) }); - Self { + Ok(Self { watcher, watcher_receiver: rx, watches: HashSet::new(), - } + }) } } @@ -134,9 +134,3 @@ impl VfsBackend for StdBackend { self.watcher.unwatch(path).map_err(io::Error::other) } } - -impl Default for StdBackend { - fn default() -> Self { - Self::new() - } -} diff --git a/plugin/http/init.lua b/plugin/http/init.lua index 106ebc45..3d05c6d3 100644 --- a/plugin/http/init.lua +++ b/plugin/http/init.lua @@ -14,6 +14,13 @@ local Http = {} Http.Error = HttpError Http.Response = HttpResponse +-- Monkey patch msgpack.UInt64.new to lossily convert the low and high bits of the integer +-- to a native Luau number. We should change the upstream decoder to emit a native +-- integer, once those are live. +function msgpack.UInt64.new(mostSignificantPart: number, leastSignificantPart: number): number + return (mostSignificantPart % 2 ^ 32) * 2 ^ 32 + (leastSignificantPart % 2 ^ 32) +end + local function performRequest(requestParams) local requestId = lastRequestId + 1 lastRequestId = requestId diff --git a/plugin/run-tests.server.lua b/plugin/run-tests.server.lua index 0ab3cc05..aa5daa9f 100644 --- a/plugin/run-tests.server.lua +++ b/plugin/run-tests.server.lua @@ -1,8 +1,8 @@ local ReplicatedStorage = game:GetService("ReplicatedStorage") -local TestEZ = require(ReplicatedStorage.Packages:WaitForChild("TestEZ", 10)) +local TestEZ = require(ReplicatedStorage:WaitForChild("Packages", 10):WaitForChild("TestEZ", 10)) -local Rojo = ReplicatedStorage.Rojo +local Rojo = ReplicatedStorage:WaitForChild("Rojo", 10) local Settings = require(Rojo.Plugin.Settings) Settings:set("logLevel", "Trace") diff --git a/plugin/src/Reconciler/countMatchingProperties.lua b/plugin/src/Reconciler/countMatchingProperties.lua new file mode 100644 index 00000000..0ddfdcc3 --- /dev/null +++ b/plugin/src/Reconciler/countMatchingProperties.lua @@ -0,0 +1,44 @@ +--[[ + Counts how many of a virtual instance's properties match the live values on a + candidate Roblox instance. `hydrate` uses this to break ties when several + existing children share the same Name and ClassName. + + This mirrors the read -> decode -> compare flow that `diff` uses, reusing the + same `getProperty`, `decodeValue`, and `trueEquals` helpers. +]] + +local getProperty = require(script.Parent.getProperty) +local decodeValue = require(script.Parent.decodeValue) +local trueEquals = require(script.Parent.trueEquals) + +local function countMatchingProperties(instance, virtualInstance, instanceMap) + local score = 0 + + for propertyName, virtualValue in virtualInstance.Properties do + -- Skip refs. During hydration the instanceMap is still being built + -- top-down, so a ref may point at an instance we haven't hydrated yet + -- and therefore can't decode reliably. Refs are also a poor + -- disambiguator between same-named siblings. + if next(virtualValue) == "Ref" then + continue + end + + local getSuccess, existingValue = getProperty(instance, propertyName) + if not getSuccess then + continue + end + + local decodeSuccess, decodedValue = decodeValue(virtualValue, instanceMap) + if not decodeSuccess then + continue + end + + if trueEquals(existingValue, decodedValue) then + score += 1 + end + end + + return score +end + +return countMatchingProperties diff --git a/plugin/src/Reconciler/countMatchingProperties.spec.lua b/plugin/src/Reconciler/countMatchingProperties.spec.lua new file mode 100644 index 00000000..3d6bb18d --- /dev/null +++ b/plugin/src/Reconciler/countMatchingProperties.spec.lua @@ -0,0 +1,91 @@ +return function() + local countMatchingProperties = require(script.Parent.countMatchingProperties) + + local InstanceMap = require(script.Parent.Parent.InstanceMap) + + it("counts properties whose values match the instance", function() + local instance = Instance.new("StringValue") + instance.Value = "hello" + + local virtualInstance = { + ClassName = "StringValue", + Name = "Value", + Properties = { + Value = { String = "hello" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(1) + end) + + it("does not count properties whose values differ", function() + local instance = Instance.new("StringValue") + instance.Value = "hello" + + local virtualInstance = { + ClassName = "StringValue", + Name = "Value", + Properties = { + Value = { String = "different" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(0) + end) + + it("counts multiple matching properties independently", function() + local instance = Instance.new("Part") + instance.Anchored = true + instance.CanCollide = false + + local virtualInstance = { + ClassName = "Part", + Name = "Part", + Properties = { + Anchored = { Bool = true }, + CanCollide = { Bool = false }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(2) + + -- Flip one so only a single property matches. + instance.CanCollide = true + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(1) + end) + + it("skips unknown properties without counting or erroring", function() + local instance = Instance.new("Folder") + + local virtualInstance = { + ClassName = "Folder", + Name = "Folder", + Properties = { + FAKE_PROPERTY = { String = "nope" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(0) + end) + + it("skips Ref properties without counting or erroring", function() + local instance = Instance.new("ObjectValue") + + local virtualInstance = { + ClassName = "ObjectValue", + Name = "ObjectValue", + Properties = { + -- A ref must be skipped rather than decoded: during hydration + -- the target may not be in the map yet. + Value = { Ref = "00000000000000000000000000000000" }, + }, + Children = {}, + } + + expect(countMatchingProperties(instance, virtualInstance, InstanceMap.new())).to.equal(0) + end) +end diff --git a/plugin/src/Reconciler/diff.lua b/plugin/src/Reconciler/diff.lua index c8da0939..73d41236 100644 --- a/plugin/src/Reconciler/diff.lua +++ b/plugin/src/Reconciler/diff.lua @@ -10,104 +10,12 @@ local invariant = require(script.Parent.Parent.invariant) local getProperty = require(script.Parent.getProperty) local Error = require(script.Parent.Error) local decodeValue = require(script.Parent.decodeValue) +local trueEquals = require(script.Parent.trueEquals) local function isEmpty(table) return next(table) == nil end -local function fuzzyEq(a: number, b: number, epsilon: number): boolean - return math.abs(a - b) < epsilon -end - -local function trueEquals(a, b): boolean - -- Exit early for simple equality values - if a == b then - return true - end - - -- Treat nil and { Ref = "000...0" } as equal - if - (a == nil and type(b) == "table" and b.Ref == "00000000000000000000000000000000") - or (b == nil and type(a) == "table" and a.Ref == "00000000000000000000000000000000") - then - return true - end - - local typeA, typeB = typeof(a), typeof(b) - - -- For tables, try recursive deep equality - if typeA == "table" and typeB == "table" then - local checkedKeys = {} - for key, value in a do - checkedKeys[key] = true - if not trueEquals(value, b[key]) then - return false - end - end - for key, value in b do - if checkedKeys[key] then - continue - end - if not trueEquals(value, a[key]) then - return false - end - end - return true - - -- For NaN, check if both values are not equal to themselves - elseif a ~= a and b ~= b then - return true - - -- For numbers, compare with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "number" and typeB == "number" then - return fuzzyEq(a, b, 0.0001) - - -- For EnumItem->number, compare the EnumItem's value - elseif typeA == "number" and typeB == "EnumItem" then - return a == b.Value - elseif typeA == "EnumItem" and typeB == "number" then - return a.Value == b - - -- For Color3s, compare to RGB ints to avoid floating point inequality - elseif typeA == "Color3" and typeB == "Color3" then - local aR, aG, aB = math.floor(a.R * 255), math.floor(a.G * 255), math.floor(a.B * 255) - local bR, bG, bB = math.floor(b.R * 255), math.floor(b.G * 255), math.floor(b.B * 255) - return aR == bR and aG == bG and aB == bB - - -- For CFrames, compare to components with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "CFrame" and typeB == "CFrame" then - local aComponents, bComponents = { a:GetComponents() }, { b:GetComponents() } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - - -- For Vector3s, compare to components with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "Vector3" and typeB == "Vector3" then - local aComponents, bComponents = { a.X, a.Y, a.Z }, { b.X, b.Y, b.Z } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - - -- For Vector2s, compare to components with epsilon of 0.0001 to avoid floating point inequality - elseif typeA == "Vector2" and typeB == "Vector2" then - local aComponents, bComponents = { a.X, a.Y }, { b.X, b.Y } - for i, aComponent in aComponents do - if not fuzzyEq(aComponent, bComponents[i], 0.0001) then - return false - end - end - return true - end - - return false -end - local function shouldDeleteUnknownInstances(virtualInstance) if virtualInstance.Metadata ~= nil then return not virtualInstance.Metadata.ignoreUnknownInstances diff --git a/plugin/src/Reconciler/hydrate.lua b/plugin/src/Reconciler/hydrate.lua index d7798160..fdd6c4a2 100644 --- a/plugin/src/Reconciler/hydrate.lua +++ b/plugin/src/Reconciler/hydrate.lua @@ -3,9 +3,22 @@ concrete instances and assigning them IDs. ]] -local invariant = require(script.Parent.Parent.invariant) +local Packages = script.Parent.Parent.Parent.Packages +local Log = require(Packages.Log) -local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) +local invariant = require(script.Parent.Parent.invariant) +local countMatchingProperties = require(script.Parent.countMatchingProperties) + +-- When several existing children share a Name and ClassName we disambiguate +-- them by scoring how well each one's properties match the virtual instance. +-- That scoring is far more expensive than a Name/ClassName check, so we only do +-- it for reasonably-sized groups. Larger groups (e.g. a folder with thousands of +-- identically-named parts) fall back to the original order-based matching, which +-- bounds the added work to roughly MAX_CANDIDATES_TO_SCORE^2 property reads per +-- group regardless of how large the group is. +local MAX_CANDIDATES_TO_SCORE = 32 + +local function hydrateInner(stats, instanceMap, virtualInstances, rootId, rootInstance) local virtualInstance = virtualInstances[rootId] if virtualInstance == nil then @@ -13,38 +26,163 @@ local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) end instanceMap:insert(rootId, rootInstance) + stats.hydrated += 1 local existingChildren = rootInstance:GetChildren() - -- 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 + -- Group existing children by Name then ClassName so each virtual child can + -- find its candidate matches without scanning every sibling. This is what + -- keeps hydration fast for parents with thousands of children. Nesting the + -- two tables (rather than a combined key) keeps the Name and ClassName checks + -- exact, with no way for one to bleed into the other. + local buckets = {} + for _, childInstance in existingChildren do + -- 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) + if not accessSuccess then + continue + end + + local bucketsByClassName = buckets[name] + if bucketsByClassName == nil then + bucketsByClassName = {} + buckets[name] = bucketsByClassName + end + + local bucket = bucketsByClassName[className] + if bucket == nil then + bucket = { cursor = 1, instances = {} } + bucketsByClassName[className] = bucket + end + + table.insert(bucket.instances, childInstance) end + -- Tracks which existing children have already been paired, so one instance + -- isn't matched to two different virtual instances. + local visited = {} + for _, childId in ipairs(virtualInstance.Children) do local virtualChild = virtualInstances[childId] - 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) + local bucketsByClassName = buckets[virtualChild.Name] + local bucket = bucketsByClassName and bucketsByClassName[virtualChild.ClassName] + if bucket == nil then + -- No existing instance matches; diff will mark this id for creation. + Log.trace( + "hydrate: no existing instance matches {} ({}) for id {}", + virtualChild.Name, + virtualChild.ClassName, + childId + ) + continue + end - -- 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) - break + local instances = bucket.instances + + -- Advance past any leading children that have already been paired. The + -- cursor makes order-based matching amortized O(1) per child even for + -- very large groups, rather than rescanning the visited prefix. + while bucket.cursor <= #instances and visited[instances[bucket.cursor]] do + bucket.cursor += 1 + end + if bucket.cursor > #instances then + -- Every matching instance has already been paired with an earlier id. + Log.trace( + "hydrate: no unpaired instance left for {} ({}) for id {}", + virtualChild.Name, + virtualChild.ClassName, + childId + ) + continue + end + + -- The cursor points at the earliest unvisited child, so the slots from + -- here to the end bound how many candidates remain. Visited children + -- after the cursor (gaps) only appear once a group is small enough to be + -- scored -- the order-based path below always takes the earliest, which + -- keeps the visited region a contiguous prefix. So whenever this count + -- exceeds the cap it is exact, and we can pick the earliest match without + -- collecting anything. + local remaining = #instances - bucket.cursor + 1 + + local match + if remaining > MAX_CANDIDATES_TO_SCORE then + -- Too many to score affordably; take the earliest in child order, + -- reproducing the original Name + ClassName behavior. + match = instances[bucket.cursor] + Log.trace( + "hydrate: {} candidates named {} ({}) exceeds the scoring cap of {}; matching id {} by child order", + remaining, + virtualChild.Name, + virtualChild.ClassName, + MAX_CANDIDATES_TO_SCORE, + childId + ) + else + -- Collect the (at most `remaining`) unvisited candidates. + local candidates = {} + for index = bucket.cursor, #instances do + local childInstance = instances[index] + if not visited[childInstance] then + table.insert(candidates, childInstance) end end + + if #candidates == 1 then + -- Only one candidate, so there's nothing to disambiguate. + match = candidates[1] + else + -- Break the tie by choosing the candidate whose properties best + -- match the virtual instance, falling back to the earliest in + -- child order when scores are equal. + local bestScore = -1 + for _, childInstance in candidates do + local score = countMatchingProperties(childInstance, virtualChild, instanceMap) + if score > bestScore then + bestScore = score + match = childInstance + end + end + + stats.ambiguousGroups += 1 + stats.candidatesScored += #candidates + Log.trace( + "hydrate: disambiguated {} candidates named {} ({}) for id {} by property match (best score {})", + #candidates, + virtualChild.Name, + virtualChild.ClassName, + childId, + bestScore + ) + end end + + visited[match] = true + hydrateInner(stats, instanceMap, virtualInstances, childId, match) end end +local function hydrate(instanceMap, virtualInstances, rootId, rootInstance) + -- Tallies of the work hydration did, surfaced in a single debug log below so + -- the cost of property-based disambiguation is visible without per-node spam. + local stats = { + hydrated = 0, + ambiguousGroups = 0, + candidatesScored = 0, + } + + hydrateInner(stats, instanceMap, virtualInstances, rootId, rootInstance) + + Log.debug( + "Hydrated {} instances ({} ambiguous name+class groups, {} candidates scored)", + stats.hydrated, + stats.ambiguousGroups, + stats.candidatesScored + ) +end + return hydrate diff --git a/plugin/src/Reconciler/hydrate.spec.lua b/plugin/src/Reconciler/hydrate.spec.lua index 226d37f3..7f6fea7c 100644 --- a/plugin/src/Reconciler/hydrate.spec.lua +++ b/plugin/src/Reconciler/hydrate.spec.lua @@ -126,4 +126,140 @@ return function() expect(knownInstances.fromIds["CHILD1"]).to.equal(child1) expect(knownInstances.fromIds["CHILD2"]).to.equal(child2) end) + + it("should disambiguate duplicate-named siblings by matching properties", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = { "CHILD_A", "CHILD_B" }, + }, + + CHILD_A = { + ClassName = "StringValue", + Name = "a", + Properties = { Value = { String = "first" } }, + Children = {}, + }, + + CHILD_B = { + ClassName = "StringValue", + Name = "a", + Properties = { Value = { String = "second" } }, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + -- Created in the reverse order of the virtual children, so a purely + -- order-based tiebreak would mis-pair them. + local child1 = Instance.new("StringValue") + child1.Name = "a" + child1.Value = "second" + child1.Parent = rootInstance + + local child2 = Instance.new("StringValue") + child2.Name = "a" + child2.Value = "first" + child2.Parent = rootInstance + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(3) + expect(knownInstances.fromIds["CHILD_A"]).to.equal(child2) + expect(knownInstances.fromIds["CHILD_B"]).to.equal(child1) + end) + + it("should fall back to child order for duplicate-named siblings with no distinguishing properties", function() + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = { "CHILD_A", "CHILD_B" }, + }, + + CHILD_A = { + ClassName = "Folder", + Name = "a", + Properties = {}, + Children = {}, + }, + + CHILD_B = { + ClassName = "Folder", + Name = "a", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + local child1 = Instance.new("Folder") + child1.Name = "a" + child1.Parent = rootInstance + + local child2 = Instance.new("Folder") + child2.Name = "a" + child2.Parent = rootInstance + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(3) + -- With equal scores the earliest unvisited child wins, preserving the + -- original order-based behavior. + expect(knownInstances.fromIds["CHILD_A"]).to.equal(child1) + expect(knownInstances.fromIds["CHILD_B"]).to.equal(child2) + end) + + it("should fall back to child order for very large duplicate-named groups", function() + -- More candidates than hydrate is willing to score at once. The group + -- must fall back to order-based matching, so virtual child N pairs with + -- existing child N regardless of properties. + local count = 64 + + local knownInstances = InstanceMap.new() + local virtualInstances = { + ROOT = { + ClassName = "Folder", + Name = "Root", + Properties = {}, + Children = {}, + }, + } + + local rootInstance = Instance.new("Folder") + + local expectedInstances = {} + for i = 1, count do + local id = "CHILD_" .. i + table.insert(virtualInstances.ROOT.Children, id) + virtualInstances[id] = { + ClassName = "StringValue", + Name = "a", + -- Distinct values that, if scored, would pair by value rather + -- than by order. + Properties = { Value = { String = "value " .. i } }, + Children = {}, + } + + local child = Instance.new("StringValue") + child.Name = "a" + child.Value = "value " .. (count - i + 1) + child.Parent = rootInstance + expectedInstances[id] = child + end + + hydrate(knownInstances, virtualInstances, "ROOT", rootInstance) + + expect(knownInstances:size()).to.equal(count + 1) + for id, expectedInstance in expectedInstances do + expect(knownInstances.fromIds[id]).to.equal(expectedInstance) + end + end) end diff --git a/plugin/src/Reconciler/trueEquals.lua b/plugin/src/Reconciler/trueEquals.lua new file mode 100644 index 00000000..7dadf476 --- /dev/null +++ b/plugin/src/Reconciler/trueEquals.lua @@ -0,0 +1,100 @@ +--[[ + Fuzzy value-equality used to compare a decoded virtual property value against + the live value read from a real instance. Shared by `diff` (to decide whether + a property changed) and `hydrate` (to score candidate instances). +]] + +local function fuzzyEq(a: number, b: number, epsilon: number): boolean + return math.abs(a - b) < epsilon +end + +local function trueEquals(a, b): boolean + -- Exit early for simple equality values + if a == b then + return true + end + + -- Treat nil and { Ref = "000...0" } as equal + if + (a == nil and type(b) == "table" and b.Ref == "00000000000000000000000000000000") + or (b == nil and type(a) == "table" and a.Ref == "00000000000000000000000000000000") + then + return true + end + + local typeA, typeB = typeof(a), typeof(b) + + -- For tables, try recursive deep equality + if typeA == "table" and typeB == "table" then + local checkedKeys = {} + for key, value in a do + checkedKeys[key] = true + if not trueEquals(value, b[key]) then + return false + end + end + for key, value in b do + if checkedKeys[key] then + continue + end + if not trueEquals(value, a[key]) then + return false + end + end + return true + + -- For NaN, check if both values are not equal to themselves + elseif a ~= a and b ~= b then + return true + + -- For numbers, compare with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "number" and typeB == "number" then + return fuzzyEq(a, b, 0.0001) + + -- For EnumItem->number, compare the EnumItem's value + elseif typeA == "number" and typeB == "EnumItem" then + return a == b.Value + elseif typeA == "EnumItem" and typeB == "number" then + return a.Value == b + + -- For Color3s, compare to RGB ints to avoid floating point inequality + elseif typeA == "Color3" and typeB == "Color3" then + local aR, aG, aB = math.floor(a.R * 255), math.floor(a.G * 255), math.floor(a.B * 255) + local bR, bG, bB = math.floor(b.R * 255), math.floor(b.G * 255), math.floor(b.B * 255) + return aR == bR and aG == bG and aB == bB + + -- For CFrames, compare to components with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "CFrame" and typeB == "CFrame" then + local aComponents, bComponents = { a:GetComponents() }, { b:GetComponents() } + for i, aComponent in aComponents do + if not fuzzyEq(aComponent, bComponents[i], 0.0001) then + return false + end + end + return true + + -- For Vector3s, compare to components with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "Vector3" and typeB == "Vector3" then + local aComponents, bComponents = { a.X, a.Y, a.Z }, { b.X, b.Y, b.Z } + for i, aComponent in aComponents do + if not fuzzyEq(aComponent, bComponents[i], 0.0001) then + return false + end + end + return true + + -- For Vector2s, compare to components with epsilon of 0.0001 to avoid floating point inequality + elseif typeA == "Vector2" and typeB == "Vector2" then + local aComponents, bComponents = { a.X, a.Y }, { b.X, b.Y } + for i, aComponent in aComponents do + if not fuzzyEq(aComponent, bComponents[i], 0.0001) then + return false + end + end + return true + end + + return false +end + +return trueEquals diff --git a/plugin/src/ServeSession.lua b/plugin/src/ServeSession.lua index adee9e4d..7e9fc742 100644 --- a/plugin/src/ServeSession.lua +++ b/plugin/src/ServeSession.lua @@ -18,6 +18,7 @@ local PatchSet = require(script.Parent.PatchSet) local Reconciler = require(script.Parent.Reconciler) local strict = require(script.Parent.strict) local Settings = require(script.Parent.Settings) +local orderSwaps = require(script.Parent.orderSwaps) local Status = strict("Session.Status", { NotStarted = "NotStarted", @@ -320,6 +321,14 @@ function ServeSession:__replaceInstances(idList) return false end + -- Roblox appends to GetChildren() on every reparent, so the order in which + -- we re-parent replacements determines their final sibling order. + -- We process ancestors before descendants (so each replacement's + -- parent already exists when we re-parent it) and siblings in their original + -- GetChildren() order. Because the loop below moves the old instance's + -- children into the replacement *before* re-parenting the replacement, this + -- rebuilds GetChildren() exactly as it was before the swap. + local swaps = {} for id, replacement in replacements do local oldInstance = self.__instanceMap.fromIds[id] if not oldInstance then @@ -328,6 +337,16 @@ function ServeSession:__replaceInstances(idList) continue end + table.insert(swaps, { + id = id, + replacement = replacement, + oldInstance = oldInstance, + }) + end + + for _, swap in orderSwaps(swaps) do + local id, replacement, oldInstance = swap.id, swap.replacement, swap.oldInstance + self.__instanceMap:insert(id, replacement) Log.trace("Swapping Instance {} out via api/models/ endpoint", id) local oldParent = oldInstance.Parent diff --git a/plugin/src/orderSwaps.lua b/plugin/src/orderSwaps.lua new file mode 100644 index 00000000..aa4a438e --- /dev/null +++ b/plugin/src/orderSwaps.lua @@ -0,0 +1,44 @@ +--[[ + Determines the order in which `ServeSession:__replaceInstances` should swap + instances so that sibling order is preserved. + + Roblox appends to `GetChildren()` on every reparent, so the order in which we + re-parent replacements determines their final sibling order. To rebuild + `GetChildren()` exactly as it was before the swap we must: + + * process ancestors before descendants, so each replacement's parent already + exists when we re-parent the replacement, and + * process siblings in their original `GetChildren()` order. + + `swaps` is an array of `{ id, replacement, oldInstance }` entries. This sorts + the array in place (annotating each entry with `depth`/`siblingIndex`) and + returns it. +]] +local function orderSwaps(swaps) + for _, swap in swaps do + local depth = 0 + local ancestor = swap.oldInstance.Parent + while ancestor ~= nil do + depth += 1 + ancestor = ancestor.Parent + end + swap.depth = depth + + local siblingIndex = 0 + if swap.oldInstance.Parent ~= nil then + siblingIndex = table.find(swap.oldInstance.Parent:GetChildren(), swap.oldInstance) or 0 + end + swap.siblingIndex = siblingIndex + end + + table.sort(swaps, function(a, b) + if a.depth ~= b.depth then + return a.depth < b.depth + end + return a.siblingIndex < b.siblingIndex + end) + + return swaps +end + +return orderSwaps diff --git a/plugin/src/orderSwaps.spec.lua b/plugin/src/orderSwaps.spec.lua new file mode 100644 index 00000000..04c730fd --- /dev/null +++ b/plugin/src/orderSwaps.spec.lua @@ -0,0 +1,57 @@ +return function() + local orderSwaps = require(script.Parent.orderSwaps) + + it("orders same-named siblings by their original GetChildren order", function() + local parent = Instance.new("Model") + local a1 = Instance.new("Part") + a1.Name = "a" + a1.Parent = parent + local a2 = Instance.new("Part") + a2.Name = "a" + a2.Parent = parent + local a3 = Instance.new("Part") + a3.Name = "a" + a3.Parent = parent + + -- Input deliberately out of sibling order. + -- orderSwaps must restore the GetChildren() order. + local ordered = orderSwaps({ + { id = "3", oldInstance = a3 }, + { id = "1", oldInstance = a1 }, + { id = "2", oldInstance = a2 }, + }) + + expect(ordered[1].oldInstance).to.equal(a1) + expect(ordered[2].oldInstance).to.equal(a2) + expect(ordered[3].oldInstance).to.equal(a3) + end) + + it("orders ancestors before descendants", function() + local root = Instance.new("Model") + local child = Instance.new("Folder") + child.Parent = root + local grandchild = Instance.new("Part") + grandchild.Parent = child + + local ordered = orderSwaps({ + { id = "grandchild", oldInstance = grandchild }, + { id = "child", oldInstance = child }, + { id = "root", oldInstance = root }, + }) + + expect(ordered[1].oldInstance).to.equal(root) + expect(ordered[2].oldInstance).to.equal(child) + expect(ordered[3].oldInstance).to.equal(grandchild) + end) + + it("returns a single swap unchanged", function() + local part = Instance.new("Part") + + local ordered = orderSwaps({ + { id = "1", oldInstance = part }, + }) + + expect(#ordered).to.equal(1) + expect(ordered[1].oldInstance).to.equal(part) + end) +end diff --git a/src/change_processor.rs b/src/change_processor.rs index 6c5c8224..c1f245f2 100644 --- a/src/change_processor.rs +++ b/src/change_processor.rs @@ -200,7 +200,15 @@ impl JobThreadContext { if let Some(instance) = tree.get_instance(id) { if let Some(instigating_source) = &instance.metadata().instigating_source { match instigating_source { - InstigatingSource::Path(path) => fs::remove_file(path).unwrap(), + InstigatingSource::Path(path) => { + if let Err(err) = fs::remove_file(path) { + log::error!( + "Failed to remove file {}: {}", + path.display(), + err + ); + } + } InstigatingSource::ProjectNode { .. } => { log::warn!( "Cannot remove instance {:?}, it's from a project file", @@ -244,7 +252,13 @@ impl JobThreadContext { match instigating_source { InstigatingSource::Path(path) => { if let Some(Variant::String(value)) = changed_value { - fs::write(path, value).unwrap(); + if let Err(err) = fs::write(path, value) { + log::error!( + "Failed to write file {}: {}", + path.display(), + err + ); + } } else { log::warn!("Cannot change Source to non-string value."); } diff --git a/src/cli/build.rs b/src/cli/build.rs index c2ddebb3..82e81387 100644 --- a/src/cli/build.rs +++ b/src/cli/build.rs @@ -75,10 +75,10 @@ impl BuildCommand { _ => unreachable!(), }; - let project_path = resolve_path(&self.project); + let project_path = resolve_path(&self.project)?; log::trace!("Constructing in-memory filesystem"); - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; vfs.set_watch_enabled(self.watch); let session = ServeSession::new(vfs, project_path)?; @@ -87,11 +87,16 @@ impl BuildCommand { write_model(&session, &output_path, output_kind)?; if self.watch { - let rt = Runtime::new().unwrap(); + let rt = Runtime::new().context("Failed to start the async runtime for watch mode")?; loop { let receiver = session.message_queue().subscribe(cursor); - let (new_cursor, _patch_set) = rt.block_on(receiver).unwrap(); + let (new_cursor, _patch_set) = match rt.block_on(receiver) { + Ok(message) => message, + // The message queue was dropped, so there is nothing left + // to watch. Stop watching gracefully. + Err(_) => break, + }; cursor = new_cursor; write_model(&session, &output_path, output_kind)?; diff --git a/src/cli/fmt_project.rs b/src/cli/fmt_project.rs index 1c165974..e1c7adaa 100644 --- a/src/cli/fmt_project.rs +++ b/src/cli/fmt_project.rs @@ -18,10 +18,10 @@ pub struct FmtProjectCommand { impl FmtProjectCommand { pub fn run(self) -> anyhow::Result<()> { - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; vfs.set_watch_enabled(false); - let base_path = resolve_path(&self.project); + let base_path = resolve_path(&self.project)?; let project = Project::load_fuzzy(&vfs, &base_path)? .context("A project file is required to run 'rojo fmt-project'")?; diff --git a/src/cli/init.rs b/src/cli/init.rs index eb323310..434c2e34 100644 --- a/src/cli/init.rs +++ b/src/cli/init.rs @@ -9,7 +9,7 @@ use std::{ io::{self, Write}, }; -use anyhow::{bail, format_err}; +use anyhow::{bail, format_err, Context}; use clap::Parser; use fs_err as fs; use fs_err::OpenOptions; @@ -42,9 +42,9 @@ pub struct InitCommand { impl InitCommand { pub fn run(self) -> anyhow::Result<()> { - let template = self.kind.template(); + let template = self.kind.template()?; - let base_path = resolve_path(&self.path); + let base_path = resolve_path(&self.path)?; fs::create_dir_all(&base_path)?; let canonical = fs::canonicalize(&base_path)?; @@ -128,7 +128,7 @@ pub enum InitKind { } impl InitKind { - fn template(&self) -> InMemoryFs { + fn template(&self) -> anyhow::Result { let template_path = match self { Self::Place => "place", Self::Model => "model", @@ -136,20 +136,24 @@ impl InitKind { }; let snapshot: VfsSnapshot = bincode::deserialize(TEMPLATE_BINCODE) - .expect("Rojo's templates were not properly packed into Rojo's binary"); + .context("Rojo's templates were not properly packed into Rojo's binary. This is a bug in Rojo; please file an issue.")?; - if let VfsSnapshot::Dir { mut children } = snapshot { - if let Some(template) = children.remove(template_path) { - let mut fs = InMemoryFs::new(); - fs.load_snapshot("", template) - .expect("loading a template in memory should never fail"); - fs - } else { - panic!("template for project type {:?} is missing", self) - } - } else { - panic!("Rojo's templates were packed as a file instead of a directory") - } + let VfsSnapshot::Dir { mut children } = snapshot else { + bail!("Rojo's templates were packed as a file instead of a directory. This is a bug in Rojo; please file an issue."); + }; + + let template = children.remove(template_path).ok_or_else(|| { + format_err!( + "The template for project type {:?} is missing. This is a bug in Rojo; please file an issue.", + self + ) + })?; + + let mut fs = InMemoryFs::new(); + fs.load_snapshot("", template) + .context("Failed to load Rojo's bundled template into memory")?; + + Ok(fs) } } diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 7d3d87b1..caef95aa 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -12,6 +12,7 @@ mod upload; use std::{borrow::Cow, env, path::Path, str::FromStr}; +use anyhow::Context; use clap::Parser; use thiserror::Error; @@ -125,10 +126,14 @@ pub enum Subcommand { Syncback(SyncbackCommand), } -pub(super) fn resolve_path(path: &Path) -> Cow<'_, Path> { +pub(super) fn resolve_path(path: &Path) -> anyhow::Result> { if path.is_absolute() { - Cow::Borrowed(path) + Ok(Cow::Borrowed(path)) } else { - Cow::Owned(env::current_dir().unwrap().join(path)) + let current_dir = env::current_dir().context( + "Could not determine the current working directory. \ + It may have been deleted, or Rojo may not have permission to access it.", + )?; + Ok(Cow::Owned(current_dir.join(path))) } } diff --git a/src/cli/serve.rs b/src/cli/serve.rs index f89e4387..e33c3fc5 100644 --- a/src/cli/serve.rs +++ b/src/cli/serve.rs @@ -35,9 +35,9 @@ pub struct ServeCommand { impl ServeCommand { pub fn run(self, global: GlobalOptions) -> anyhow::Result<()> { - let project_path = resolve_path(&self.project); + let project_path = resolve_path(&self.project)?; - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; let session = Arc::new(ServeSession::new(vfs, project_path)?); @@ -53,8 +53,9 @@ impl ServeCommand { let server = LiveServer::new(session); - let _ = show_start_message(ip, port, global.color.into()); - server.start((ip, port).into()); + server.start((ip, port).into(), || { + let _ = show_start_message(ip, port, global.color.into()); + })?; Ok(()) } diff --git a/src/cli/sourcemap.rs b/src/cli/sourcemap.rs index 94369e7c..10fff590 100644 --- a/src/cli/sourcemap.rs +++ b/src/cli/sourcemap.rs @@ -5,6 +5,7 @@ use std::{ path::{self, Path, PathBuf}, }; +use anyhow::Context; use clap::Parser; use fs_err::File; use memofs::Vfs; @@ -71,10 +72,10 @@ pub struct SourcemapCommand { impl SourcemapCommand { pub fn run(self) -> anyhow::Result<()> { - let project_path = fs_err::canonicalize(resolve_path(&self.project))?; + let project_path = fs_err::canonicalize(resolve_path(&self.project)?)?; log::trace!("Constructing filesystem with StdBackend"); - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; vfs.set_watch_enabled(self.watch); log::trace!("Setting up session for sourcemap generation"); @@ -100,11 +101,16 @@ impl SourcemapCommand { if self.watch { log::trace!("Setting up runtime for watch mode"); - let rt = Runtime::new().unwrap(); + let rt = Runtime::new().context("Failed to start the async runtime for watch mode")?; loop { let receiver = session.message_queue().subscribe(cursor); - let (new_cursor, patch_set) = rt.block_on(receiver).unwrap(); + let (new_cursor, patch_set) = match rt.block_on(receiver) { + Ok(message) => message, + // The message queue was dropped, so there is nothing left + // to watch. Stop watching gracefully. + Err(_) => break, + }; cursor = new_cursor; if patch_set_affects_sourcemap(&session, &patch_set, filter) { diff --git a/src/cli/syncback.rs b/src/cli/syncback.rs index 282d55c8..7bb274fb 100644 --- a/src/cli/syncback.rs +++ b/src/cli/syncback.rs @@ -58,8 +58,8 @@ pub struct SyncbackCommand { impl SyncbackCommand { pub fn run(&self, global: GlobalOptions) -> anyhow::Result<()> { - let path_old = resolve_path(&self.project); - let path_new = resolve_path(&self.input); + let path_old = resolve_path(&self.project)?; + let path_new = resolve_path(&self.input)?; let input_kind = FileKind::from_path(&path_new).context(UNKNOWN_INPUT_KIND_ERR)?; let dom_start_timer = Instant::now(); @@ -69,7 +69,7 @@ impl SyncbackCommand { dom_start_timer.elapsed().as_secs_f32() ); - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; vfs.set_watch_enabled(false); let project_start_timer = Instant::now(); diff --git a/src/cli/upload.rs b/src/cli/upload.rs index 3c3610fd..bc9d427e 100644 --- a/src/cli/upload.rs +++ b/src/cli/upload.rs @@ -38,9 +38,9 @@ pub struct UploadCommand { impl UploadCommand { pub fn run(self) -> Result<(), anyhow::Error> { - let project_path = resolve_path(&self.project); + let project_path = resolve_path(&self.project)?; - let vfs = Vfs::new_default(); + let vfs = Vfs::new_default()?; let session = ServeSession::new(vfs, project_path)?; diff --git a/src/snapshot_middleware/csv.rs b/src/snapshot_middleware/csv.rs index 18fa1424..b4a86efc 100644 --- a/src/snapshot_middleware/csv.rs +++ b/src/snapshot_middleware/csv.rs @@ -195,7 +195,7 @@ struct LocalizationEntry<'a> { /// https://github.com/BurntSushi/rust-csv/issues/151 /// /// This function operates in one step in order to minimize data-copying. -fn convert_localization_csv(contents: &[u8]) -> Result { +fn convert_localization_csv(contents: &[u8]) -> anyhow::Result { let mut reader = csv::Reader::from_reader(contents); let headers = reader.headers()?.clone(); @@ -237,7 +237,7 @@ fn convert_localization_csv(contents: &[u8]) -> Result { } let encoded = - serde_json::to_string(&entries).expect("Could not encode JSON for localization table"); + serde_json::to_string(&entries).context("Could not encode JSON for localization table")?; Ok(encoded) } diff --git a/src/web/mod.rs b/src/web/mod.rs index d6df4b15..8e33d9c5 100644 --- a/src/web/mod.rs +++ b/src/web/mod.rs @@ -12,6 +12,7 @@ use std::convert::Infallible; use std::net::SocketAddr; use std::sync::Arc; +use anyhow::Context; use hyper::{ server::Server, service::{make_service_fn, service_fn}, @@ -30,7 +31,12 @@ impl LiveServer { LiveServer { serve_session } } - pub fn start(self, address: SocketAddr) { + /// Starts the server on the given address, blocking until it stops. + /// + /// `on_listening` is invoked once the server has successfully bound to the + /// address, so callers can defer printing any "listening" message until + /// after binding can no longer fail (e.g. due to the port being in use). + pub fn start(self, address: SocketAddr, on_listening: impl FnOnce()) -> anyhow::Result<()> { let serve_session = Arc::clone(&self.serve_session); let make_service = make_service_fn(move |_conn| { @@ -53,9 +59,25 @@ impl LiveServer { } }); - let rt = Runtime::new().unwrap(); + let rt = Runtime::new().context("Failed to start the async runtime for the web server")?; let _guard = rt.enter(); - let server = Server::bind(&address).serve(make_service); - rt.block_on(server).unwrap(); + let server = Server::try_bind(&address) + .with_context(|| { + format!( + "Could not start the Rojo server on {address}.\n\ + The address may already be in use or reserved. Another Rojo server might already \ + be running, or another program may be using that port.\n\ + You can pick a different port with the --port option." + ) + })? + .serve(make_service); + + // Binding succeeded, so it's now safe to tell the user we're listening. + on_listening(); + + rt.block_on(server) + .context("The Rojo web server encountered a fatal error")?; + + Ok(()) } } diff --git a/src/web/ui.rs b/src/web/ui.rs index 4d63eaad..f6361b4f 100644 --- a/src/web/ui.rs +++ b/src/web/ui.rs @@ -6,7 +6,7 @@ use std::{borrow::Cow, sync::Arc, time::Duration}; -use hyper::{header, Body, Method, Request, Response, StatusCode}; +use hyper::{Body, Method, Request, Response, StatusCode}; use rbx_dom_weak::types::{Ref, Variant}; use ritz::{html, Fragment, HtmlContent, HtmlSelfClosingTag}; @@ -16,7 +16,7 @@ use crate::{ web::{ assets, interface::{ErrorResponse, SERVER_VERSION}, - util::json, + util::{json, response}, }, }; @@ -45,17 +45,11 @@ impl UiService { } fn handle_logo(&self) -> Response { - Response::builder() - .header(header::CONTENT_TYPE, "image/png") - .body(Body::from(assets::logo())) - .unwrap() + response(StatusCode::OK, "image/png", assets::logo()) } fn handle_icon(&self) -> Response { - Response::builder() - .header(header::CONTENT_TYPE, "image/png") - .body(Body::from(assets::icon())) - .unwrap() + response(StatusCode::OK, "image/png", assets::icon()) } fn handle_home(&self) -> Response { @@ -66,10 +60,11 @@ impl UiService { }); - Response::builder() - .header(header::CONTENT_TYPE, "text/html") - .body(Body::from(format!("{}", page))) - .unwrap() + response( + StatusCode::OK, + "text/html", + format!("{}", page), + ) } fn handle_show_instances(&self) -> Response { @@ -80,10 +75,11 @@ impl UiService { { Self::instance(&tree, root_id) } }); - Response::builder() - .header(header::CONTENT_TYPE, "text/html") - .body(Body::from(format!("{}", page))) - .unwrap() + response( + StatusCode::OK, + "text/html", + format!("{}", page), + ) } fn instance(tree: &RojoTree, id: Ref) -> HtmlContent<'_> { diff --git a/src/web/util.rs b/src/web/util.rs index 1c1cbb50..bd1f6b81 100644 --- a/src/web/util.rs +++ b/src/web/util.rs @@ -1,6 +1,27 @@ use hyper::{header::CONTENT_TYPE, Body, Response, StatusCode}; use serde::{Deserialize, Serialize}; +/// Builds an HTTP response, falling back to an empty `500` response (rather than +/// panicking) if the response could not be constructed. With constant headers +/// and a valid status code this never actually fails, but routing every +/// response through here means a malformed response can never crash the server. +pub fn response( + code: StatusCode, + content_type: &'static str, + body: impl Into, +) -> Response { + Response::builder() + .status(code) + .header(CONTENT_TYPE, content_type) + .body(body.into()) + .unwrap_or_else(|err| { + log::error!("Failed to build HTTP response: {}", err); + let mut fallback = Response::new(Body::empty()); + *fallback.status_mut() = StatusCode::INTERNAL_SERVER_ERROR; + fallback + }) +} + pub fn msgpack_ok(value: T) -> Response { msgpack(value, StatusCode::OK) } @@ -12,18 +33,14 @@ pub fn msgpack(value: T, code: StatusCode) -> Response { .with_struct_map(); if let Err(err) = value.serialize(&mut serializer) { - return Response::builder() - .status(StatusCode::INTERNAL_SERVER_ERROR) - .header(CONTENT_TYPE, "text/plain") - .body(Body::from(err.to_string())) - .unwrap(); + return response( + StatusCode::INTERNAL_SERVER_ERROR, + "text/plain", + err.to_string(), + ); }; - Response::builder() - .status(code) - .header(CONTENT_TYPE, "application/msgpack") - .body(Body::from(serialized)) - .unwrap() + response(code, "application/msgpack", serialized) } pub fn serialize_msgpack(value: T) -> anyhow::Result> { @@ -49,17 +66,13 @@ pub fn json(value: T, code: StatusCode) -> Response { let serialized = match serde_json::to_string(&value) { Ok(v) => v, Err(err) => { - return Response::builder() - .status(StatusCode::INTERNAL_SERVER_ERROR) - .header(CONTENT_TYPE, "text/plain") - .body(Body::from(err.to_string())) - .unwrap(); + return response( + StatusCode::INTERNAL_SERVER_ERROR, + "text/plain", + err.to_string(), + ); } }; - Response::builder() - .status(code) - .header(CONTENT_TYPE, "application/json") - .body(Body::from(serialized)) - .unwrap() + response(code, "application/json", serialized) }