Add source generator for dependency injection

[smoogipoo]

This one's been brewing for a long time while I tried to figure out the best way to do things. It provides a structural foundation on top of which we can build other source generators.

Strategy

There are two primary goals that guided my decisions:

1. Implement the complete functionality of dependency injection without compromises.
2. Make the SG as transparent to the user as possible. Especially considering the recent realm-dotnet issues where the project started failing to compile inexplicably as a result of requiring us to implement the interface on classes.

Before I explain further, let me mention that I'm not particularly proud of the naming in this PR and I understand it can be confusing. I'm open to alternatives.

For source generators to work at all for us, we unfortunately need to make every Drawable class partial.

To meet the above goals, the structure I've come up with is to add a partial class that implements a specific interface - ISourceGeneratedDependencyActivator. If the dependency activator detects that this interface has been implemented, it uses it instead of going via the reflection path.

The ISourceGeneratedDependencyActivator interface exposes one method in particular - RegisterDependencyActivator(IDependencyActivator). The source generator implements this method in the form of:

if (activator.IsRegistered(type))
    return;
// Base call if required
activator.Register(...);

Of importance, Register() here generates two Func<>s which inject/cache dependencies for the current object. It does not do the whole activation process in-line. The reason for this is that I want this to be transparent and want both the reflection and SG modes to work concurrently regardless of the type hierarchy (e.g. if code hasn't been recompiled to support the SG).
That is to say, as far as DependencyActivator (the o!f static class orchestrating everything) is concerned, the returned Func<>s are just the normal callbacks generated by the reflection pathway.

This is all placed in a file named g_{class_name}_{guid}_Dependencies.cs which you can get to by going to the definition of classes in Rider. Why the GUID? Because class names can conflict. There doesn't seem to be a proper solution to this offered by the Roslyn team.

Analyser + codefix

I've created a pretty "basic" analyser for warning against non-partial classes when required. It attempts to discover:

• Types used as the argument to calls of DependencyContainer.Inject().
• Types used as type arguments of CachedModelDependencyContainer<T>.
• Types implementing ITransformable, IDrawable or ISourceGeneratedDependencyActivator.

This covers all current cases while being as least invasive as possible (i.e. not analysing every member of every class ever).

A code fix has been provided for the diagnostic, however if you're planning to codefix the whole solution I suggest the running the following command until it outputs 0 changes:

dotnet format analyzers osu.Desktop.slnf --verbosity d

The code fix seems to not fix all issues on the first attempt. This is potentially caused by my implementation, however I've tried everything that I could find and nothing's worked. I'll look into fixing this as a future effort, however for the most part it's a one-time thing.

The analyser and code fixer don't have tests at the moment. These are also planned in a future effort and will be structured similarly to the source generator tests and the osu-localisation-analyser project.

Notes

• Member accessor validation is not supported yet. I think these will probably be added to an analyser in a future effort, but I've disabled the relevant tests in smoogipoo@f332420 for now.
• Build time has increased. This can probably be improved further, however I don't want to drag on this PR much longer.
• Going to definitions of classes is a +1 process now, with every single definition now giving you two locations.
• I eventually want to update the source generator to make use of IIncrementalGenerator, but there's a bit more reading to do for that.
• The class is generated via AST rather than raw code. This is a personal/stylistic choice because I originally implemented it using raw code and couldn't bear it.
• The SG code quality is not quite where I want it to be at the moment - it's lacking documentation (compared to something like osu-localisation-analyser) and the tests are lacking structure. This will be improved in a future effort.

Testing

You can test this branch with the following trees:
osu-framework: https://github.com/smoogipoo/osu-framework/tree/sg-dependency-activator-partial-classes
osu: https://github.com/smoogipoo/osu/tree/sg-test (includes local-framework)

I've tested:

• Only osu-framework
• osu + local framework.
• osu + osu-framework package.

I have not tested VSCode compatibility, but I have no reason to believe it won't work, unlike osu-localisation-analyser where there's differences in how IDEs add files to workspaces from analysers/codefixes.

I'll leave it to @peppy to post profiling results.

[peppy]

Only providing memory based tests for now (there will definitely be performance improvements on the CPU side as you'd expect, but these will roughly track the reduction in allocations and GC object tracking):

Testing is done by loading the game from a cold start, and entering song select (with only 9 beatmap sets loaded). Note that this also includes my other recent optimisations to AudioComponent / GetUnboundCopy, which are contributing to the overall reduction (#5532 / #5531).

before	after
Retained (overall)
Retained (system)
Retained (bindable aka unrelated to this PR)
Allocations: 3.12m (317mb)	Allocations: 1.58m (186mb)
Retained: 745k (129mb)	Retained: 343k (95mb)

[smoogipoo]

Before merging this, please give any suggestions for names, if any. It's going to be extremely hard to change things after the fact since this is all compiled.

[peppy]

Which naming in particular?

[smoogipoo]

• ISourceGeneratedDependencyActivator - the implemented interface (important).
• IDependencyActivator - the type passed into the implemented ISourceGeneratedDependencyActivator. RegisterDependencyActivator() method (important).
• RegisterDependencyActivator() - the method implemented on the partial class which sends back the caching/injection Func<>s (important).
• DependencyActivator - the class orchestrating everything (not important).
• DependencyActivatorProxy - the actual object passed into the RegisterDependencyActivator method (not important).

[bdach]

Code review

Because this is slated for a quick merge, I explicitly did not read any of the generator internals at this point. I probably will at a later date, but reviewing a 3k line diffstat in detail would be counterproductive anyway. I see a lot of tests here and therefore I'm just assuming they're broad enough to cover most cases. In a way, as long as outputs of the generator are good the details don't matter too much, functionally speaking.

I did read the actual changes in DI though as they are the key part of this diff. I think I get what this is trying to do, and it makes sense to me (that said I didn't read it that long).

When it comes to naming, I don't think it matters that much as I had much more trouble with deciphering the control flow at first rather than the naming, for two main reasons:

• The fact that DependencyActivator is both a static utility used by Drawable, but also has instances that do stuff and get stored in the static utility, and code for both is interspersed across DependencyActivator, was throwing me off.
• The flow in DependencyActivatorProxy is... something. Took me the better part of 15 minutes to figure out what that discard assignment was doing (hint for future readers - it is not in fact a no-op, as DependencyActivator instance ctors store the reference to selves in the static cache).

That said, when it comes to the names questioned above, my view is thus:

• ISourceGeneratedDependencyActivator: Fine by me.
• IDependencyActivator: Would suggest IDependencyRegistry? IDependencyActivatorRegistry? Maybe IDependencyCache even? Rationale being that no activating is happening there, it's just an access layer to the cache dictionary, more or less.
• RegisterDependencyActivator(): In line with the above, would propose renaming to RegisterForDependencyActivation() or similar.
• DependencyActivator: Fine by me. The Activator suffix is key here for me in that this is the class that actually does the magic (the static stuff does it directly, the activator instances do it indirectly via the static stuff).
• DependencyActivatorProxy: Fine by me and does not really matter to anyone as it is an implementation detail.

See what you think about these. I don't insist on these, they're just subjective feelings. Curious if you see those as any better than what's there right now.

Running the code fix

I'm not sure why, but dotnet format actually didn't do anything for me to run the code fix until I stumbled onto a random google search result, whereupon

dotnet_analyzer_diagnostic.severity = error

was placed in .globalconfig. Then the thing worked after I ran

$ dotnet format analyzers osu.Framework/osu.Framework.csproj --verbosity d --diagnostics=OFSG001

to scope down to just this one code fix rather than fix absolutely every inspection reported by everything ever (which is what the .globalconfig change enabled). Unsure if this is expected. Probably not a big deal if this is supposed to be a one-time operation, though?

After doing the above I'm getting the same results as your test branches, though (after enough iterations).

Build time

Tested on game-side.

• Master w/ framework checkout:

  dotnet build osu.Desktop.slnf  4,01s user 0,66s system 19% cpu 24,318 total
  

• Linked test branch

  dotnet build osu.Desktop.slnf  3,96s user 0,77s system 18% cpu 24,957 total
  

Not sure whether I measured wrong, but this is basically not an issue for me because I can't really tell that anything's changed. Test procedure was basically

$ dotnet clean
$ dotnet restore osu.Desktop.slnf
$ time dotnet build osu.Desktop.slnf

Benchmark results

Running the benchmark attached in this PR on the test branch, I get:

Method	ClearCaches	Mean	Error	StdDev	Median	Gen 0	Allocated
TestInjectWithReflection	False	163.83 ns	3.297 ns	7.962 ns	162.19 ns	0.0019	24 B
TestWithSourceGenerator	False	72.15 ns	1.420 ns	2.804 ns	71.60 ns	0.0019	24 B
TestInjectWithReflection	True	12,630.62 ns	1,887.559 ns	5,565.509 ns	7,901.41 ns	0.4578	5,873 B
TestWithSourceGenerator	True	403.76 ns	5.822 ns	8.891 ns	402.02 ns	0.0553	704 B

That is pretty cut-and-dry. Wins across the board.

Development & quick testing

• Wrote a tiny component just to see how the generation behaves. Everything is as expected, works fine without any intervention.
• Briefly smoke-tested the game-side branch with generation on, found nothing wrong. You probably did more testing than me anyways.
• Briefly smoke-tested game on current master with local framework checkout to test out backwards compatibility with reflection-based DI. Found nothing wrong here too.

---

So to sum all of the above up, aside from the naming suggestions above I don't have much to offer or object to. Good work on this 👍

[smoogipoo]

Adjusted naming as proposed, favouring IDependencyActivatorRegistry because the other two IDependencyX ones to me sound like they're used to retrieve dependencies.

• The fact that DependencyActivator is both a static utility used by Drawable, but also has instances that do stuff and get stored in the static utility, and code for both is interspersed across DependencyActivator, was throwing me off.

I noticed this as well and it's something I want to refactor as a followup.

[smoogipoo]

I'm gonna need another nupkg after this is merged due to the naming changes.

[peppy]

The new naming looks good to be, so I'll do that soon.

[peppy]

💯