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

Unexpected anonymous class registration on a C# closure #108

Closed
NeoXtreem opened this issue Nov 5, 2019 · 14 comments
Closed

Unexpected anonymous class registration on a C# closure #108

NeoXtreem opened this issue Nov 5, 2019 · 14 comments

Comments

@NeoXtreem
Copy link

NeoXtreem commented Nov 5, 2019

Scans of non-public classes result in registrations of compiler-generated classes on C# closures. E.g.:

internal class Foo
{
    public void Bar(int i)
    {
        async Task Baz()
        {
            i = i;
        }
    }
}

And then:

services.Scan(s => s.FromCallingAssembly().AddClasses(false));

This results in the following unexpected registration:

ServiceType = {Name = "<g__Baz|0>d" FullName = "MyApp.Foo+<>c__DisplayClass0_0+<g__Baz|0>d"}, ImplementationType = {Name = "<g__Baz|0>d" FullName = "MyApp.Foo+<>c__DisplayClass0_0+<g__Baz|0>d"}

If I derive Foo from a new (empty) interface IFoo and add .AsImplementedInterfaces() to the Scan call, I get the following unexpected registration instead:

ServiceType = {Name = "IAsyncStateMachine" FullName = "System.Runtime.CompilerServices.IAsyncStateMachine"}, ImplementationType = {Name = "<g__Baz|0>d" FullName = "MyApp.Foo+<>c__DisplayClass0_0+<g__Baz|0>d"}

@khellang
Copy link
Owner

khellang commented Nov 5, 2019

Are you able to get any more information about these types? Do they have any attributes?

@NeoXtreem
Copy link
Author

NeoXtreem commented Nov 5, 2019

No, the type is exactly as I put it in the description above. I tested it with exactly that code, and it happens. No attributes, nothing else. It's a .NET Core 3.0 app, but I don't think that makes a difference.

The code looks simple, but the key is in the parameter to the public method (i) which is then accessed in the async local function. This causes an anonymous class to be generated due to the C# closure in another compiler-generated class because of the async local function.

@khellang
Copy link
Owner

khellang commented Nov 5, 2019

I'm talking about the unexpected registered display classes, not the Foo class you posted. I'm just looking to see whether ignoring the CompilerGeneratedAttribute is enough.

@NeoXtreem
Copy link
Author

Nothing shown in CustomAttributes, only NestedPrivate | Sealed | BeforeFieldInit in Attributes.

@khellang
Copy link
Owner

khellang commented Nov 5, 2019

Ah, I see what's going on. Roslyn's generating a private nested state machine class inside the display class. Only the outer class has CompilerGeneratedAttribute on it.

I'm not really sure what the best way to solve this would be.

  1. Check outer classes for CompilerGeneratedAttribute if the type is nested
  2. Filter out types implementing IAsyncStateMachine
  3. Filter out nested private types

Or maybe a combination of them? 🤔

@NeoXtreem
Copy link
Author

Option 1 might not work because there could be several nested layers. Option 2 might not work if someone decides to implement that interface themselves. Option 3 would exclude non-generated types as well which might not be desired in some weird DI use cases.

My suggestion would be, if the scan is done recursively, to skip any classes nested in one with CompilerGeneratedAttribute.

@khellang
Copy link
Owner

khellang commented Nov 5, 2019

Good points. I guess I have to recursively check nested classes for outer types with the attribute then 👌

@NeoXtreem
Copy link
Author

NeoXtreem commented Nov 5, 2019

This would be deterministic and in line with your existing functionality that checks for that attribute. But this may require a refactor as scanning anti-recursively through all ancestors for each type would be very slow. And you can't just check the immediate outer type as this would then fail for nested types more than one level deep below the compiler-generated type. I would suggest refactoring so that the scan goes through all types recursively and skips recursion into nested types for types that have CompilerGeneratedAttribute.

Or, if your assembly scan just presents you with a flat list, then maybe you can do a two-pass scan. The first pass builds up a hierarchical tree of types, and the second pass then does the recursive scan and registration.

@khellang
Copy link
Owner

khellang commented Nov 5, 2019

I'm just getting a flat list of declared types from the assembly. I will only walk up the "tree" if the type is nested and stop once I find the attribute. I can't imagine this will be a big perf hit as I think it's pretty uncommon to have many layers of nesting and it would only be in cases where private classes are included.

@NeoXtreem
Copy link
Author

Sure, but this could in theory also be done in one pass by marking each type as compiler-generated as you add it to the tree data structure. You do this by checking if either it has CompilerGeneratedAttribute or if its immediate parent is marked as compiler-generated, and you will have the parent available when processing each class. You then register the type at that point also (at the same time as adding it to the tree data structure) if it is not compiler-generated.

I wrote a similar algorithm recently to process flat data into a hierarchical tree structure where each item had a parent ID. I could potentially do this for you if you point me to the code where the scan of the flat list is done. It would be much more deterministic, better performing and cater for virtually unlimited nested levels. You never know how complex a program can get, especially if someone has some crazy design made up of layers and layers of nested local functions because they're explicitly coding a hierarchical data model!

@khellang
Copy link
Owner

khellang commented Nov 6, 2019

you will have the parent available when processing each class

For nested types, this is already available in the reflection model through the DeclaringType property 😊

@NeoXtreem
Copy link
Author

Exactly, that's why I said you will have it available. The algorithm I suggest would therefore have a time complexity of O(n). If you traversed the entire ancestral hierarchy for each item, you would then have a time complexity of O(n log n) which isn't ideal.

@khellang
Copy link
Owner

khellang commented Feb 6, 2020

Pushed a new version to NuGet that should hopefully address this. Let me know if it doesn't 😅

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

No branches or pull requests

2 participants