diff --git a/plugins/sandbox.echo/Sandbox.Echo/EchoPlugin.cs b/plugins/sandbox.echo/Sandbox.Echo/EchoPlugin.cs index e8da0fb..6c84665 100644 --- a/plugins/sandbox.echo/Sandbox.Echo/EchoPlugin.cs +++ b/plugins/sandbox.echo/Sandbox.Echo/EchoPlugin.cs @@ -6,9 +6,10 @@ using Sandbox.Echo.Contracts; namespace Sandbox.Echo; /// -/// M0's test fixture. Exists only to exercise the reload loop end to end — -/// edit, rebuild, ALC reload, headless run, dump — before any real -/// subsystem exists to test it against. See M0 in docs/kernel-contract.md §8. +/// M0's test fixture. Exists to exercise the full loop end to end — plugin +/// load, a real cross-ALC system registered and invoked by Schedule, and +/// hot reload — before any real subsystem exists to test any of it +/// against. See M0 in docs/kernel-contract.md §8. /// public sealed class EchoPlugin : IPlugin { @@ -27,7 +28,7 @@ public sealed class EchoPlugin : IPlugin static void Tick(IWorld world) { - // TODO(M0): bump every Ping.Count by one. Placeholder until the - // Scheduler actually exists to invoke this. + foreach (var go in world.Query()) + go.GetComponent()!.Count++; } } diff --git a/src/Engine.Kernel/Scheduling/ISchedule.cs b/src/Engine.Kernel/Scheduling/ISchedule.cs index a28ff58..81fcf8d 100644 --- a/src/Engine.Kernel/Scheduling/ISchedule.cs +++ b/src/Engine.Kernel/Scheduling/ISchedule.cs @@ -1,3 +1,5 @@ +using Engine.Kernel.World; + namespace Engine.Kernel.Scheduling; /// @@ -5,7 +7,18 @@ namespace Engine.Kernel.Scheduling; /// public interface ISchedule { - ISystemBuilder Add(Stage stage, Delegate system); + /// + /// Action<IWorld> specifically, not Delegate — a + /// lambda passed where the target type is exactly Delegate + /// doesn't reliably compile down to System.Action<IWorld> + /// at runtime (the compiler's natural-type inference for lambdas can + /// synthesize a different, unspeakable delegate type instead, so a + /// runtime is Action<IWorld> check silently fails). Method + /// groups and lambdas both convert to Action<IWorld> + /// correctly when it's the actual parameter type — see §3 for both + /// forms in use. + /// + ISystemBuilder Add(Stage stage, Action system); /// Called from a plugin's Shutdown() — must remove everything /// Configure() added, or the ALC it lives in will never unload. See §4. diff --git a/src/Engine.Kernel/Scheduling/Schedule.cs b/src/Engine.Kernel/Scheduling/Schedule.cs index c09cfe8..8465ce2 100644 --- a/src/Engine.Kernel/Scheduling/Schedule.cs +++ b/src/Engine.Kernel/Scheduling/Schedule.cs @@ -1,13 +1,10 @@ using System.Reflection; +using Engine.Kernel.World; namespace Engine.Kernel.Scheduling; /// -/// Bookkeeping only for now — no stage execution, no parallelism, no -/// enforcement of declared access. That's the real Scheduler work described -/// in docs/kernel-contract.md §2 and §7; this pass exists to make -/// actually correct, because it's the one thing -/// PluginHost's reload correctness depends on. +/// Registration plus execution. See docs/kernel-contract.md §2 and §7. /// /// Ownership is tracked by the registering delegate's declaring assembly, /// not by a string tag on each entry — a plugin can only accidentally @@ -24,7 +21,7 @@ public sealed class Schedule : ISchedule internal void RegisterPlugin(string pluginId, Assembly implementationAssembly) => _pluginAssemblies[pluginId] = implementationAssembly; - public ISystemBuilder Add(Stage stage, Delegate system) + public ISystemBuilder Add(Stage stage, Action system) { var entry = new SystemEntry(stage, system); _systems.Add(entry); @@ -39,4 +36,64 @@ public sealed class Schedule : ISchedule if (_pluginAssemblies.Remove(pluginId, out var assembly)) _systems.RemoveAll(e => e.System.Method.DeclaringType?.Assembly == assembly); } + + /// + /// Runs every system registered for , grouped + /// into conflict-free batches by declared Reads/Writes (see + /// ) and, in DEBUG builds, enforced against + /// what each one actually touches (see SystemAccessScope). + /// + /// TODO: batches are computed but run sequentially, not on separate + /// threads — real parallel dispatch needs structural changes + /// (Create/Destroy/AddComponent/RemoveComponent) deferred to a command + /// buffer flushed after the batch first. Without that, two systems with + /// disjoint *declared* types can still race on shared, non-thread-safe + /// storage: AddComponent<T>() on a GameObject mutates that + /// GameObject's own component list regardless of T, and GameWorld's + /// index/roots collections aren't safe for concurrent mutation either. + /// See the Scheduler row in docs/kernel-contract.md §2. + /// + public void RunStage(Stage stage, IWorld world) + { + var stageSystems = _systems.Where(e => e.Stage == stage).ToList(); + + foreach (var batch in ComputeBatches(stageSystems)) + { + foreach (var entry in batch) + Invoke(entry, world); + } + } + + private static void Invoke(SystemEntry entry, IWorld world) + { + using var _ = SystemAccessScope.Enter(entry.Reads, entry.Writes); + entry.System(world); + } + + /// + /// Groups systems into the fewest sequential batches such that no two + /// systems in the same batch conflict — greedy first-fit, preserving + /// registration order. Not yet consumed for real parallelism (see the + /// TODO on RunStage), but exercised directly by ScheduleTests so the + /// conflict logic itself is validated independent of that. + /// + internal static List> ComputeBatches(IReadOnlyList systems) + { + var batches = new List>(); + + foreach (var entry in systems) + { + var batch = batches.FirstOrDefault(b => b.TrueForAll(other => !Conflicts(entry, other))); + + if (batch is not null) + batch.Add(entry); + else + batches.Add([entry]); + } + + return batches; + } + + private static bool Conflicts(SystemEntry a, SystemEntry b) => + a.Writes.Overlaps(b.Reads) || a.Writes.Overlaps(b.Writes) || b.Writes.Overlaps(a.Reads); } diff --git a/src/Engine.Kernel/Scheduling/SystemAccessScope.cs b/src/Engine.Kernel/Scheduling/SystemAccessScope.cs new file mode 100644 index 0000000..6007a27 --- /dev/null +++ b/src/Engine.Kernel/Scheduling/SystemAccessScope.cs @@ -0,0 +1,68 @@ +namespace Engine.Kernel.Scheduling; + +/// +/// Ambient, per-thread record of which component types the currently +/// running system declared via Reads<T>()/Writes<T>(). GameWorld +/// and GameObject consult this — when one is active — to enforce +/// docs/kernel-contract.md §7's rule that an undeclared access fails +/// loudly instead of silently working by accident. +/// +/// No scope is active outside of Schedule.RunStage's invocation of a +/// system — editor code, tests, and initial scene construction are all +/// unconstrained by design; enforcement exists for the frame loop, not for +/// every touch of a GameObject anywhere in the process. +/// +/// ThreadLocal rather than a plain static field: batches run sequentially +/// today (see the TODO on Schedule.RunStage), but this is already correct +/// for when a batch's systems run on separate threads instead. +/// +internal static class SystemAccessScope +{ + private static readonly ThreadLocal<(IReadOnlySet Reads, IReadOnlySet Writes)?> Current = new(); + + public static IDisposable Enter(IReadOnlySet reads, IReadOnlySet writes) + { + var previous = Current.Value; + Current.Value = (reads, writes); + return new Restore(previous); + } + + /// Querying or fetching a component counts as a read — either + /// Reads<T>() or Writes<T>() satisfies it. + public static void CheckRead(Type componentType) + { + var scope = Current.Value; + if (scope is null) + return; + + if (!scope.Value.Reads.Contains(componentType) && !scope.Value.Writes.Contains(componentType)) + { + throw new InvalidOperationException( + $"A system read '{componentType.Name}' without declaring Reads<{componentType.Name}>() " + + $"or Writes<{componentType.Name}>() — see docs/kernel-contract.md §7."); + } + } + + /// Structurally changing a GameObject's components — adding or + /// removing one — requires Writes<T>() specifically. Mutating a + /// component's own fields after GetComponent<T>() isn't + /// interceptable this way; see the note on GameObject.AddComponent. + public static void CheckWrite(Type componentType) + { + var scope = Current.Value; + if (scope is null) + return; + + if (!scope.Value.Writes.Contains(componentType)) + { + throw new InvalidOperationException( + $"A system structurally changed '{componentType.Name}' without declaring " + + $"Writes<{componentType.Name}>() — see docs/kernel-contract.md §7."); + } + } + + private sealed class Restore((IReadOnlySet Reads, IReadOnlySet Writes)? previous) : IDisposable + { + public void Dispose() => Current.Value = previous; + } +} diff --git a/src/Engine.Kernel/Scheduling/SystemEntry.cs b/src/Engine.Kernel/Scheduling/SystemEntry.cs index b02eac3..07cee7d 100644 --- a/src/Engine.Kernel/Scheduling/SystemEntry.cs +++ b/src/Engine.Kernel/Scheduling/SystemEntry.cs @@ -7,10 +7,10 @@ using Engine.Kernel.World; /// Scheduler can build the conflict graph described in docs/kernel-contract.md /// §2 — nothing consumes them yet; that's real Scheduler work, not this pass. /// -internal sealed class SystemEntry(Stage stage, Delegate system) +internal sealed class SystemEntry(Stage stage, Action system) { public Stage Stage { get; } = stage; - public Delegate System { get; } = system; + public Action System { get; } = system; public string? After { get; set; } public HashSet Reads { get; } = []; public HashSet Writes { get; } = []; diff --git a/src/Engine.Kernel/World/GameObject.cs b/src/Engine.Kernel/World/GameObject.cs index 818bd2f..989c997 100644 --- a/src/Engine.Kernel/World/GameObject.cs +++ b/src/Engine.Kernel/World/GameObject.cs @@ -1,4 +1,5 @@ using System.Numerics; +using Engine.Kernel.Scheduling; namespace Engine.Kernel.World; @@ -77,6 +78,8 @@ public sealed class GameObject public T? GetComponent() where T : Component { + SystemAccessScope.CheckRead(typeof(T)); + foreach (var component in _components) { if (component is T match) @@ -86,8 +89,19 @@ public sealed class GameObject return null; } + /// + /// Adding a component requires Writes<T>() — checked here, + /// at the structural change. What isn't and can't be checked: mutating + /// a component's own fields after the fact, e.g. + /// go.GetComponent<T>()!.Value = 5. That's a plain field + /// write on a plain object, with nothing to intercept it — see + /// docs/kernel-contract.md §7's note on why components stay plain + /// classes rather than something that could enforce this fully. + /// public T AddComponent() where T : Component, new() { + SystemAccessScope.CheckWrite(typeof(T)); + var component = new T(); _components.Add(component); Owner?.IndexComponentAdded(this, component); @@ -96,6 +110,8 @@ public sealed class GameObject public void RemoveComponent() where T : Component { + SystemAccessScope.CheckWrite(typeof(T)); + for (var i = 0; i < _components.Count; i++) { if (_components[i] is not T match) diff --git a/src/Engine.Kernel/World/GameWorld.cs b/src/Engine.Kernel/World/GameWorld.cs index e6635ae..1e575ce 100644 --- a/src/Engine.Kernel/World/GameWorld.cs +++ b/src/Engine.Kernel/World/GameWorld.cs @@ -1,3 +1,5 @@ +using Engine.Kernel.Scheduling; + namespace Engine.Kernel.World; /// @@ -56,8 +58,11 @@ public sealed class GameWorld : IWorld /// expected to queue structural changes to a stage boundary rather than /// let a running system trigger this — see the Scheduler row in §2. /// - public IEnumerable Query() where T : Component => - _index.TryGetValue(typeof(T), out var set) ? set : []; + public IEnumerable Query() where T : Component + { + SystemAccessScope.CheckRead(typeof(T)); + return _index.TryGetValue(typeof(T), out var set) ? set : []; + } internal void IndexComponentAdded(GameObject go, Component component) { diff --git a/tests/Engine.ConformanceHarness/AssemblyInfo.cs b/tests/Engine.ConformanceHarness/AssemblyInfo.cs new file mode 100644 index 0000000..fab288c --- /dev/null +++ b/tests/Engine.ConformanceHarness/AssemblyInfo.cs @@ -0,0 +1,9 @@ +using Xunit; + +// ALC-unload tests are sensitive to *any* concurrent activity that might +// transiently keep something reachable — xUnit parallelizes across test +// classes by default, and AlcUnloadTests running alongside +// PluginSystemTests in the same process is exactly the kind of +// interference that produces flaky, hard-to-explain unload failures. +// Everything in this assembly runs sequentially instead. +[assembly: CollectionBehavior(DisableTestParallelization = true)] diff --git a/tests/Engine.ConformanceHarness/Engine.ConformanceHarness.csproj b/tests/Engine.ConformanceHarness/Engine.ConformanceHarness.csproj index 37079ea..00cf5fc 100644 --- a/tests/Engine.ConformanceHarness/Engine.ConformanceHarness.csproj +++ b/tests/Engine.ConformanceHarness/Engine.ConformanceHarness.csproj @@ -18,6 +18,14 @@ + + +