Skip to content

Add generic delegate lambda overloads, and associated tests#1321

Merged
tillig merged 8 commits intodevelopfrom
feature/generic-delegate-overloads
Apr 5, 2022
Merged

Add generic delegate lambda overloads, and associated tests#1321
tillig merged 8 commits intodevelopfrom
feature/generic-delegate-overloads

Conversation

@alistairjevans
Copy link
Member

Closes #1320

I'll look at the optimisation for the reflection activator with matching constructor later, thought I'd get this up there.

Aside; I accidentally pushed these changes directly to develop first, and had to revert. 🤦‍♂️

@codecov
Copy link

codecov bot commented Mar 31, 2022

Codecov Report

Merging #1321 (344d7a6) into develop (3402091) will increase coverage by 0.87%.
The diff coverage is 96.73%.

❗ Current head 344d7a6 differs from pull request most recent head e616298. Consider uploading reports for the commit e616298 to get more accurate results

@@             Coverage Diff             @@
##           develop    #1321      +/-   ##
===========================================
+ Coverage    76.84%   77.71%   +0.87%     
===========================================
  Files          188      190       +2     
  Lines         5191     5417     +226     
  Branches      1064     1084      +20     
===========================================
+ Hits          3989     4210     +221     
- Misses         703      705       +2     
- Partials       499      502       +3     
Impacted Files Coverage Δ
src/Autofac/ResolutionExtensions.cs 50.76% <83.33%> (-0.38%) ⬇️
...degen/Autofac.CodeGen/DelegateRegisterGenerator.cs 97.68% <97.68%> (ø)
...ore/Resolving/BaseGenericResolveDelegateInvoker.cs 100.00% <100.00%> (ø)
src/Autofac/ResolveRequest.cs 100.00% <100.00%> (ø)

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 3402091...e616298. Read the comment docs.

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

Looks good. A couple of questions in there in the spirit of brainstorming.

Also curious if it'd be worth putting something in the benchmark suite to compare reflection vs this method of registration to demonstrate that lambda is still faster (or even if there's some tipping point, like once you hit eight dependencies then reflection starts being equal or winning). Definitely not required for this to go in, we could always add benchmarks later.

@alistairjevans
Copy link
Member Author

alistairjevans commented Apr 2, 2022

I've created a new project Autofac.CodeGen under a new codegen folder (src seemed the wrong place for it).

This is an "incremental" generator, so requires the latest SDK, which you'd need anyway to build Autofac; as far as I can tell it works fine under VSCode+OmniSharp and VS2022. You may have problems in VS2019.

The actual generated code files are not committed into source control, however I have added Verify-based snapshot testing for the generated code in a separate test project, Autofac.Test.CodeGen. There's only the one test, but it makes sure that as the build system/roslyn updates, our generated code stays the same. I've used this approach before for generators, and works pretty well.

You can find the verified files (used as the baseline) in the Snapshots folder.

Latest benchmarks with the new, generated code:

|                                   Method |       Mean |    Error |   StdDev |  Gen 0 | Allocated |
|----------------------------------------- |-----------:|---------:|---------:|-------:|----------:|
|      ResolveManuallyWithComponentContext |   850.7 ns | 10.57 ns | 12.59 ns | 0.2861 |      1 KB |
|             ResolveWithCapturedParameter |   874.1 ns |  9.86 ns |  8.23 ns | 0.2861 |      1 KB |
|        ResolveWithProvidedTypedParameter | 1,081.7 ns | 11.02 ns |  9.77 ns | 0.3548 |      1 KB |
|        ResolveWithProvidedNamedParameter | 1,074.9 ns |  8.37 ns |  7.42 ns | 0.3548 |      1 KB |
| ResolveReflectionWithRegisteredParameter | 1,105.8 ns |  8.79 ns |  7.34 ns | 0.4025 |      2 KB |

@tillig
Copy link
Member

tillig commented Apr 4, 2022

This could be "someone moved my cheese" level complaining, so if I'm just being grumpy or whatever definitely just say so - no offense will be taken. I'm just noticing that debugging generated code is sort of a pain, at least in VS Code. I can't find the generated source (it's not checked in and seems to maybe be in some sort of temp location?) so the only way to see it is to go to the unit tests and hit F12 to "Go To Definition." I can then get to the Register methods, but I can't get to the DelegateInvokers holder class from there.

No definition found for DelegateInvokers

I also tried to get some analyzer or compiler warnings out of the generated code and it was pretty hard. I never did quite get the order of operations down that needed to happen. I can see you have to change the analyzer and build (I think?) and then change Autofac.RegistrationExtensions and build again (I think?) but I never did really get it figured.

I did finally get it to issue an error during compilation, but when you do get an error, you can't navigate to the source to see what went wrong.

Trying to get a compiler/analyzer warning

Is the experience better in Visual Studio?

It may be worth adding, say, a tiny README to the Autofac.CodeGen folder that explains a little more about how it works or even just links to appropriate documentation. Where does the generated source go? What's the order of steps to make this regenerate the code? When should we expect to see compiler/analyzer warnings?

Again, if this is just me having to get used to something that other folks find pretty straightforward, let me know. I'm still hanging on to T4 templates and CodeSmith Generator and Reflection.Emit so this is pretty new to me.

@alistairjevans
Copy link
Member Author

Personally I've not used source generators much at all in VSCode. Visual Studio is evidently somewhat better than VSCode, in that it does allow you to navigate the generated code quite well. It's still not good at picking up changes to the source generator.

Can you try adding <EmitCompilerGeneratedFiles>true</EmitCompilerGeneratedFiles> to the Autofac csproj? It should put the generated files on-disk in the obj folder, might make it easier for OmniSharp to navigate to them?

I've found that the most effective/correct manner to definitely getting errors out of generated source is to run dotnet build; the CLI build is very accurate in building the latest source generator and using it in the referencing projects.

No problem adding a README file to the CodeGen project.

@alistairjevans
Copy link
Member Author

Oh, also, you didn't say in your comment, but if you are debugging, and you put a breakpoint in the (not generated) test method, can you step-in to the generated code in VSCode if you keep stepping in?

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

I'm definitely learning a lot about source generators. Pros and cons, dev experience, that sort of thing. No real major findings here, mostly just little one-liners.

<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
</PackageReference>
<PackageReference Include="Verify.SourceGenerators" Version="1.3.0" />
Copy link
Member

Choose a reason for hiding this comment

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

Definitely not obvious that the *.verified.cs files get ignored by the Verify package. VS Code, at least, doesn't really like it if you open the verified.cs files because it then tries to analyze them and things like System.Diagnostics.CodeAnalysis aren't in ImplicitUsings. Which isn't a big deal, it just took me a bit to realize why I'd get errors if I opened the file but not on build.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add something to a README about this.

@tillig
Copy link
Member

tillig commented Apr 4, 2022

It definitely looks like an OmniSharp thing where navigating to generated code won't treat that generated code like it's part of a project. It treats it more like... if you opened that file in a whole disconnected context so it doesn't tie symbols together. I've dropped a comment on the open issue for that in OmniSharp.

I am able to set a breakpoint and step into the generated code, and once I'm there I can set breakpoints inside that generated code just like I can with SourceLink. It's just getting started and having to step in there that's a challenge, but it's not insurmountable, just not obvious.

Setting <EmitCompilerGeneratedFiles>true</EmitCompilerGeneratedFiles> does get the generated source to start showing up in obj/Debug/netXXX/generated/... which is nice, but it doesn't appear to fix the "F12 navigate to definition" sort of thing in VS Code. It doesn't matter whether that property is true, VS Code debugging into generated code (like from a unit test) works regardless. It's just a navigation problem, which is something I can live with.

@alistairjevans
Copy link
Member Author

Fixed all the obvious issues, including adding the analysers to the codegen project.

I've yet to write a README file, but I'll do that before merge.

@tillig
Copy link
Member

tillig commented Apr 5, 2022

Looks like the stuff left:

  • README to kinda explain source generator stuff
  • Reusable StringBuilder
  • Updates to .editorconfig
  • Debug.Assert on those out var items.

Re: .editorconfig, we don't have to tackle that whole thing right now. I thought you were just going to mark those naming items as warning and call it a day. You can update everything if you want, for sure, but we don't have to get in the weeds there if you don't want to.

@alistairjevans
Copy link
Member Author

OK READMEs are up, think I've resolved everything outstanding now.

Copy link
Member

@tillig tillig left a comment

Choose a reason for hiding this comment

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

🦄

@tillig tillig merged commit ad07bf1 into develop Apr 5, 2022
@alistairjevans
Copy link
Member Author

👍 I'll work on a PR for the docs for the new feature.

@tillig tillig deleted the feature/generic-delegate-overloads branch May 25, 2022 19:56
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.

Feature Proposal - Generic Overloads for Register to make delegate registration more concise

2 participants