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

Simplify resolution of dependency injection candidates #5548

Merged
merged 8 commits into from
Nov 29, 2022

Conversation

smoogipoo
Copy link
Contributor

A common theme I observed is that finding candidate classes for dependency injection has considerable overheads, both in terms of correctness and likely also in performance (since more symbols have to be analysed otherwise).

The latter is the reason why the source generator didn't do as aggressive of a job as the analyser in generating the dependency injection classes, but this resulted in a disparity between the two that couldn't exist. Either both of them should miss inspections, or both of them should not.

This PR takes the second route of not missing inspections (greatest correctness) by requiring dependency injection classes to implement the IDependencyInjectionCandidate interface. For the most part this is a negligible overhead - the only such class outside of tests that I'm aware of is the osu!-side Room.

As a result, not only has the analyser been simplified to a quarter of its original size, but the SG perfectly follows the inspections provided by the analyser. The future incremental source generator implementation will also benefit from this.

The downside is that this is a breaking change, but as I mentioned above it's not a big one.

vNext

Classes used in dependency injection need to implement the IDependencyInjectionCandidate interface

This does not affect Drawable subclasses.

@smoogipoo
Copy link
Contributor Author

Will need a source generator nupkg update with the changes in this PR, for this PR >_>

@peppy peppy merged commit 4b3348e into ppy:master Nov 29, 2022
@smoogipoo smoogipoo deleted the simplify-dependency-candidate branch September 11, 2023 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants