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

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 1 commit
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions osu.Framework.Tests/Dependencies/Reflection/CachedAttributeTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
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,13 +153,13 @@ 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>
Expand All @@ -171,7 +175,7 @@ public void TestCacheNullableInternal(int? testValue)

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

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

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

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

Expand Down
112 changes: 54 additions & 58 deletions osu.Framework.Tests/Dependencies/Reflection/DependencyContainerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
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 @@ -238,16 +237,19 @@ public void TestResolveNullableInternal(int? testValue)
}

[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,60 +267,43 @@ 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);

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

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

Expand All @@ -336,6 +321,18 @@ 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);
}

private interface IBaseInterface
{
}
Expand Down Expand Up @@ -439,11 +436,10 @@ private class Receiver11 : IDependencyInjectionCandidate

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
38 changes: 37 additions & 1 deletion osu.Framework.Tests/Dependencies/Reflection/ResolvedAttributeTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#nullable disable

using System.Diagnostics.CodeAnalysis;
using System.Threading;
using NUnit.Framework;
using osu.Framework.Allocation;
using osu.Framework.Bindables;
Expand Down Expand Up @@ -160,7 +161,10 @@ public void TestResolveNullableInternal(int? testValue)
[Test]
public void TestResolveStructWithoutNullPermits()
{
Assert.Throws<DependencyNotRegisteredException>(() => new DependencyContainer().Inject(new Receiver14()));
var receiver = new Receiver14();

Assert.DoesNotThrow(() => new DependencyContainer().Inject(receiver));
Assert.AreEqual(0, receiver.Obj);
Comment on lines +148 to +151
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, this test is basically TestResolveDefaultStruct isn't it?

}

[Test]
Expand Down Expand Up @@ -212,6 +216,26 @@ public void TestResolveNonNullWithNullableReferenceTypes()
Assert.DoesNotThrow(() => createDependencies(new Bindable<int>(10)).Inject(receiver));
}

[Test]
public void TestResolveDefaultStruct()
{
var receiver = new Receiver19();

createDependencies().Inject(receiver);

Assert.That(receiver.Token, Is.EqualTo(default(CancellationToken)));
}

[Test]
public void TestResolveNullStruct()
{
var receiver = new Receiver20();

createDependencies().Inject(receiver);

Assert.That(receiver.Token, Is.Null);
}

private DependencyContainer createDependencies(params object[] toCache)
{
var dependencies = new DependencyContainer();
Expand Down Expand Up @@ -342,5 +366,17 @@ private class Receiver18 : IDependencyInjectionCandidate
[Resolved]
public Bindable<int> Obj { get; private set; } = null!;
}

private class Receiver19 : IDependencyInjectionCandidate
{
[Resolved]
public CancellationToken Token { get; private set; }
}

private class Receiver20 : IDependencyInjectionCandidate
{
[Resolved]
public CancellationToken? Token { get; private set; }
}
}
}
20 changes: 11 additions & 9 deletions osu.Framework.Tests/Dependencies/SourceGeneration/CachedAttributeTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,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 @@ -133,7 +135,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 @@ -146,13 +150,13 @@ public void TestCacheStructInternal()

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

Assert.AreEqual(provider.CachedObject.Value, dependencies.GetValue<PartialCachedStructProvider.Struct>().Value);
Assert.AreEqual(provider.CachedObject.Value, dependencies.Get<PartialCachedStructProvider.Struct>().Value);
}

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

/// <summary>
Expand All @@ -168,7 +172,7 @@ public void TestCacheNullableInternal(int? testValue)

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

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

[Test]
Expand Down Expand Up @@ -457,9 +461,7 @@ public object Provided1
{
get => null;
// ReSharper disable once ValueParameterNotUsed
set
{
}
set { }
}
}

Expand Down
Loading
Loading