Skip to content
New issue

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

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

Already on GitHub? # to your account

Open generic scanning extensions #1246

Merged

Conversation

RaymondHuy
Copy link
Member

Add remaining methods as discussed in #1232.
Some notes:

@codecov
Copy link

codecov bot commented Jan 23, 2021

Codecov Report

Merging #1246 (1687687) into develop (0afd90f) will increase coverage by 0.19%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1246      +/-   ##
===========================================
+ Coverage    76.14%   76.34%   +0.19%     
===========================================
  Files          185      185              
  Lines         4821     4870      +49     
  Branches       994     1004      +10     
===========================================
+ Hits          3671     3718      +47     
- Misses         676      679       +3     
+ Partials       474      473       -1     
Impacted Files Coverage Δ
src/Autofac/Util/TypeExtensions.cs 96.15% <ø> (-0.04%) ⬇️
...nning/OpenGenericScanningRegistrationExtensions.cs 90.19% <100.00%> (+5.82%) ⬆️
...Autofac/RegistrationExtensions.AssemblyScanning.cs 50.00% <100.00%> (+3.12%) ⬆️
...istrationExtensions.OpenGenericAssemblyScanning.cs 68.18% <100.00%> (+7.07%) ⬆️
...apters/LightweightAdapterRegistrationExtensions.cs 83.33% <0.00%> (-16.67%) ⬇️
.../OpenGenerics/OpenGenericRegistrationExtensions.cs 54.54% <0.00%> (-13.64%) ⬇️
src/Autofac/Core/Resolving/ResolveOperation.cs 84.61% <0.00%> (+1.09%) ⬆️
src/Autofac/RegistrationExtensions.Decorators.cs 63.75% <0.00%> (+1.25%) ⬆️
src/Autofac/RegistrationExtensions.Adapter.cs 20.00% <0.00%> (+6.66%) ⬆️
...eatures/AttributeFilters/RegistrationExtensions.cs 84.61% <0.00%> (+7.69%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0afd90f...3282ad8. Read the comment docs.

@alistairjevans
Copy link
Member

What is the motivation behind the name AsOpenTypesOf? Are you able to elaborate on it's purpose? What is an open type of an already open generic?

Regarding 'AssignableTo', are you simply saying that it's not possible because one generic type is not assignable to another? I think in terms of the strict implementation of the runtime's AssignableFrom, that's true, but I believe there are other places in the codebase where we say 'if I were to create a closed type of X, is that assignable to Y, closed with the same type'?. I'm not saying we absolutely need it for open generic scanning, but I'm curious if it's still possible. It may be that because we can't know about all the type constraints at registration time, we can't implement it, and if so, that's OK by me.

@alistairjevans
Copy link
Member

Unrelated; @alsami, did we stabilise the code coverage metrics already? There's a big drop in this PR for what isn't really a huge quantity of code.

@alsami
Copy link
Member

alsami commented Jan 23, 2021

Unrelated; @alsami, did we stabilise the code coverage metrics already? There's a big drop in this PR for what isn't really a huge quantity of code.

No, still have to look into it. Will do so soon. Really don't know why it does it again, maybe we will need to downgrade to the pre-release again. Will figure it out.

@RaymondHuy
Copy link
Member Author

Hi @alistairjevans AsOpenTypesOf is equivalent with AsClosedTypeOf term (when scanning assembly) but used for open-generic scanning because we don't register closed type in this context. For example:

// RedoOpenGenericCommand<T> is an open type of ICommand<T>
public class RedoOpenGenericCommand<T> : ICommand<T> {}

var cb = new ContainerBuilder();
cb.RegisterAssemblyOpenGenericTypes(typeof(ICommand<>).GetTypeInfo().Assembly)
    .AsOpenTypesOf(typeof(ICommand<>));
var c = cb.Build();

Assert.NotNull(c.Resolve<RedoOpenGenericCommand<int>>());

@alistairjevans
Copy link
Member

@RaymondHuy, I think it's just the name that of the method that may need a rethink. AsClosedTypesOf requires you to provide an open type as a parameter, but AsOpenTypesOf doesn't require you to provide a closed type? Seems a bit...unexpected.
What happens if I was to pass a closed type into that method, e.g.:

// RedoOpenGenericCommand<T> is an open type of ICommand<T>
public class RedoOpenGenericCommand<T> : ICommand<T> {}

var cb = new ContainerBuilder();
cb.RegisterAssemblyOpenGenericTypes(typeof(ICommand<>).GetTypeInfo().Assembly)
    .AsOpenTypesOf(typeof(ICommand<int>));
var c = cb.Build();

I just feel like the user expectation of the name AsOpenTypesOf would expect to pass a closed type.

Haven't you kind of implicitly created an open generic AssignableTo method here?

@RaymondHuy
Copy link
Member Author

@alistairjevans yah, maybe somehow I have self implemented the AssignableTo method but I don't know. Therefore how about the AsClosedTypeOf method, do you think it shouldn't exist when scanning Open Generic types ?

@alistairjevans
Copy link
Member

Yeah, if you call RegisterAssemblyOpenGenericTypes, you'd implicitly expect to not be able to register closed types. Closed Types != Open Generic, so it wouldn't make much sense. Can we constrain AsClosedTypesOf appropriately?

@RaymondHuy
Copy link
Member Author

@alistairjevans Yes, currently we can't use the method AsClosedTypesOf if it is inside the Open Generic Scanning context.
image

@alistairjevans
Copy link
Member

Good stuff; did you want to rename your AsOpenTypesOf, and see if it meets the semantics of AssignableTo? If it does, that feels like a better name.

@RaymondHuy
Copy link
Member Author

Thanks @alistairjevans I've updated my PR.

@alistairjevans
Copy link
Member

Thanks @RaymondHuy; can I suggest we add some tests for passing invalid types into AssignableTo? Mostly thinking of closed types, or types that aren't in the right inheritance chain or something.

@RaymondHuy
Copy link
Member Author

Hi @alistairjevans I've add testcases to my PR for passing closed types and also resolving services which are not assignable to the target open generic type. Can you check if it is satisfied ? feel free to suggest more testcases 😃

{
var cb = new ContainerBuilder();
cb.RegisterAssemblyOpenGenericTypes(typeof(ICommand<>).GetTypeInfo().Assembly)
.AssignableTo(closedType);
Copy link
Member

Choose a reason for hiding this comment

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

It feels like passing a closed type into AssignableTo for Open Generics is enough of an input mistake that we should throw an ArgumentException if someone tries it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@alistairjevans when using AssignableTo in registering assembly closed type, it doesn't throw exception when the argument is open generic type. Therefore should we keep this behavior in this case or we will modify it too ?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good point. We should retain the same behaviour for consistency, and I don't think it's worth a breaking change in the existing AssignableTo.

Copy link
Member

@alistairjevans alistairjevans left a comment

Choose a reason for hiding this comment

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

Good to go! Thanks @RaymondHuy for all the work!

@alistairjevans alistairjevans merged commit 3ca27f8 into autofac:develop Feb 12, 2021
@RaymondHuy
Copy link
Member Author

Thanks @alistairjevans, btw I wonder there are any plans for adding SourceGenerator to Autofac ? I can spend times on the implementation if we can finalize the design or something like that.

@alistairjevans
Copy link
Member

I've given it a little thought before when implementing pipelines, but didn't get too far with it (#1096 (comment)).

The problem we have is the extremely dynamic nature of Autofac's registration system, that allows components to late-resolve objects using Func<> or Lazy<>, combined with all the assembly scanning functionality. It gets really complicated to statically analyse, if not impossible. If we were to support source generators, there would most likely have to be extremely tight constraints on what you can use.

I'm still open to using it though if we can find a good way; if you want to raise an issue to discuss design that would be just fine.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants