diff --git a/Chickensoft.AutoInject.Tests/coverage.sh b/Chickensoft.AutoInject.Tests/coverage.sh index e9fb87e..4cd7581 100755 --- a/Chickensoft.AutoInject.Tests/coverage.sh +++ b/Chickensoft.AutoInject.Tests/coverage.sh @@ -26,7 +26,7 @@ dotnet build --no-restore coverlet \ "./.godot/mono/temp/bin/Debug" --verbosity detailed \ - --target $GODOT \ + --target "$GODOT" \ --targetargs "--headless --run-tests --coverage --quit-on-finish" \ --format "opencover" \ --output "./coverage/coverage.xml" \ diff --git a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs index f7520aa..f80cd6d 100644 --- a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs +++ b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependencyResolver.cs @@ -128,8 +128,9 @@ public static TValue DependOn( // First, check dependency fakes. Using a faked value takes priority over // all the other dependency resolution methods. var state = dependent.MixinState.Get(); - if (state.ProviderFakes.TryGetValue(typeof(TValue), out var fakeProvider)) { - return fakeProvider.Value(); + if (state.ProviderFakes.TryGetValue(typeof(TValue), out var fakeProvider) + && fakeProvider is DefaultProvider faker) { + return faker.Value(); } // Lookup dependency, per usual, respecting any fallback values if there @@ -145,15 +146,16 @@ public static TValue DependOn( if (providerNode is IProvide provider) { return provider.Value(); } - else if (providerNode is DefaultProvider defaultProvider) { + else if (providerNode is DefaultProvider defaultProvider) { return defaultProvider.Value(); } } else if (fallback is not null) { // See if we were given a fallback. - var provider = new DefaultProvider(fallback()); + var value = fallback(); + var provider = new DefaultProvider(value, fallback); state.Dependencies.Add(typeof(TValue), provider); - return (TValue)provider.Value(); + return provider.Value(); } throw new ProviderNotFoundException(typeof(TValue)); @@ -283,15 +285,45 @@ void onProviderInitialized(IBaseProvider provider) { // for fallback values. } - public class DefaultProvider : IBaseProvider { - private readonly dynamic _value; + public class DefaultProvider : IBaseProvider { + internal object _value; + private readonly Func _fallback; public ProviderState ProviderState { get; } - public DefaultProvider(dynamic value) { - _value = value; + // When working with reference types, we must wrap the value in a + // WeakReference() to allow the garbage collection to work when the + // assembly is being unloaded or reloaded; such as in the case of + // rebuilding within the Godot Editor if you've instantiated a node + // and run it as a tool script. + public DefaultProvider(object value, Func? fallback = default) { + _fallback = fallback ?? (() => (TValue?)value); + + _value = value.GetType().IsValueType + ? value + : new WeakReference(value); + ProviderState = new() { IsInitialized = true }; } - public dynamic Value() => _value; + public TValue Value() { + if (_value is WeakReference weakReference) { + // Try to return a reference type. + if (weakReference.Target is TValue target) { + return target; + } + + var value = _fallback() ?? + throw new InvalidOperationException( + "Fallback cannot create a null value" + ); + + _value = new WeakReference(value); + + return value; + + } + // Return a value type. + return (TValue)_value; + } } } diff --git a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependentState.cs b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependentState.cs index 2d746dc..2b3e8bf 100644 --- a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependentState.cs +++ b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/DependentState.cs @@ -51,7 +51,7 @@ public class DependentState { /// allows nodes being unit-tested to provide fake providers during unit tests /// that return mock or faked values. /// - public Dictionary ProviderFakes { + public DependencyResolver.DependencyTable ProviderFakes { get; } = []; } diff --git a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/IDependent.cs b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/IDependent.cs index f34c19f..29774f1 100644 --- a/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/IDependent.cs +++ b/Chickensoft.AutoInject.Tests/src/auto_inject/dependent/IDependent.cs @@ -98,6 +98,6 @@ this is IIntrospective introspective && public void FakeDependency(T value) where T : notnull { AddStateIfNeeded(); MixinState.Get().ProviderFakes[typeof(T)] = - new DependencyResolver.DefaultProvider(value); + new DependencyResolver.DefaultProvider(value); } } diff --git a/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs b/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs index 589b7bd..6c1d7f8 100644 --- a/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs +++ b/Chickensoft.AutoInject.Tests/test/fixtures/Dependents.cs @@ -1,5 +1,6 @@ namespace Chickensoft.AutoInject.Tests.Subjects; +using System; using Chickensoft.Introspection; using Godot; @@ -56,15 +57,50 @@ public void OnResolved() { } } +[Meta(typeof(IAutoOn), typeof(IDependent))] +public partial class WeakReferenceDependent : Node { + public override void _Notification(int what) => this.Notify(what); + + [Dependency] + public object MyDependency => this.DependOn(Fallback); + + public Func? Fallback { get; set; } + public bool OnResolvedCalled { get; private set; } + + public void OnReady() { } + + public void OnResolved() => OnResolvedCalled = true; +} + +[Meta(typeof(IAutoOn), typeof(IDependent))] +public partial class ReferenceDependentFallback : Node { + public override void _Notification(int what) => this.Notify(what); + + [Dependency] + public object MyDependency => this.DependOn(() => FallbackValue); + + public object FallbackValue { get; set; } = new Resource(); + public bool OnResolvedCalled { get; private set; } + public object ResolvedValue { get; set; } = null!; + + public void OnReady() { } + + public void OnResolved() { + OnResolvedCalled = true; + ResolvedValue = MyDependency; + } +} + [Meta(typeof(IAutoOn), typeof(IDependent))] public partial class IntDependent : Node { public override void _Notification(int what) => this.Notify(what); [Dependency] - public int MyDependency => this.DependOn(); + public int MyDependency => this.DependOn(FallbackValue); public bool OnResolvedCalled { get; private set; } public int ResolvedValue { get; set; } + public Func? FallbackValue { get; set; } public void OnReady() { } diff --git a/Chickensoft.AutoInject.Tests/test/src/MiscTest.cs b/Chickensoft.AutoInject.Tests/test/src/MiscTest.cs index 58223a9..7ebf576 100644 --- a/Chickensoft.AutoInject.Tests/test/src/MiscTest.cs +++ b/Chickensoft.AutoInject.Tests/test/src/MiscTest.cs @@ -47,7 +47,7 @@ public void IDependentOnResolvedDoesNothing() { [Test] public void DefaultProviderState() { - var defaultProvider = new DependencyResolver.DefaultProvider("hi"); + var defaultProvider = new DependencyResolver.DefaultProvider("hi"); defaultProvider.ProviderState.ShouldNotBeNull(); } } diff --git a/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs b/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs index b5e825f..9c3c487 100644 --- a/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs +++ b/Chickensoft.AutoInject.Tests/test/src/ResolutionTest.cs @@ -1,5 +1,7 @@ namespace Chickensoft.AutoInject.Tests; +using System; +using System.Collections.Generic; using System.Threading.Tasks; using Chickensoft.AutoInject.Tests.Subjects; using Chickensoft.GoDotTest; @@ -131,9 +133,9 @@ public void ThrowsWhenNoProviderFound() { } [Test] - public void UsesFallbackValueWhenNoProviderFound() { - var fallback = "Hello, world!"; - var dependent = new StringDependentFallback { + public void UsesReferenceFallbackValueWhenNoProviderFound() { + var fallback = new Resource(); + var dependent = new ReferenceDependentFallback { FallbackValue = fallback }; @@ -142,6 +144,77 @@ public void UsesFallbackValueWhenNoProviderFound() { dependent.ResolvedValue.ShouldBe(fallback); dependent.MyDependency.ShouldBe(fallback); } + + [Test] + public void DependsOnValueType() { + var value = 10; + var depObj = new IntDependent() { FallbackValue = () => value }; + var dependent = depObj as IDependent; + + depObj._Notification((int)Node.NotificationReady); + + + depObj.OnResolvedCalled.ShouldBeTrue(); + depObj.ResolvedValue.ShouldBe(value); + + depObj._Notification((int)Node.NotificationExitTree); + + dependent.DependentState.Pending.ShouldBeEmpty(); + + depObj.QueueFree(); + } + + [Test] + public void ThrowsIfFallbackProducesNullAfterPreviousValueIsGarbageCollected( + ) { + var currentFallback = 0; + var replacementValue = new object(); + var fallbacks = new List() { new(), null, replacementValue }; + + var value = Utils.CreateWeakReference(); + + // Fallback will be called 3 times in this test. First will be non-null, + // second will be null, third will be non-null and different from the first. + var depObj = new WeakReferenceDependent() { + Fallback = () => fallbacks[currentFallback++]! + }; + + var dependent = depObj as IDependent; + + depObj._Notification((int)Node.NotificationReady); + + // Let's access the fallback value to ensure the default provider is setup. + depObj.MyDependency.ShouldNotBeNull(); + + // Simulate a garbage collected object. We support weak references to + // dependencies to avoid causing build issues when reloading the scene. + Utils.ClearWeakReference(value); + + // To test this highly specific scenario, we have to clear ALL + // weak references to the object, including the one in the default provider + // that's generated behind-the-scenes for us. + + // Let's dig out the weak ref used in the default provider from the + // dependent's internal state... + var underlyingDefaultProvider = + (DependencyResolver.DefaultProvider) + depObj.MixinState.Get().Dependencies[typeof(object)]; + + var actualWeakRef = (WeakReference)underlyingDefaultProvider._value; + + Utils.ClearWeakReference(actualWeakRef); + + var e = Should.Throw( + () => depObj.MyDependency + ); + + e.Message.ShouldContain("cannot create a null value"); + + // Now that the fallback returns a valid value, the dependency should + // be resolved once again. + depObj.MyDependency.ShouldBeSameAs(replacementValue); + } + [Test] public void ThrowsOnDependencyTableThatWasTamperedWith() { var fallback = "Hello, world!"; @@ -232,4 +305,17 @@ public BadProvider() { }; } } + + public static class Utils { + public static WeakReference CreateWeakReference() => new(new object()); + + public static void ClearWeakReference(WeakReference weakReference) { + weakReference.Target = null; + + while (weakReference.Target is not null) { + GC.Collect(); + GC.WaitForPendingFinalizers(); + } + } + } } diff --git a/cspell.json b/cspell.json index 4f5e4dd..518c935 100644 --- a/cspell.json +++ b/cspell.json @@ -13,9 +13,6 @@ "Chickensoft.AutoInject/nupkg/**/*.*" ], "words": [ - "devbuild", - "mktemp", - "skipautoprops", "assemblyfilters", "automerge", "branchcoverage", @@ -25,7 +22,9 @@ "Chickensoft", "classfilters", "CYGWIN", + "devbuild", "endregion", + "Finalizers", "globaltool", "godotengine", "godotpackage", @@ -35,6 +34,7 @@ "Metatype", "methodcoverage", "missingall", + "mktemp", "msbuild", "MSYS", "nameof", @@ -52,6 +52,7 @@ "reportgenerator", "reporttypes", "Shouldly", + "skipautoprops", "subfolders", "targetargs", "targetdir",