Make C# source generators incremental

[raulsntos]

Switch to incremental source generators to increase their performance as they are executed in steps and avoid re-doing work.

TODO

• I am not convinced that the way I'm reading the analyzer properties is the best way to do it since the syntax step is still going to be executed regardless of the properties' values.
• I haven't tested them thoroughly and I'd like to make sure I didn't introduce a regression before marking the PR out of draft.

Production edit: closes godotengine/godot-roadmap#50

[hawkerm]

I didn't realize there was a draft for this already, I had made a discussion proposal here as ISourceGenerator does indeed have a lot of performance implications for anything beyond the smallest of projects.

If you're looking for structure from other generators the MVVM Toolkit ones are some of the best around, they have tests setup as well.

For generator performance, you can look to use context.SyntaxProvider.ForAttributeWithMetadataName to very efficiently look for specific attributes.

For tests, you can also use things like Compilation.GetFileContentsByName to check the output from the generator, from other SG tests here.

Seems like setting up tests for the current generator could be a good first step? I know having tests makes running source generators and debugging them as part of debugging a test a whole lot easier of a workflow.

Also, not sure how decisions in godotengine/godot-proposals#7895 effect this code; I'm not familiar enough with the engine and extension internals to know.

[Lidsel]

Some suggestions:

1. Combine the generators for Godot classes into a single generator, so the expensive value-gathering code runs only once. Move just the builders to separate classes instead.
2. GodotClassData should be a record and already contain all the class + member information you're going to need for the builders in value-comparable form (this includes value-comparable collections). This allows skipping needless re-generation when nothing about a class changes between compilations.
3. As pointed out, ForAttributeWithMetadataName is the preferred approach for identifying syntax that should trigger code generation and its use on Godot classes can be enforced with an analyzer, but that approach increases user friction. Decide which is best.
4. The diagnostic-reporting code should be in analyzers instead. Analyzers run asynchronously and aren't as performance-tight as generators. @paulloz also mentioned there being issues with displaying the diagnostics in IDEs if they come from generators.

[hawkerm]

@Lidsel what do you mean by the following:

but that approach increases user friction. Decide which is best.

What user friction is increased? User use of attributes should be unchanged between the two implementations. This is just generator implementation detail using ForAttributeWithMetadataName to find attributes?

Agree that dedicated analyzers can certainly be useful to provide hints/help to users on using correct patterns.

[Lidsel]

@hawkerm The user would have to add an attribute to every Godot type-derived class for ForAttributeWithMetadataName to work. If the types are made from within the editor it could be done automatically in the template, but not if the user creates a class with the regular class template in the IDE. Also on existing projects they would need to run a code fix to add them retroactively.
It's not a big amount of friction, but I also understand there haven't been complaints about the performance so far.

[raulsntos]

1. Combine the generators for Godot classes into a single generator, so the expensive value-gathering code runs only once. Move just the builders to separate classes instead.

I was thinking the same thing. Usually I prefer smaller PRs that make small improvements over big PRs that are hard to review, so I just wanted to make an initial move to incremental generators before doing more changes.

2. GodotClassData should be a record and already contain all the class + member information you're going to need for the builders in value-comparable form (this includes value-comparable collections). This allows skipping needless re-generation when nothing about a class changes between compilations.

Sure, like I said I wanted to keep the PR small to start, but it sounds like a good idea.

3. As pointed out, ForAttributeWithMetadataName is the preferred approach for identifying syntax that should trigger code generation and its use on Godot classes can be enforced with an analyzer, but that approach increases user friction. Decide which is best.

This would require C# classes to have some attribute and, as you say, would increase user friction. I don't think we'll want to do that with the scripting API. It would also break compatibility.

4. The diagnostic-reporting code should be in analyzers instead. Analyzers run asynchronously and aren't as performance-tight as generators. @paulloz also mentioned there being issues with displaying the diagnostics in IDEs if they come from generators.

I think that depends on the diagnostic, if the generator is already going to be performing the check anyway might as well report the diagnostic, that's what the API is there for, I imagine. Of course if there's some checks that we can move outside of the generators into analyzers that would be great.

As for IDEs not supporting diagnostics from generators, that sounds like an IDE bug to me. I think I've heard from users that Visual Studio does display them, but I can't confirm since I don't use it. I've seen them reported in VSCode at some point with the new Roslyn-based C# extension but I think source generators are currently broken, so I'm currently using the Omnisharp backend.

---

As you can see this PR is somewhat abandoned at the moment, I haven't had the time to work in it. I'm mostly worried about breaking changes, so I think a good first step would be to implement proper tests (I was looking at Microsoft.CodeAnalysis.Testing). Then we can make refactors without worrying so much about breaking stuff.

[hawkerm]

@hawkerm The user would have to add an attribute to every Godot type-derived class for ForAttributeWithMetadataName to work. If the types are made from within the editor it could be done automatically in the template, but not if the user creates a class with the regular class template in the IDE. Also on existing projects they would need to run a code fix to add them retroactively. It's not a big amount of friction, but I also understand there haven't been complaints about the performance so far.

I was recommending this API for the existing attributes like Export, so there wouldn't be changes required there.

For the classes themselves, that's of course a different discussion. Not sure if there's a better approach from a performance perspective. At least with Godot most things inherit from some sort of Node, so that should be a starting point to limiting scope of the generator.

[Lidsel]

I think that depends on the diagnostic, if the generator is already going to be performing the check anyway might as well report the diagnostic, that's what the API is there for, I imagine.

@raulsntos The recommendation to report diagnostics in analyzers is based on general recommendations from the Roslyn team. A source generator will trigger whenever the source changes i.e. on every keystroke in editing any code file in the project, you want to minimize the amount of work it does for the value providers. Reporting diagnostics is extra work and it will do it for every class it examines which is all of them every time. An analyzer, from what I can tell, will run just on the files you open, rerun while you're editing them and on the entire project when you're building it - i.e. when it's actually actionable.

I was recommending this API for the existing attributes like Export, so there wouldn't be changes required there.

@hawkerm By the time we're examining for Export here we've moved on from syntactic to semantic model and looking at symbols.

[paulloz]

if the generator is already going to be performing the check anyway might as well report the diagnostic

Hard agree.

I've seen them reported in VSCode at some point with the new Roslyn-based C# extension

That may indeed be an issue with Omnisharp that'll get phased out when (if?) the issues with the Roslyn extension are fixed. In my own tests (granted, more than a year old now), diagnostics reported from an ISourceGenerator never showed in VSC. But all of those reported from a DiagnosticAnalyzer appeared how they should've. I'd be interested to know what's the state of other IDEs on that.