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.

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

Conversation

smoogipoo
Copy link
Contributor

Resolves #6157

Consider this an RFC.

The way structs are handled right now is a mess. If I (as the person who wrote all of this) can't understand how they're handled, then I don't think anyone can. And my confusion is the result of two conflicting thought processes:

  • CancellationToken is somehow a special type. How is it so?
  • The way I view structs is as always being initialised, whether in a "zeroed" state or otherwise (unless they're nullable).

Every value type is a special type that can only be cached by osu!framework. CancellationToken is doubly special because it sometimes arrives in load(), and sometimes doesn't, depending on whether you're in an async-load context. So the only proper way to use BDL with CancellationToken is to make it nullable.

In my opinion, if you want to cache and resolve a value-type dependency and accept whatever footguns that comes with (by-val copies, you can't cache multiple ints, etc), then you should be able to do so. But the framework should retain predictable semantics, which for me means matching the C# spec - ValueType = default! is a zero-initialised struct. This is a simplification of semantics to me.

New semantics:

Get<int>() => 0;               // Allowed
Get<int?>() => null;           // Allowed
Cache<int>(0);                 // Allowed
Cache<int>(null);              // Disallowed
Cache<int?>(0);                // Warning in NRT contexts
CacheAs<IInterface>(MyStruct); // Allowed - boxed.
Cache<object>(null);           // Exception
Cache<object?>(null);          // Exception + warning in NRT contexts

Is this something I foresee being used? No. It's just for the sake of clarifying semantics.

@smoogipoo smoogipoo changed the title Rework dependencies to allow value types Allow value types to be cached and injected as dependencies Jan 29, 2024
smoogipoo and others added 2 commits January 29, 2024 18:16
Co-authored-by: Berkan Diler <berkan.diler1@ingka.ikea.com>
@bdach
Copy link
Collaborator

bdach commented Feb 7, 2024

Why are tests failing here? I'd expect failures in the sourcegen path until a package bump or something, but the reflection path is failing too?

@smoogipoo
Copy link
Contributor Author

Sourcegen won't need updates; I just forgot to update a test. Will get to it.

@bdach
Copy link
Collaborator

bdach commented Feb 10, 2024

Reading the OP:

Cache<int?>(0);                // Warning in NRT contexts

Why would this warn? Looks like perfectly legal code to me...

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

rather difficult diff to get through


/// <summary>
/// Caches an instance of a type as a type of <paramref name="type"/>. This instance will be returned each time you <see cref="Get(Type)"/>.
/// Caches an instance of a type as its most derived type. This instance will be returned each time you <see cref="Get{T}()"/>.
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're editing these, I'd probably clarify further to say

Suggested change
/// Caches an instance of a type as its most derived type. This instance will be returned each time you <see cref="Get{T}()"/>.
/// Caches an instance of a type as its most derived type as returned by <see cref="object.GetType"/>. This instance will be returned each time you <see cref="Get{T}()"/>.

(goes for all other overloads of this)

Comment on lines +46 to +48
object? Get(Type type);

object? Get(Type type, CacheInfo info);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are these for? It's not super clear from reading diff but seeing object? return type on public API doesn't feel great...


public T Get<T>(CacheInfo info)
{
TryGet(out T value, info);
Copy link
Collaborator

Choose a reason for hiding this comment

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

A TryGet() call with no return value check? What's going on here? Isn't this gonna end up being wrong for reference types?

@@ -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.

{
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.

Comment on lines +148 to +151
var receiver = new Receiver14();

Assert.DoesNotThrow(() => new DependencyContainer().Inject(receiver));
Assert.AreEqual(0, receiver.Obj);
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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are there changes in this test but not the reflection variant of it?

@smoogipoo
Copy link
Contributor Author

Why would this warn? Looks like perfectly legal code to me...

Because caching null is not allowed 1 2, therefore attempting to cache a may-be-null value warns about that.

@smoogipoo
Copy link
Contributor Author

Almost thinking all of these tests need to be rewritten...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CancellationToken should probably always be present in BDL calls
3 participants