Re-enable RegsiterTypes filtering; better message for RegisterType issues #1389

tillig · 2023-08-04T18:58:19Z

Fixes RegisterTypes will attempt to register abstract types (undocumented breaking change in v6.5.0) #1388 - puts back the type filtering that was on RegisterTypes so if "invalid" types (types that generally can't be created via reflection - interfaces, abstract classes, etc.) get passed in, they will be ignored and not registered.
Refactored some of the methods in the scanning namespace so they are in more appropriately named classes and have better/more descriptive names themselves. That way it's easier to see what they're for and who is using them.
Adds more up-front error handling for RegisterType(Type t) and RegisterType<T>() calls so if someone tries to use a type in there that generally can't be created via reflection, an ArgumentException will come up and explain the issue rather than waiting for the container build to occur and hitting a "no constructors found" exception.
Centralized the "check" for what may be generally registerable so it can be shared between assembly scanning and ACTNARS.
Housekeeping:
- Fix parameterised => parameterized. Given the fallback language on the assemblies is en-US it seemed like fixing up docs and internal naming to also be en-US was reasonable. Also allows spell checkers set to that dictionary to find fewer things.
- Fix ctxt => context. Another spell checker issue. Community practice generally either uses ctx or context, with most .NET code in the framework being a fully spelled-out context. Updated references to go with the fully spelled-out version.

I'll add some comments to the important bits for extra attention.

- `ctxt` => `context` everywhere. Community generally uses either `ctx` or `context`, and it didn't make sense to abbreviate. Most .NET examples spell it out. - `parameterised` => `parameterized`. Our default language attribute is en-US so it makes sense to make it common. We have it both ways in here. - Removed unused "using" statements. - Removed unnecessary FxCop suppressions.

- Separate methods into more appropriate owning classes. - Update method names to be more descriptive. - Add documentation to explain non-obvious logic.

tillig · 2023-08-04T19:02:50Z

src/Autofac/Builder/RegistrationBuilder.cs

@@ -61,6 +62,12 @@ public static class RegistrationBuilder
    public static IRegistrationBuilder<TImplementer, ConcreteReflectionActivatorData, SingleRegistrationStyle> ForType<TImplementer>()
        where TImplementer : notnull
    {
+        // Open generics can't be generic type parameters so we don't have to check for that here.


RegistrationBuilder.ForType (both overloads) is where all the RegisterType<T> and RegisterType(Type t) calls come through. It also gets used a few places directly like in ACTNARS and assembly/type scanning.

Adding the check here means we can catch any issues where someone is trying to do something that probably won't work with reflection activation and provide a more proactive error.

It does mean, though, that we may do the same checks twice on a given type. For example, while scanning the set of types to register, we'll filter off things that don't match this criteria; then when the registration gets created it will be checked again here. Ideally it means we won't get any of these errors during any sort of scanning, but it may have container build performance impacts due to the double-check.

tillig · 2023-08-04T19:03:27Z

src/Autofac/Builder/RegistrationBuilderResources.resx

+  <data name="OnlyRegisterableTypesAllowed" xml:space="preserve">
+    <value>The type '{0}' is not concrete. Only concrete types (no interfaces, abstract classes, open generics, etc.) can be registered by type.</value>


This is the exception message that will be thrown if you register types that generally can't be activated by reflection.

tillig · 2023-08-04T19:04:05Z

src/Autofac/Features/ResolveAnything/AnyConcreteTypeNotAlreadyRegisteredSource.cs

-        if (!serviceType.IsClass ||
-            serviceType.IsSubclassOf(typeof(Delegate)) ||
-            serviceType.IsAbstract ||
+        if (!serviceType.MayAllowReflectionActivation(allowCompilerGenerated: true) ||


ACTNARS now uses the shared logic for filtering.

tillig · 2023-08-04T19:06:06Z

src/Autofac/Features/Scanning/AssemblyExtensions.cs

@@ -0,0 +1,53 @@
+// Copyright (c) Autofac Project. All rights reserved.


This all came out of the OpenGenericScanningRegistrationExtensions and ScanningRegistrationExtensions classes.

tillig · 2023-08-04T19:06:36Z

src/Autofac/Features/Scanning/TypeExtensions.cs

@@ -0,0 +1,168 @@
+// Copyright (c) Autofac Project. All rights reserved.


This all came out of the OpenGenericScanningRegistrationExtensions and ScanningRegistrationExtensions classes.

codecov · 2023-08-04T19:07:09Z

Codecov Report

Patch coverage: 92.68% and project coverage change: -0.02% ⚠️

Comparison is base (dc4e4cd) 78.49% compared to head (36da90d) 78.47%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1389      +/-   ##
===========================================
- Coverage    78.49%   78.47%   -0.02%     
===========================================
  Files          199      201       +2     
  Lines         5706     5715       +9     
  Branches      1166     1169       +3     
===========================================
+ Hits          4479     4485       +6     
- Misses         714      716       +2     
- Partials       513      514       +1

Files Changed	Coverage Δ
src/Autofac/Core/InternalReflectionCaches.cs	`100.00% <ø> (ø)`
src/Autofac/Core/Resolving/ResolvePipeline.cs	`80.00% <0.00%> (ø)`
src/Autofac/Core/UniqueService.cs	`77.77% <0.00%> (ø)`
.../OpenGenerics/OpenGenericRegistrationExtensions.cs	`68.18% <ø> (ø)`
...fac/Features/OwnedInstances/InstancePerOwnedKey.cs	`60.00% <0.00%> (ø)`
src/Autofac/TypeExtensions.cs	`35.89% <0.00%> (ø)`
src/Autofac/Util/AssemblyExtensions.cs	`40.00% <ø> (-26.67%)`	⬇️
...degen/Autofac.CodeGen/DelegateRegisterGenerator.cs	`97.63% <66.66%> (ø)`
src/Autofac/Builder/RegistrationBuilder.cs	`82.05% <66.66%> (-1.29%)`	⬇️
...eatures/Scanning/ScanningRegistrationExtensions.cs	`63.07% <66.66%> (-5.35%)`	⬇️
... and 29 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

tillig · 2023-08-04T19:07:15Z

src/Autofac/Features/Scanning/OpenGenericScanningRegistrationExtensions.cs

@@ -13,7 +13,7 @@ namespace Autofac.Features.Scanning;
 /// <summary>
 /// Helper methods to assist in scanning registration.
 /// </summary>
-internal static partial class ScanningRegistrationExtensions
+internal static class OpenGenericScanningRegistrationExtensions


Having this just One Big Class was causing some of the confusion with what was consuming what.

tillig · 2023-08-04T19:08:04Z

src/Autofac/Util/AssemblyExtensions.cs

@@ -40,22 +40,4 @@ public static IEnumerable<Type> GetLoadableTypes(this Assembly assembly)
            return ex.Types.Where(t => t is not null)!;
        }
    }
-


This is only used in the scanning namespace, so I moved it there.

tillig · 2023-08-04T19:09:52Z

src/Autofac/Util/InternalTypeExtensions.cs

+    // If people want only public types, a LINQ Where clause can be used.
+    //
+    // Run IsCompilerGenerated check last due to perf. See AssemblyScanningPerformanceTests.MeasurePerformance.
+    internal static bool MayAllowReflectionActivation(this Type? type, bool allowCompilerGenerated = false) => type != null && type.IsClass && !type.IsAbstract && !type.IsDelegate() && (allowCompilerGenerated || !type.IsCompilerGenerated());


This gets used a lot - ACTNARS, RegisterTypes, RegistrationBuilder.FromType, assembly scanning, open generic assembly scanning. The permutations are challenging because sometimes (open generic scanning) you want open generics, but usually you don't; sometimes (ACTNARS) you want compiler generated, but usually you don't.

But at least this gives us a single spot for doing these checks and ensuring the proper order.

tillig · 2023-08-04T19:10:27Z

test/Autofac.Specification.Test/Registration/TypeRegistrationTests.cs

@@ -96,6 +106,26 @@ public void RegisterTypeAsUnsupportedService()
        Assert.Throws<ArgumentException>(() => builder.Build());
    }

+    [Fact]


Added some tests for RegisterType and RegisterTypes, both.

alistairjevans

For what its worth, this change also improves the error generated if a value type is registered. Before this change, this test:

[Fact]
public void ValueTypeActivation()
{
    var cb = new ContainerBuilder();
    cb.RegisterType<A>().As<IA>();
    cb.RegisterType<MyValueType>();
    var c = cb.Build();
    c.Resolve<MyValueType>();
}

private struct MyValueType
{
    public MyValueType(IA a)
    {
        A = a;
    }

    public IA A { get; }
}

Produces a strange and confusing error:

 Message: 
   System.ArgumentException : Expression of type 'Autofac.Specification.Test.Registration.TypeRegistrationTests+MyValueType' cannot be used for return type 'System.Object'

 Stack Trace: 
   Expression.ValidateLambdaArgs(Type delegateType, Expression& body, ReadOnlyCollection`1 parameters, String paramName)
   Expression.Lambda[TDelegate](Expression body, String name, Boolean tailCall, IEnumerable`1 parameters)
   Expression.Lambda[TDelegate](Expression body, Boolean tailCall, IEnumerable`1 parameters)
   Expression.Lambda[TDelegate](Expression body, ParameterExpression[] parameters)
   ConstructorBinder.GetConstructorInvoker(ConstructorInfo constructorInfo) line 165

(I believe we generally don't support activating value types, and this isn't just another error.)

Now the error will get raised at RegisterType.

alistairjevans · 2023-08-10T07:04:51Z

src/Autofac/Util/InternalTypeExtensions.cs

+    // If people want only public types, a LINQ Where clause can be used.
+    //
+    // Run IsCompilerGenerated check last due to perf. See AssemblyScanningPerformanceTests.MeasurePerformance.
+    internal static bool MayAllowReflectionActivation(this Type? type, bool allowCompilerGenerated = false) => type != null && type.IsClass && !type.IsAbstract && !type.IsDelegate() && (allowCompilerGenerated || !type.IsCompilerGenerated());


Propose is not null rather than != null; Type comparison can be overridden.

Updated all of our != null to is not null since it seems like that'd apply everywhere.

alistairjevans · 2023-08-10T07:14:07Z

test/Autofac.Specification.Test/Registration/TypeRegistrationTests.cs

+        Assert.True(container.TryResolve(typeof(MyOpenGeneric<int>), out object _));
+        Assert.False(container.TryResolve(typeof(IMyService), out _));
+        Assert.False(container.TryResolve(typeof(MyDelegate), out _));
+        Assert.False(container.TryResolve(typeof(MyAbstractClass), out _));


Suggest adding a test case for value types?

alistairjevans · 2023-08-10T07:14:35Z

test/Autofac.Specification.Test/Registration/TypeRegistrationTests.cs

+    [InlineData(typeof(MyDelegate))]
+    [InlineData(typeof(IMyService))]
+    [InlineData(typeof(MyAbstractClass))]
+    [InlineData(typeof(MyOpenGeneric<>))]


As below, suggest a test case for value types.

alistairjevans

🐓

tillig added 9 commits August 4, 2023 07:01

RegisterType tests for filtering.

2d3b367

Filter out null types from the list when scanning.

903b2f1

Housekeeping: parameterised => parameterized.

7996d57

Share "concrete type" filter between assembly and type scanning.

ea6a125

Refactor and update naming for scanning utilities.

1fde081

- Separate methods into more appropriate owning classes. - Update method names to be more descriptive. - Add documentation to explain non-obvious logic.

Spelling word.

80d1978

Centralize check for potential to do reflection activation.

aab6734

Better message for invalid RegisterType calls.

5e0b472

tillig commented Aug 4, 2023

View reviewed changes

alistairjevans requested changes Aug 10, 2023

View reviewed changes

tillig added 2 commits August 10, 2023 10:51

Tests for value type registrations.

13b3fee

Convert all '!= null' to 'is not null'

36da90d

alistairjevans approved these changes Aug 11, 2023

View reviewed changes

tillig merged commit 17e22eb into develop Aug 11, 2023

tillig deleted the feature/registertypes-filtering branch August 11, 2023 13:47

		<data name="OnlyRegisterableTypesAllowed" xml:space="preserve">
		<value>The type '{0}' is not concrete. Only concrete types (no interfaces, abstract classes, open generics, etc.) can be registered by type.</value>

		@@ -0,0 +1,53 @@
		// Copyright (c) Autofac Project. All rights reserved.

		@@ -0,0 +1,168 @@
		// Copyright (c) Autofac Project. All rights reserved.

Uh oh!

Re-enable RegsiterTypes filtering; better message for RegisterType issues #1389

Re-enable RegsiterTypes filtering; better message for RegisterType issues #1389

Uh oh!

Conversation

tillig commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alistairjevans left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alistairjevans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tillig commented Aug 4, 2023 •

edited

Loading

codecov bot commented Aug 4, 2023 •

edited

Loading

alistairjevans left a comment •

edited

Loading