All StructId generators are not incremental and extremely inefficient (analyzers too)

[Sergio0694]

Overview

Disclaimer: I'm not using this package. Just leaving some feedback here to help the ecosystem 🙂

The incremental generators in this project unfortunately have some (big) issues: they are completely not incremental, and they are extremely inefficient. The latter is because they introduce a whole bunch of generator state tables that will never compare as equal anyway (as they're capturing values that cannot be equated), meaning they'll generate source again every single time.

To clarify:

• These generators as is are completely incorrect
• They need to be rewritten to use an attribute as trigger, and use ForAttributeWithMetadataName

If the objection is "but using an attribute is less convenient for users", sure. Maybe. But that's the only way to make this correct and it's a design principle that should not be considered optional for generators. Performance has to come first, because poorly performing generators (like these ones below) impact the entire IDE experience.

You should also enable this property to get the Roslyn analyzer help spot all of these issues (or, most of them):

<EnforceExtendedAnalyzerRules>true</EnforceExtendedAnalyzerRules>

The offending generators:

StructId/src/StructId.Analyzer/BaseGenerator.cs

Lines 27 to 64 in ff93ea8

	public virtual void Initialize(IncrementalGeneratorInitializationContext context)
	{
	var known = context.CompilationProvider
	.Select((x, _) => new KnownTypes(x));
	// Locate the required type
	var types = context.CompilationProvider
	.Select((x, _) => x.GetTypeByMetadataName(referenceType));
	var ids = context.CompilationProvider
	.SelectMany((x, _) => x.Assembly.GetAllTypes().OfType<INamedTypeSymbol>())
	.Where(x => x.IsStructId())
	.Where(x => x.IsPartial());
	var combined = ids.Combine(types)
	// NOTE: we never generate for compilations that don't have the specified value interface type
	.Where(x => x.Right != null)
	.Combine(known)
	.Select((x, _) =>
	{
	var ((structId, referenceType), known) = x;
	// The value type is either a generic type argument for IStructId<T>, or the string type
	// for the non-generic IStructId
	var valueType = structId.AllInterfaces
	.First(x => x.Name == "IStructId")
	.TypeArguments.OfType<INamedTypeSymbol>().FirstOrDefault() ??
	known.String;
	return new TemplateArgs(structId, valueType, referenceType!, known);
	});
	if (referenceCheck == ReferenceCheck.ValueIsType)
	combined = combined.Where(x => x.TValue.Is(x.ReferenceType));
	combined = OnInitialize(context, combined);
	context.RegisterImplementationSourceOutput(combined, GenerateCode);

StructId/src/StructId.Analyzer/DapperGenerator.cs

Lines 36 to 87 in ff93ea8

	var customHandlers = context.CompilationProvider
	.SelectMany((x, _) => x.Assembly.GetAllTypes().OfType<INamedTypeSymbol>())
	.Combine(context.CompilationProvider.Select((x, _) => x.GetTypeByMetadataName("Dapper.SqlMapper+TypeHandler`1")))
	.Where(x => x.Left != null && x.Right != null &&
	x.Left.Is(x.Right) &&
	// Don't emit as plain handlers if they are id templates
	!x.Left.GetAttributes().Any(a => a.IsValueTemplate()))
	.Select((x, _) => x.Left)
	.Collect();
	// Non built-in value types can be templatized by using [TValue] templates. These would necessarily be
	// file-local types which are not registered as handlers themselves but applied to each struct id TValue in turn.
	var templatizedValues = context.SelectTemplatizedValues()
	.Where(x => !IsBuiltIn(x.TValue.ToFullName()))
	.Combine(context.CompilationProvider.Select((x, _) => x.GetTypeByMetadataName("Dapper.SqlMapper+TypeHandler`1")))
	.Where(x => x.Left.Template.TTemplate.Is(x.Right))
	.Select((x, _) => x.Left);
	// If there are custom type handlers for value types that are in turn used in struct ids, we need to register them
	// as handlers that pass-through to the value handler itself.
	var customHandled = source
	.Combine(customHandlers.Combine(templatizedValues.Collect()))
	.Select((x, _) =>
	{
	(TemplateArgs args, (ImmutableArray<INamedTypeSymbol> handlers, ImmutableArray<TemplatizedTValue> templatized)) = x;
	var handlerType = args.ReferenceType.Construct(args.TValue);
	var handler = handlers.FirstOrDefault(x => x.Is(handlerType, false));
	if (handler == null)
	{
	var templated = templatized.Where(x => x.TValue.Equals(args.TValue, SymbolEqualityComparer.Default))
	.FirstOrDefault();
	// Consider templatized handlers that will be emitted as custom handlers too for registration.
	if (templated != null)
	{
	var identifier = templated.Template.Syntax.ApplyValue(templated.TValue)
	.DescendantNodes()
	.OfType<TypeDeclarationSyntax>()
	.First()
	.Identifier.Text;
	// Use lighter symbol since our template rendering only uses the type name.
	handler = new KnownTypeNameSymbol(identifier);
	}
	}
	return args with { ReferenceType = handler! };
	})
	.Where(x => x.ReferenceType != null);
	context.RegisterSourceOutput(builtInHandled.Collect().Combine(customHandled.Collect()).Combine(templatizedValues.Collect()), GenerateHandlers);

StructId/src/StructId.Analyzer/EntityFrameworkGenerator.cs

Lines 50 to 67 in ff93ea8

	var converters = context.CompilationProvider
	.SelectMany((x, _) => x.Assembly.GetAllTypes().OfType<INamedTypeSymbol>())
	.Combine(context.CompilationProvider.Select((x, _) => x.GetTypeByMetadataName(ValueConverterType)))
	.Where(x => x.Left != null && x.Right != null &&
	x.Left.Is(x.Right) &&
	!x.Left.IsUnboundGenericType &&
	x.Left.BaseType?.TypeArguments.Length == 2 &&
	// Don't emit as plain converters if they are value templates
	!x.Left.GetAttributes().Any(a => a.IsValueTemplate()))
	.Select((x, _) => x.Left)
	.Collect();
	var templatizedValues = context.SelectTemplatizedValues()
	.Combine(context.CompilationProvider.Select((x, _) => x.GetTypeByMetadataName(ValueConverterType)))
	.Where(x => x.Left.Template.TTemplate.Is(x.Right))
	.Select((x, _) => x.Left);
	context.RegisterSourceOutput(source.Collect().Combine(converters).Combine(templatizedValues.Collect()), GenerateValueSelector);

StructId/src/StructId.Analyzer/NewtonsoftJsonGenerator.cs

Lines 18 to 19 in ff93ea8

	var source = context.CompilationProvider
	.Select((x, _) => (new KnownTypes(x), x.GetTypeByMetadataName("Newtonsoft.Json.JsonConverter`1")));

StructId/src/StructId.Analyzer/TemplatedGenerator.cs

Lines 81 to 155 in ff93ea8

	public void Initialize(IncrementalGeneratorInitializationContext context)
	{
	var known = context.CompilationProvider
	.Select((x, _) => new KnownTypes(x));
	var templates = context.CompilationProvider
	.SelectMany((x, _) => x.GetAllTypes(includeReferenced: true).OfType<INamedTypeSymbol>())
	.Where(x =>
	// Ensure template is a file-local partial record struct
	x.TypeKind == TypeKind.Struct && x.IsRecord && x.IsFileLocal &&
	// We can only work with templates where we have the actual syntax tree.
	x.DeclaringSyntaxReferences.Any(
	// And we can locate the TStructIdAttribute type that should be applied to it.
	r => r.GetSyntax() is TypeDeclarationSyntax declaration && x.GetAttributes().Any(
	a => a.IsStructIdTemplate())))
	.Combine(known)
	.Select((x, cancellation) =>
	{
	var (tself, known) = x;
	// We infer the idType from the required primary constructor Value parameter type
	var tvalue = (INamedTypeSymbol)tself.GetMembers().OfType<IPropertySymbol>().First(p => p.Name == "Value").Type;
	var attribute = tself.GetAttributes().First(a => a.IsStructIdTemplate());
	// The id type isn't declared in the same file, so we don't do anything fancy with it.
	if (tvalue.DeclaringSyntaxReferences.Length == 0)
	return new Template(tself, tvalue, attribute, known);
	// Otherwise, the idType is a file-local type with a single interface
	var type = tvalue.DeclaringSyntaxReferences[0].GetSyntax(cancellation) as TypeDeclarationSyntax;
	var iface = type?.BaseList?.Types.FirstOrDefault()?.Type;
	if (type == null || iface == null)
	return new Template(tself, tvalue, attribute, known) { OriginalTValue = tvalue };
	if (x.Right.Compilation.GetSemanticModel(type.SyntaxTree).GetSymbolInfo(iface).Symbol is not INamedTypeSymbol ifaceType)
	return new Template(tself, tvalue, attribute, known);
	// if the interface is a generic type with a single type argument that is the same as the idType
	// make it an unbound generic type. We'll bind it to the actual idType later at template render time.
	if (ifaceType.IsGenericType && ifaceType.TypeArguments.Length == 1 && ifaceType.TypeArguments[0].Equals(tvalue, SymbolEqualityComparer.Default))
	ifaceType = ifaceType.ConstructUnboundGenericType();
	return new Template(tself, ifaceType, attribute, known)
	{
	OriginalTValue = tvalue
	};
	})
	.Collect();
	var ids = context.CompilationProvider
	.SelectMany((x, _) => x.Assembly.GetAllTypes().OfType<INamedTypeSymbol>()
	.Where(t => !t.IsValueTemplate() && !t.IsStructIdTemplate()))
	.Where(x => x.IsRecord && x.IsValueType && x.IsPartial())
	.Combine(known)
	.Where(x => x.Left.Is(x.Right.IStructId) || x.Left.Is(x.Right.IStructIdT))
	.Combine(templates)
	.Where(x =>
	{
	var ((id, known), templates) = x;
	var structId = id.AllInterfaces.FirstOrDefault(i => i.Is(known.IStructId) || i.Is(known.IStructIdT));
	return structId != null;
	})
	.SelectMany((x, _) =>
	{
	var ((id, known), templates) = x;
	// Locate the IStructId<TValue> interface implemented by the id
	var structId = id.AllInterfaces.First(i => i.Is(known.IStructId) || i.Is(known.IStructIdT));
	var tid = structId.IsGenericType ? (INamedTypeSymbol)structId.TypeArguments[0] : known.String;
	// If the TValue/Value implements or inherits from the template base type and/or its interfaces
	return templates
	.Where(template => template.AppliesTo(tid))
	.Select(template => new TemplatizedStructId(id, tid, template));
	});
	context.RegisterSourceOutput(ids, GenerateCode);
	}

This model is also completely not incremental:

StructId/src/StructId.Analyzer/KnownTypes.cs

Line 10 in ff93ea8

public record KnownTypes(Compilation Compilation)

Analyzers

Performance issues in this project are not exclusive to generators, but analyzers are also not optimal. However, these should be lower priority than fixing the generators, since at least analyzers don't directly block the IDE (though they should still be fixed).

For instance, here's an offending analyzer:

StructId/src/StructId.Analyzer/RecordAnalyzer.cs

Lines 25 to 42 in ff93ea8

	context.RegisterSyntaxNodeAction(Analyze, SyntaxKind.ClassDeclaration);
	context.RegisterSyntaxNodeAction(Analyze, SyntaxKind.StructDeclaration);
	context.RegisterSyntaxNodeAction(Analyze, SyntaxKind.RecordDeclaration);
	context.RegisterSyntaxNodeAction(Analyze, SyntaxKind.RecordStructDeclaration);
	}
	static void Analyze(SyntaxNodeAnalysisContext context)
	{
	var known = new KnownTypes(context.Compilation);
	if (context.Node is not TypeDeclarationSyntax typeDeclaration ||
	known.IStructIdT is not { } structIdTypeOfT ||
	known.IStructId is not { } structIdType)
	return;
	var symbol = context.SemanticModel.GetDeclaredSymbol(typeDeclaration);
	if (symbol is null)
	return;

Targeting all syntax nodes and then doing GetDeclaredSymbol on each of them is not efficient. You should use an appropriate callback method, like one for an INamedTypeSymbol, and use that. If you need to also compare against well known types, you should gather them in a compilation start action, and then flow them into a nested symbol callback.

Solution

Like I mentioned above, all these generators need to be rewritten to use an attribute as trigger, and use ForAttributeWithMetadataName. Not wanting to use an attribute for convenience is not a valid argument for having a generator that is outright breaking all design principles of incremental generators, and introducing performance issues that will impact the whole IDE.

If it heps, you can check out some other incremental generators and analyzers for reference, such as:

• https://github.com/Sergio0694/ComputeSharp
• https://github.com/CommunityToolkit/dotnet (MVVM Toolkit)
• 🧪 [Experiment] DependencyPropertyGenerator CommunityToolkit/Labs-Windows#624 (DependencyProperty generator)

[kzu]

Thanks for taking a look at this project @Sergio0694! Very much appreciated.

I'll take your analyzer suggestion and see how to refactor that, good point on that front.

On moving to attribute for triggering codegen: having to both implement an interface (or provide a ctor) AND adding an attribute seems detrimental to the user experience (but so would be bad IDE perf). I wouldn't want an API designed to satisfy the compiler at the expense of the API's usability itself, but perhaps a faster analyzer + codefix can make the attribute requirement bearable. I'll investigate further.

Being just a v1, I didn't spend a lot of time polishing perf on the codegen itself, but rather getting to a feature set I could use myself on another project (where perf isn't an issue -yet?- since it's a green field project).

Thanks again for the suggestions and for getting involved. Will keep this issue open and updated (but I'm going on vacation soon-ish so likely it will be some time).

Update

After some experimentation, it seems the advise/recommendation is (at least partially) wrong and shouldn't be considered The True Way. See #60 (comment).

[viceroypenguin]

@kzu When I see a new greenfield project that replicates what already exists, I always like to ask why it exists. So: why does this exist when Vogen and StronglyTypedId exist?

[viceroypenguin]

@kzu I will also reiterate that you have no yet addressed the fact that generators should trigger from an attribute. Please reopen this issue and address this specifically.

[viceroypenguin]

On moving to attribute for triggering codegen: having to both implement an interface (or provide a ctor) AND adding an attribute seems detrimental to the user experience (but so would be bad IDE perf). I wouldn't want an API designed to satisfy the compiler at the expense of the API's usability itself, but perhaps a faster analyzer + codefix can make the attribute requirement bearable. I'll investigate further.

The reality is that the analyzer and codefix performance have absolutely nothing to do with the generator performance. You must address the generator performance issue without any consideration of the performance of analyzers and codefixes.

I wouldn't want an API designed to satisfy the compiler at the expense of the API's usability itself

The reality is that you must satisfy the compiler in this case. It is a requirement. Not doing so will create performance issues for users in IDEs (VS, VS Code, Rider, etc.) on larger codebases.

having to both implement an interface (or provide a ctor) AND adding an attribute

Why do your types need to inherit an interface? Why not just have the attribute and have the generated code add the interface?

[kzu]

I said "partially fixes" and GH closed it. It wasn't intentional.