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

Add open generic registration api and resolve feature #367

Merged
merged 8 commits into from
Dec 19, 2023

Conversation

DenisPimenov
Copy link
Contributor

Hi! I try to implement open generic feature #301. If you have time to look and tell me what can be changed or improved I can do it

@vercel
Copy link

vercel bot commented Apr 17, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/hadashia/vcontainer/42FG2b3uqBQcKJysb5xMwiRxGK2h
✅ Preview: https://vcontainer-git-fork-denispimenov-feature-open-generic-hadashia.vercel.app

@hadashiA
Copy link
Owner

hadashiA commented May 11, 2022

Thanks for working on it!
Sorry I haven't had time to look at the slightly bigger changes. I'll look at it sometime this month or next!

@DenisPimenov
Copy link
Contributor Author

Hey! Can you tell me if there is any progress on this issue?

@hadashiA
Copy link
Owner

@DenisPimenov Sorry for the delay in replying.

First of all, I would like to thank you. I think this implementation is not bad.

However, I would like to confirm two points.

#200 (comment)

Since this approach is only making instances of the closed generic types using reflection it means it won't work with IL2CPP, right? It will only work if code generation is used since it will then generate the code that actually uses those types so they won't be missing in runtime.

As pointed out here, in an IL2CPP environment, classes that are not statically referenced will probably be omitted.
Therefore, in reality, I think it may be less problematic to list the closed generic type registrations instead of using the open generic api.

If this problem cannot be solved, there is probably a problem.

Second, in some use cases like MessagePipe,
we want all ISubscriber in the project to be registered, even if they are not referenced anywhere.
For example, as far as I know, autofac has the API like this: https://autofac.readthedocs.io/en/latest/register/scanning.html

So, my I understand is that, there are two ways to support generic types.

  1. Dynamic generic type making at resolve time
  2. Assembly scanning at container initialization time

If either or both of these are to be implemented, API conflicts need to be avoided.

( However, both 1 and 2 probably have IL2CPP issues...

@vercel
Copy link

vercel bot commented Oct 31, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
vcontainer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 18, 2023 0:01am

@DenisPimenov
Copy link
Contributor Author

@hadashiA I will try different builds with il2cpp and report the results in a few days.

@slimshader
Copy link
Contributor

Is this still happening?

@hadashiA
Copy link
Owner

hadashiA commented Dec 9, 2023

Perhaps, the full generic sharing in Unity 2022.1 allows the following to work:

        var t1 = typeof(GenericClass<>).MakeGenericType(typeof(int));
        var i1 = (GenericClass<int>)Activator.CreateInstance(t1);

        var t2 = typeof(GenericClass<>).MakeGenericType(typeof(ClassA));
        var i2 = (GenericClass<ClassA>)Activator.CreateInstance(t2);

        var t3 = typeof(GenericClass<>).MakeGenericType(typeof(StructA));
        var i3 = (GenericClass<StructA>)Activator.CreateInstance(t3);

        var t4 = typeof(GenericStruct<>).MakeGenericType(typeof(int));
        var i4 = (GenericStruct<int>)Activator.CreateInstance(t4);

        var t5 = typeof(GenericStruct<>).MakeGenericType(typeof(ClassA));
        var i5 = (GenericStruct<ClassA>)Activator.CreateInstance(t5);

        var t6 = typeof(GenericStruct<>).MakeGenericType(typeof(StructA));
        var i6 = (GenericStruct<StructA>)Activator.CreateInstance(t6);

So this PR should probably work fine, as long as it's Unity 2022.1 or later.

I'd like to refine the API a bit more and merge this functionality.

# Conflicts:
#	VContainer/Assets/VContainer/Tests/Fixtures.cs
@DenisPimenov
Copy link
Contributor Author

Hi. It has been a long time since opening PR. I have synchronized with master, but the tests are not passing. I will fix them.

…as AsSelf and AsImplementedInterfaces

Optimize type cache calls
@DenisPimenov
Copy link
Contributor Author

Hi! I fixed all the tests and fixed singleton resolution when the service is configured with multiple implementation types.

@hadashiA
Copy link
Owner

@DenisPimenov
Oh. Sorry. I was going to merge, can you just fix the conflicts?

@DenisPimenov
Copy link
Contributor Author

@hadashiA I have synchronized with master, but it has compilation errors

@DenisPimenov
Copy link
Contributor Author

Can you fix them?

registration = null;
return false;
}

public bool Exists(Type type) => hashTable.TryGet(type, out _);
Copy link

Choose a reason for hiding this comment

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

Suggested change
public bool Exists(Type type) => hashTable.TryGet(type, out _);
public bool Exists(Type type)
{
if (type.IsConstructedGenericType)
{
type = RuntimeTypeCache.OpenGenericTypeOf(type);
}
return hashTable.TryGet(type, out _);
}

I tested out your implementation in my project and ran into an issue here.
Exists doesn't behave correctly and returns false when checking registered open generics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I forgot the scopes tests and missed that error. I'll add them and your fix as soon as the compilation errors in the master branch are fixed

Choose a reason for hiding this comment

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

When I resolve IEnumerable<OpenGenericInterface> or IReadOnlyList<OpenGenericInterface>, I get an empty list. I expect to return me a list for all classes that are registered as this open generic interface.

Additionally in regard to that, I suggest to add tests for resolving multiple instances of an interface.

@hadashiA
Copy link
Owner

I have synchronized with master, but it has compilation errors

Oh . . I see.
This is another issue, so I'll check over here after the merge.

@hadashiA hadashiA merged commit 2f8e5d4 into hadashiA:master Dec 19, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants