Add support for generic constraints to Actor source generator#1535
Add support for generic constraints to Actor source generator#1535oising wants to merge 7 commits intodapr:masterfrom
Conversation
Signed-off-by: Oisin Grehan <oisin.grehan@ionodes.com>
aa000f9 to
087f11b
Compare
Signed-off-by: Oisin Grehan <oisin.grehan@ionodes.com>
1fb41e7 to
435933c
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for generic type constraints to the Actor source generator, enabling it to properly handle and generate constraint clauses for generic actor interfaces. The generator now correctly preserves constraints like where T: class, where T: struct, where T: unmanaged, interface constraints, where T: new(), and where T: notnull when creating actor client classes.
- Implemented constraint clause generation for all supported generic constraint types
- Added logic to iterate through type parameters and build appropriate constraint syntax
- Added comprehensive test coverage for both single and multiple type parameter scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Dapr.Actors.Generators/ActorClientGenerator.cs | Implements constraint clause generation logic for generic type parameters in the actor client generator |
| test/Dapr.Actors.Generators.Test/ActorClientGeneratorTests.cs | Adds test cases for single and multiple type parameter constraints to verify the generator works correctly |
| // Add unmanaged constraint | ||
| if (typeParam.HasUnmanagedTypeConstraint) | ||
| { | ||
| constraints.Add(SyntaxFactory.TypeConstraint(SyntaxFactory.IdentifierName("unmanaged"))); |
There was a problem hiding this comment.
The unmanaged constraint should be added before type constraints but after class/struct constraints. The current order may not follow C# constraint ordering requirements where 'unmanaged' should come before interface constraints.
There was a problem hiding this comment.
@copilot Reevaluate this and suggest a fix using the information at https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/generics/constraints-on-type-parameters
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Oisin Grehan <oising@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public sealed class TestActorClient<TTrait> : Test.ITestActor<TTrait> where TTrait : Test.ITrait | ||
| { |
There was a problem hiding this comment.
The expected generated code has inconsistent type parameter names: the class is declared as TestActorClient<TTrait> but the constraint clause refers to TTrait. These must match (and should likely use the same type parameter identifier as the source interface).
| [GenerateActorClient] | ||
| public interface ITestActor<TTrait, TValidator> | ||
| where TTrait : ITrait, new() | ||
| where TValidator : class, IValidator<TTrait> | ||
| { |
There was a problem hiding this comment.
This test input source has a typo in the constraint clause: the interface type parameter is TTrait, but the where clause uses TTrait. As written, the originalSource won’t compile, so the test won’t validate constraint copying.
| public System.Threading.Tasks.Task<TTrait> GetValidatedTrait(string name) | ||
| { | ||
| return this.actorProxy.InvokeMethodAsync<string, TTrait>(""GetValidatedTrait"", name); | ||
| } |
There was a problem hiding this comment.
The expected generated code uses InvokeMethodAsync<string, TTrait>(...), but TTrait isn’t a type parameter on the generated class (it’s TTrait). This mismatch will make the expected output invalid and fail the snapshot comparison once the generator is fixed.
| if (typeParam.HasReferenceTypeConstraint || | ||
| typeParam.HasValueTypeConstraint || | ||
| typeParam.HasUnmanagedTypeConstraint || | ||
| typeParam.HasNotNullConstraint || |
There was a problem hiding this comment.
The condition deciding whether to emit a where ... clause doesn’t include HasConstructorConstraint. If a type parameter only has new() (no other constraints), no constraint clause will be generated, even though the PR claims new() is supported.
| typeParam.HasNotNullConstraint || | |
| typeParam.HasNotNullConstraint || | |
| typeParam.HasConstructorConstraint || |
| // Add type constraints (e.g., where T : IInterface) | ||
| foreach (var constraintType in typeParam.ConstraintTypes) | ||
| { | ||
| constraints.Add(SyntaxFactory.TypeConstraint( | ||
| SyntaxFactory.ParseTypeName(constraintType.ToString()))); | ||
| } | ||
|
|
||
| // Add notnull constraint | ||
| if (typeParam.HasNotNullConstraint) | ||
| { | ||
| constraints.Add(SyntaxFactory.TypeConstraint(SyntaxFactory.IdentifierName("notnull"))); | ||
| } |
There was a problem hiding this comment.
Constraint ordering here can produce invalid C#: notnull is appended after the type/interface constraints, but C# requires special constraints (class/struct/unmanaged/notnull) to come before any type constraints, with new() last. Reorder the list so notnull is emitted before ConstraintTypes (and keep new() last).
| public interface ITestActor<in TTrait> where TTrait : ITrait | ||
| { | ||
| Task<bool> SetTrait(TTrait trait); | ||
| Task<ITrait> GetTrait(string name); | ||
| } |
There was a problem hiding this comment.
The new test input source won’t compile: the type parameter is TTrait (ITestActor<in TTrait>), but SetTrait uses TTrait as the parameter type. This should reference the declared type parameter name consistently so the generator test is exercising constraint copying rather than failing to compile the input.
Description
The actor source generator will now correctly add any type constraints to the client class. Supported:
<class or interface>...and any legal combinations thereof.
I've added tests for a single case (single type parameter and one constraint) and a complex case (multiple type parametes and constraints.)
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #1534
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: