Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (registration.ImplementationType != null) | ||
| { | ||
| var implementationDefinition = GetGenericDefinition(registration.ImplementationType); | ||
|
|
||
| if (!IsOpenGenericType(implementationDefinition)) | ||
| { | ||
| var diagnostic = Diagnostic.Create( | ||
| DiagnosticDescriptors.OpenGenericImplementationMustBeOpenGeneric, | ||
| location, | ||
| registration.ImplementationType.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat), | ||
| serviceDisplay); | ||
| _context.ReportDiagnostic(diagnostic); | ||
| isValid = false; | ||
| } | ||
| else | ||
| { | ||
| if (implementationDefinition.Arity != registration.ServiceType.Arity) | ||
| { | ||
| var diagnostic = Diagnostic.Create( | ||
| DiagnosticDescriptors.OpenGenericImplementationArityMismatch, | ||
| location, | ||
| registration.ImplementationType.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat), | ||
| serviceDisplay, | ||
| registration.ServiceType.Arity); | ||
| _context.ReportDiagnostic(diagnostic); | ||
| isValid = false; | ||
| } | ||
|
|
||
| if (!IsAssignableToOpenGeneric(registration.ServiceType, implementationDefinition)) | ||
| { | ||
| var diagnostic = Diagnostic.Create( | ||
| DiagnosticDescriptors.OpenGenericImplementationNotAssignable, | ||
| location, | ||
| registration.ImplementationType.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat), | ||
| serviceDisplay); | ||
| _context.ReportDiagnostic(diagnostic); | ||
| isValid = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new open generic validation only compares generic type definitions (IsAssignableToOpenGeneric) before accepting an implementation. That returns true even when the implementation fixes its own type parameters to constants, e.g. class PublisherImpl<T> : IPublisher<int>. Such a registration passes validation because the definitions match (IPublisher<>), yet the generator will try to construct PublisherImpl<string> for IPublisher<string> and emit code that fails to compile. To avoid these false negatives, validation should attempt to construct the implementation with the service’s type arguments (or otherwise ensure its generic parameters flow to the service) rather than comparing definitions alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Can you create a UniTest to validate it?
| } | ||
| else if (factoryMethod.ReturnType is INamedTypeSymbol returnNamedType) | ||
| { | ||
| var returnDefinition = GetGenericDefinition(returnNamedType); | ||
| if (!IsAssignableToOpenGeneric(registration.ServiceType, returnDefinition)) | ||
| { | ||
| var diagnostic = Diagnostic.Create( | ||
| DiagnosticDescriptors.OpenGenericFactoryReturnTypeNotAssignable, | ||
| location, | ||
| factoryMethod.Name, | ||
| returnNamedType.ToDisplayString(SymbolDisplayFormat.CSharpErrorMessageFormat), | ||
| serviceDisplay); | ||
| _context.ReportDiagnostic(diagnostic); | ||
| isValid = false; | ||
| } |
There was a problem hiding this comment.
The factory check only verifies that the return type’s generic definition is assignable to the service. A factory like IPublisher<int> Create<T>() therefore passes validation because IPublisher<int> reduces to IPublisher<>, even though it cannot supply IPublisher<string> and the generator will emit code that does not compile. The validation should ensure the constructed return type matches the requested service type for any type arguments (e.g. by constructing the method with the service’s type parameters and checking the resulting return type) so invalid factories emit JAB0026 instead of breaking code generation.
Useful? React with 👍 / 👎.
AlonTalmi
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces comprehensive support for open generic service registrations by implementing validation logic, diagnostic reporting, and test coverage for various open generic scenarios.
Key Changes:
- Added validation for open generic registrations including instance-based registrations, implementation type compatibility, factory method requirements, and arity matching
- Introduced seven new diagnostic descriptors (JAB0021-JAB0027) covering various open generic validation scenarios
- Extended functional tests to verify open generic resolution via constructors and factory methods
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ServiceProviderBuilder.cs | Core validation logic for open generic registrations with helper methods and diagnostic reporting |
| DiagnosticDescriptors.cs | Seven new error diagnostics for open generic validation scenarios |
| ContainerGenerator.cs | Registration of new diagnostic descriptors with the analyzer |
| DiagnosticsTest.cs | Comprehensive test coverage for each new diagnostic scenario |
| ContainerTests.cs | Functional tests for open generic resolution and factory-based creation |
| Publishing.cs | Test mock interfaces for publisher pattern |
| ServiceImplementationWithGenericServiceInConstructor.cs | Test mock for constructor injection scenario |
| Jab.FunctionalTests.Attributes.csproj | Project reference to include new test mock |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var constructedFactoryMethod = factoryMethod.ConstructedFrom.Construct(serviceTypeArguments, serviceTypeAnnotations); | ||
| if (constructedFactoryMethod.ReturnType is INamedTypeSymbol returnNamedType) | ||
| { | ||
| if (!_context.Compilation.ClassifyConversion(returnNamedType, constructedServiceType).IsImplicit) |
There was a problem hiding this comment.
The constructed factory method validation logic is duplicated with similar patterns in the implementation type validation (lines 1173-1174). Consider extracting the conversion check into a helper method to reduce duplication and improve maintainability.
| @@ -0,0 +1,9 @@ | |||
| namespace JabTests; | |||
|
|
|||
There was a problem hiding this comment.
[nitpick] The IPublisher interface is empty and lacks documentation explaining its purpose as a test mock for open generic publisher pattern validation. Consider adding a comment or summary to clarify its role in the test suite.
| /// <summary> | |
| /// Test mock interface used for validating the open generic publisher pattern in the test suite. | |
| /// </summary> |
…ions # Conflicts: # src/Jab.Tests/DiagnosticsTest.cs # src/Jab/ContainerGenerator.cs
AlonTalmi
force-pushed
the
codex/add-support-for-open-generic-registrations
branch
from
ea5b46c to
bc9324b
Compare
Summary
Testing
dotnet test(fails: dotnet CLI unavailable in container)Codex Task