Incremental generator shouldn't include symbols in the pipeline

[Youssef1313]

Is there an existing issue for this?

• I have searched the existing issues

Current Behavior

TextColorToGenerator includes symbols in userGeneratedClassesProvider and mauiControlsAssemblySymbolProvider, then filtered out in inputs.

Maui/src/CommunityToolkit.Maui.SourceGenerators/Generators/TextColorToGenerator.cs

Lines 28 to 84 in 9925e10

	// Get All Classes in User Library
	var userGeneratedClassesProvider = context.SyntaxProvider.CreateSyntaxProvider(
	static (syntaxNode, cancellationToken) => syntaxNode is ClassDeclarationSyntax { BaseList: not null },
	static (context, cancellationToken) =>
	{
	var compilation = context.SemanticModel.Compilation;
	var iTextStyleInterfaceSymbol = compilation.GetTypeByMetadataName(iTextStyleInterface);
	var iAnimatableInterfaceSymbol = compilation.GetTypeByMetadataName(iAnimatableInterface);
	if (iTextStyleInterfaceSymbol is null || iAnimatableInterfaceSymbol is null)
	{
	throw new Exception("There's no .NET MAUI referenced in the project.");
	}
	var classSymbol = (INamedTypeSymbol?)context.SemanticModel.GetDeclaredSymbol(context.Node);
	// If the ClassDlecarationSyntax doesn't implements those interfaces we just return null
	if (classSymbol is null
	|| !(classSymbol.AllInterfaces.Contains(iAnimatableInterfaceSymbol, SymbolEqualityComparer.Default)
	&& classSymbol.AllInterfaces.Contains(iTextStyleInterfaceSymbol, SymbolEqualityComparer.Default)))
	{
	return null;
	}
	return classSymbol;
	});
	// Get Microsoft.Maui.Controls Symbols that implements the desired interfaces
	var mauiControlsAssemblySymbolProvider = context.CompilationProvider.Select(
	static (compilation, token) =>
	{
	var iTextStyleInterfaceSymbol = compilation.GetTypeByMetadataName(iTextStyleInterface);
	var iAnimatableInterfaceSymbol = compilation.GetTypeByMetadataName(iAnimatableInterface);
	if (iTextStyleInterfaceSymbol is null || iAnimatableInterfaceSymbol is null)
	{
	throw new Exception("There's no .NET MAUI referenced in the project.");
	}
	var mauiAssembly = compilation.SourceModule.ReferencedAssemblySymbols.Single(q => q.Name == mauiControlsAssembly);
	var symbols = GetMauiInterfaceImplementors(mauiAssembly, iAnimatableInterfaceSymbol, iTextStyleInterfaceSymbol).Where(static x => x is not null);
	return symbols;
	});
	// Here we Collect all the Classes candidates from the first pipeline
	// Then we merge them with the Maui.Controls that implements the desired interfaces
	// Then we make sure they are unique and the user control doesn't inherit from any Maui control that implements the desired interface already
	// Then we transform the ISymbol to be a type that we can compare and preserve the Incremental behavior of this Source Generator
	var inputs = userGeneratedClassesProvider.Collect()
	.Combine(mauiControlsAssemblySymbolProvider)
	.SelectMany(static (x, _) => Deduplicate(x.Left, x.Right).ToImmutableArray())
	.Select(static (x, _) => GenerateMetadata(x));
	context.RegisterSourceOutput(inputs, Execution);

Expected Behavior

It's a better idea to not include symbols in the pipeline, ie, filter everything early.

Steps To Reproduce

N/A

Link to public reproduction project repository

N/A

Environment

N/A

Anything else?

dotnet/roslyn-analyzers#6352

[brminnick]

Thanks for the tip @Youssef1313!

It's a better idea to not include symbols in the pipeline, ie, filter everything early.

Could you give an example what this looks like, or, even better, open a PR?

I'm admittedly not a Source Generator expert and don't fully grasp your recommendations. But I'm excited to learn more!

[VladislavAntonyuk]

#815 (comment)

[brminnick]

And will this be a performance improvement,a memory improvement or a syntax/code/best practices improvement?

[Youssef1313]

@brminnick The discussion in dotnet/roslyn-analyzers#6352 is quite detailed.

From dotnet/roslyn-analyzers#6352 (comment):

You should not have symbols in the pipeline, as that can root previous compilations and prevent large swaths of memory from being freed.

There also should be a performance improvement.

[pictos]

I would like to see a benchmark before changing this. I tested it before moving forward and the SG keeps its incremental behavior. It's true that having symbols will break the Incremental behavior, but just if it gets inside the RegisterSourceOutput. As you can see at the end all the symbols are converted to our own model.

[Youssef1313]

It's true that having symbols will break the Incremental behavior, but just if it gets inside the RegisterSourceOutput

See dotnet/roslyn-analyzers#6352 (comment)

Yeah, symbols in any part of teh pipeline as values in the tables just won't work. They will always compare unequal, causing all entries to be recomputed, and all further steps to happen. So, in effect, you can have them in the tables... it just means you have no incrementality there. :)

[brminnick]

@Youssef1313 Could you provide the data/charts/benchmark etc. that demonstrates that we have broken incrementallity in our Source Generator?

I'd like to better understand the performance impact of our current implementation versus your recommended improvement.

I also agree with @pictos that the next PR that changes/improves our source generator should implement this same benchmark. We'll run + verify this benchmark our CI/CD pipeline to ensure we never accidentally degrade performance in our Source Generators going forward.

[ghost]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 3 days. It will be closed if no further activity occurs within 2 days of this comment. If it is closed, feel free to comment when you are able to provide the additional information and we will re-investigate.

[Youssef1313]

Not really stale. This needs an action.

[pictos]

@Youssef1313 We asked you to provide info that our source generator doesn't work as expected. The last PR that I did fixed the issue and it's working better now. If you don't provide the repro we will close this

[Youssef1313]

There is really no repro needed. All explanations are in dotnet/roslyn-analyzers#6352.

The compiler team clearly states that incremental generator pipeline shouldn't include symbols. This is also being followed properly in generators in CommunityToolkit/dotnet.

[Youssef1313]

Thanks for confirming @Sergio0694.

we went through extensive work there to ensure to follow all recommended steps, which included fixing the issue being reported here. I know it's a long, tiresome process, but it's necessary to ensure generators are working as best they could.

Yeah, I have taken this exercise too in one of the generators in Uno Platform, and while it was a very small generator, and took me quite a bit to write it properly.

[VladislavAntonyuk]

@Youssef1313 Would you like to create a PR with the fix?

[Youssef1313]

It will be hard currently. Maybe I can take a look in a month.

[Sergio0694]

"your comment was very rude and I didn't like it"

I'm sure Youssef will be able to say that, but I can guarantee you he didn't mean that to come across that way (language barrier maybe?). He's a super active member of the community that's always extremely helpful and willing to help folks out, and with lots of community contributions in Roslyn and other repos (eg. he helped in the Toolkit before as well). I read that as "let's establish that this needs an action and it's not an incorrect issue to dismiss", rather than "you have to fix this now" as a demand 🙂

"we generate those classes just once"

This is where the confusion might be coming from. The issue we're trying to point out is that, due to how the pipeline is currently setup, you think you're generating those classes just once, when in fact the generator is doing the entire work and generating every single file again for every single keystroke currently, all the time, because the pipeline is always invalidated 😅

[pictos]

This is where the confusion might be coming from. The issue we're trying to point out is that, due to how the pipeline is currently setup, you think you're generating those classes just once, when in fact the generator is doing the entire work and generating every single file again for every single keystroke currently, all the time, because the pipeline is always invalidated 😅

@Sergio0694, before the merge I checked if the SG was behaving as incremental... I recorded two videos in order to prove it when I was working on it. In both videos take a look on the DateTime stamp

This one is before the fix, as you will see the file is generating every time:

SG_bug_for_MCT.mp4

The next one is after the fix

SG_Fixed.mp4

[Sergio0694]

Sorry my bad, I was missing the last couple lines in the selector. You're correct, it will not generate all files every time as the last two steps are cached. It will still, however, re-run all previous steps for every single edit up until that point (unnecessarily), and also root compilation objects, causing them to remain in memory for longer than necessary and increase memory usage.

I agree this is less critical than if it was really regenerating all files for every edit, sure. Still worth a fix, given time though 🙂

[pictos]

@Sergio0694 no need to be sorry (: , SG has a lot of details to pay attention to.
I agree with you that makes sense to improve it... But this will take a while since isn't a blocker.
And of course this can be fast with community support