Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow value types to be cached and injected as dependencies #6159

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Allow value types to be cached and injected as dependencies #6159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a0d4d5
Rework dependencies to allow value types
smoogipoo Jan 29, 2024
5ee40fd
Use exception helper
smoogipoo Jan 29, 2024
8ff8f8c
Properly hint for notnull type requirements
smoogipoo Jan 29, 2024
fd23266
Merge branch 'master' into bdl-structs
smoogipoo Feb 7, 2024
4082fe3
Update tests around invalid usages
smoogipoo Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 16 additions & 23 deletions osu.Framework.Tests/Dependencies/Reflection/CachedAttributeTest.cs
View file
Open in desktop
Expand Up @@ -51,11 +51,13 @@ public void TestCacheTypeOverrideParentCache()
}

[Test]
public void TestAttemptToCacheStruct()
public void TestCacheStruct()
{
var provider = new Provider4();

Assert.Throws<ArgumentException>(() => DependencyActivator.MergeDependencies(provider, new DependencyContainer()));
var dependencies = DependencyActivator.MergeDependencies(provider, new DependencyContainer());

Assert.IsNotNull(dependencies.Get<int?>());
}

[Test]
Expand Down Expand Up @@ -136,7 +138,9 @@ public void TestCacheStructAsInterface()
{
var provider = new Provider12();

Assert.Throws<ArgumentException>(() => DependencyActivator.MergeDependencies(provider, new DependencyContainer()));
var dependencies = DependencyActivator.MergeDependencies(provider, new DependencyContainer());

Assert.IsNotNull(dependencies.Get<IProvidedInterface1>());
}

/// <summary>
Expand All @@ -149,29 +153,22 @@ public void TestCacheStructInternal()

var dependencies = DependencyActivator.MergeDependencies(provider, new DependencyContainer());

Assert.AreEqual(provider.CachedObject.Value, dependencies.GetValue<CachedStructProvider.Struct>().Value);
Assert.AreEqual(provider.CachedObject.Value, dependencies.Get<CachedStructProvider.Struct>().Value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is pretty much superseded by TestCacheStruct it looks like. Probably can be removed.

}

[Test]
public void TestGetValueNullInternal()
public void TestGetNullInternal()
{
Assert.AreEqual(default(int), new DependencyContainer().GetValue<int>());
Assert.AreEqual(default(int), new DependencyContainer().Get<int>());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates a duality in semantics in that a "required value-type dependency" does not practically exist. If you forget to cache one, anyone that tries to resolve it will just get default back.

If the argument here is that that is fine and matches the general type semantics of C# in general, that's fine, but should probably be documented better.

}

/// <summary>
/// Test caching a nullable, where the providing type is within the osu.Framework assembly.
/// </summary>
[TestCase(null)]
[TestCase(10)]
public void TestCacheNullableInternal(int? testValue)
[Test]
public void TestAttemptCacheNullableInternal()
{
var provider = new CachedNullableProvider();
provider.SetValue(null);

provider.SetValue(testValue);

var dependencies = DependencyActivator.MergeDependencies(provider, new DependencyContainer());

Assert.AreEqual(testValue, dependencies.GetValue<int?>());
Assert.Throws<NullDependencyException>(() => DependencyActivator.MergeDependencies(provider, new DependencyContainer()));
}

[Test]
Expand Down Expand Up @@ -460,9 +457,7 @@ private class Provider22 : IDependencyInjectionCandidate
public object Provided1
{
get => null;
set
{
}
set { }
}
}

Expand All @@ -471,9 +466,7 @@ private class Provider23 : IDependencyInjectionCandidate
[Cached]
public object Provided1
{
set
{
}
set { }
}
}

Expand Down
151 changes: 66 additions & 85 deletions osu.Framework.Tests/Dependencies/Reflection/DependencyContainerTest.cs
View file
Open in desktop
Expand Up @@ -6,7 +6,6 @@
using System;
using System.Diagnostics.CodeAnalysis;
using System.Threading;
using JetBrains.Annotations;
using NUnit.Framework;
using osu.Framework.Allocation;
using osu.Framework.Testing.Dependencies;
Expand Down Expand Up @@ -137,22 +136,22 @@ public void TestMultipleCacheFails()
Assert.Throws<TypeAlreadyCachedException>(() => dependencies.Cache(testObject2));
}

/// <summary>
/// Special case because "where T : class" also allows interfaces.
/// </summary>
[Test]
public void TestAttemptCacheStruct()
public void TestCacheStruct()
{
Assert.Throws<ArgumentException>(() => new DependencyContainer().Cache(new BaseStructObject()));
var dependencies = new DependencyContainer();
dependencies.Cache(new BaseStructObject());

Assert.IsNotNull(dependencies.Get<BaseStructObject?>());
}

/// <summary>
/// Special case because "where T : class" also allows interfaces.
/// </summary>
[Test]
public void TestAttemptCacheAsStruct()
public void TestCacheAsStruct()
{
Assert.Throws<ArgumentException>(() => new DependencyContainer().CacheAs<IBaseInterface>(new BaseStructObject()));
var dependencies = new DependencyContainer();
dependencies.CacheAs<IBaseInterface>(new BaseStructObject());

Assert.IsNotNull(dependencies.Get<IBaseInterface>());
}

/// <summary>
Expand All @@ -166,9 +165,9 @@ public void TestCacheCancellationToken()

var dependencies = new DependencyContainer();

Assert.DoesNotThrow(() => dependencies.CacheValue(token));
Assert.DoesNotThrow(() => dependencies.Cache(token));

var retrieved = dependencies.GetValue<CancellationToken>();
var retrieved = dependencies.Get<CancellationToken>();

source.Cancel();

Expand Down Expand Up @@ -221,33 +220,20 @@ public void TestReceiveStructInternal()
Assert.AreEqual(testObject.CachedObject.Value, receiver.TestObject.Value);
}

[TestCase(null)]
[TestCase(10)]
public void TestResolveNullableInternal(int? testValue)
{
var receiver = new Receiver11();

var testObject = new CachedNullableProvider();
testObject.SetValue(testValue);

var dependencies = DependencyActivator.MergeDependencies(testObject, new DependencyContainer());

dependencies.Inject(receiver);

Assert.AreEqual(testValue, receiver.TestObject);
}

[Test]
public void TestCacheNullInternal()
public void TestAttemptCacheNullInternal()
{
Assert.DoesNotThrow(() => new DependencyContainer().CacheValue(null));
Assert.DoesNotThrow(() => new DependencyContainer().CacheValueAs<object>(null));
Assert.Throws<ArgumentNullException>(() => new DependencyContainer().Cache(null!));
Assert.Throws<ArgumentNullException>(() => new DependencyContainer().CacheAs<object>(null!));
}

[Test]
public void TestResolveStructWithoutNullPermits()
{
Assert.Throws<DependencyNotRegisteredException>(() => new DependencyContainer().Inject(new Receiver12()));
var receiver = new Receiver12();

Assert.DoesNotThrow(() => new DependencyContainer().Inject(receiver));
Assert.AreEqual(0, receiver.TestObject);
}

[Test]
Expand All @@ -265,61 +251,41 @@ public void TestCacheAsNullableInternal()
int? testObject = 5;

var dependencies = new DependencyContainer();
dependencies.CacheValueAs(testObject);
dependencies.CacheAs(testObject);

Assert.AreEqual(testObject, dependencies.GetValue<int>());
Assert.AreEqual(testObject, dependencies.GetValue<int?>());
Assert.AreEqual(testObject, dependencies.Get<int>());
Assert.AreEqual(testObject, dependencies.Get<int?>());
}

[Test]
public void TestCacheWithDependencyInfo()
[TestCase(null, null)]
[TestCase("name", null)]
[TestCase(null, typeof(object))]
[TestCase("name", typeof(object))]
public void TestCacheWithDependencyInfo(string name, Type parent)
{
var cases = new[]
{
default,
new CacheInfo("name"),
new CacheInfo(parent: typeof(object)),
new CacheInfo("name", typeof(object))
};
CacheInfo info = new CacheInfo(name, parent);

var dependencies = new DependencyContainer();
dependencies.CacheAs(1, info);

for (int i = 0; i < cases.Length; i++)
dependencies.CacheValueAs(i, cases[i]);

Assert.Multiple(() =>
{
for (int i = 0; i < cases.Length; i++)
Assert.AreEqual(i, dependencies.GetValue<int>(cases[i]));
});
Assert.AreEqual(1, dependencies.Get<int>(info));
}

[Test]
public void TestDependenciesOverrideParent()
[TestCase(null, null)]
[TestCase("name", null)]
[TestCase(null, typeof(object))]
[TestCase("name", typeof(object))]
public void TestDependenciesOverrideParent(string name, Type parent)
{
var cases = new[]
{
default,
new CacheInfo("name"),
new CacheInfo(parent: typeof(object)),
new CacheInfo("name", typeof(object))
};
CacheInfo info = new CacheInfo(name, parent);

var dependencies = new DependencyContainer();

for (int i = 0; i < cases.Length; i++)
dependencies.CacheValueAs(i, cases[i]);
dependencies.CacheAs(1, info);

dependencies = new DependencyContainer(dependencies);
dependencies.CacheAs(2, info);

for (int i = 0; i < cases.Length; i++)
dependencies.CacheValueAs(cases.Length + i, cases[i]);

Assert.Multiple(() =>
{
for (int i = 0; i < cases.Length; i++)
Assert.AreEqual(cases.Length + i, dependencies.GetValue<int>(cases[i]));
});
Assert.Multiple(() => { Assert.AreEqual(2, dependencies.Get<int>(info)); });
}

[Test]
Expand All @@ -336,8 +302,31 @@ public void TestResolveWithNullableReferenceTypes()
Assert.DoesNotThrow(() => dependencies.Inject(receiver));
}

[Test]
public void TestResolveDefaultStruct()
{
Assert.That(new DependencyContainer().Get<CancellationToken>(), Is.EqualTo(default(CancellationToken)));
}

[Test]
public void TestResolveNullStruct()
{
Assert.That(new DependencyContainer().Get<CancellationToken?>(), Is.Null);
}

[Test]
public void TestModifyBoxedStruct()
{
var dependencies = new DependencyContainer();
dependencies.CacheAs<IBaseInterface>(new BaseStructObject { TestValue = 1 });
dependencies.Get<IBaseInterface>().TestValue = 2;

Assert.That(dependencies.Get<IBaseInterface>().TestValue, Is.EqualTo(2));
}

private interface IBaseInterface
{
int TestValue { get; set; }
}

private class BaseObject
Expand All @@ -347,6 +336,7 @@ private class BaseObject

private struct BaseStructObject : IBaseInterface
{
public int TestValue { get; set; }
}

private class DerivedObject : BaseObject
Expand Down Expand Up @@ -429,21 +419,12 @@ private class Receiver10 : IDependencyInjectionCandidate
private void load(CachedStructProvider.Struct testObject) => TestObject = testObject;
}

private class Receiver11 : IDependencyInjectionCandidate
{
public int? TestObject { get; private set; }

[BackgroundDependencyLoader]
private void load(int? testObject) => TestObject = testObject;
}

private class Receiver12 : IDependencyInjectionCandidate
{
[UsedImplicitly] // param used implicitly
public int TestObject { get; private set; } = 1;

[BackgroundDependencyLoader]
private void load(int testObject)
{
}
private void load(int testObject) => TestObject = testObject;
}

private class Receiver13 : IDependencyInjectionCandidate
Expand Down