Skip to content

Add diagnostic analyzer for service registration validation#298

Open
pwelter34 wants to merge 7 commits intomainfrom
claude/add-source-generator-diagnostics-wB78i
Open

Add diagnostic analyzer for service registration validation#298
pwelter34 wants to merge 7 commits intomainfrom
claude/add-source-generator-diagnostics-wB78i

Conversation

@pwelter34
Copy link
Copy Markdown
Member

Summary

Introduces a new diagnostic analyzer (ServiceRegistrationAnalyzer) that validates Injectio service registration attributes and methods at compile-time, providing developers with immediate feedback on configuration errors.

Key Changes

  • New ServiceRegistrationAnalyzer class: A Roslyn-based diagnostic analyzer that validates:

    • [RegisterServices] method signatures (must have IServiceCollection as first parameter, optional string collection as second)
    • Service registration attributes ([RegisterSingleton], [RegisterScoped], [RegisterTransient])
    • Factory method existence, static modifier, and signature validation
    • Service type assignability (implementation must implement/inherit from declared service types)
    • Abstract type instantiation without factory methods
    • Non-static [RegisterServices] methods on abstract classes
  • New DiagnosticDescriptors class: Defines 9 diagnostic rules (INJECT0001-INJECT0009) with appropriate severity levels and user-friendly messages

  • Comprehensive test suite (ServiceRegistrationDiagnosticTests): 14 test cases covering:

    • Invalid method signatures and parameters
    • Factory method validation scenarios
    • Service type mismatch detection
    • Valid registration patterns (no false positives)

Implementation Details

  • Analyzer registers symbol actions for both SymbolKind.Method and SymbolKind.NamedType
  • Validates generic type arguments and named parameters from registration attributes
  • Supports multiple registration strategies (Self, ImplementedInterfaces, SelfWithInterfaces, SelfWithProxyFactory)
  • Uses fully qualified type names with nullable reference type modifiers for accurate type comparison
  • Validates factory method signatures support both keyed and non-keyed service registration patterns

https://claude.ai/code/session_01FpKv78qmKTTnNZAGyXxGwv

claude added 6 commits April 2, 2026 20:30
Introduce compile-time diagnostics (INJECT0001-INJECT0009) that warn about
invalid RegisterServices method signatures, missing/invalid factory methods,
service-implementation type mismatches, and abstract type issues. Diagnostics
are captured as equatable DiagnosticInfo records during the semantic transform
phase to maximize incremental generator pipeline caching.

https://claude.ai/code/session_01FpKv78qmKTTnNZAGyXxGwv
Reporting diagnostics inside the source generator prevents pipeline
caching from working optimally. Move all validation into a dedicated
ServiceRegistrationAnalyzer (DiagnosticAnalyzer) that runs independently,
keeping the generator focused on pure code generation.

- Add ServiceRegistrationAnalyzer with RegisterSymbolAction for methods
  and named types
- Revert ServiceRegistrationGenerator to original (no diagnostic logic)
- Remove DiagnosticInfo.cs (no longer needed without generator-side
  serialization)
- Revert ServiceRegistrationContext (remove Diagnostics property)
- Update tests to use CompilationWithAnalyzers

https://claude.ai/code/session_01FpKv78qmKTTnNZAGyXxGwv
Move duplicated helper methods (IsMethodAttribute, IsKnownAttribute,
IsServiceCollection, IsStringCollection, IsServiceProvider,
ResolveRegistrationStrategy, FullyQualifiedNullableFormat) into a
shared SymbolHelpers class used by both the generator and analyzer.

https://claude.ai/code/session_01FpKv78qmKTTnNZAGyXxGwv
Update package reference from xunit 2.9.3 to xunit.v3 1.1.0 to match
the v3 runner already in use. Replace Xunit.Abstractions usings with
Xunit (ITestOutputHelper moved in v3).

https://claude.ai/code/session_01FpKv78qmKTTnNZAGyXxGwv
…generic validation

- Replace Verify.Xunit (pulls xunit v2 transitively) with Verify.XunitV3
- Remove xunit.runner.visualstudio (bundled in xunit v3)
- Add AnalyzerReleases.Shipped.md and AnalyzerReleases.Unshipped.md for
  RS2008 release tracking compliance
- Move ToUnboundGenericType to shared SymbolHelpers
- Fix false positive INJECT0007 for open generic registrations by
  comparing unbound generic forms of interfaces and base types

https://claude.ai/code/session_01FpKv78qmKTTnNZAGyXxGwv
…on warnings

- Remove ITestOutputHelper parameter from test base classes (XUnit.Hosting
  v4 TestHostBase constructor takes only the fixture)
- Use SymbolEqualityComparer.Default.Equals instead of != for ISymbol
  comparisons to fix RS1024 warnings

https://claude.ai/code/session_01FpKv78qmKTTnNZAGyXxGwv
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a Roslyn diagnostic analyzer to validate Injectio service registration patterns at compile time, and updates the test projects to use xUnit v3 while refactoring shared symbol/type helpers used by the generator/analyzer.

Changes:

  • Added ServiceRegistrationAnalyzer + DiagnosticDescriptors (INJECT0001–INJECT0009) and Roslyn analyzer release tracking files.
  • Refactored generator symbol/type utilities into new SymbolHelpers and updated ServiceRegistrationGenerator to use it.
  • Added analyzer-focused unit tests and migrated test projects/packages to xUnit v3 + Verify.XunitV3.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/Injectio.Tests/ServiceRegistrationDiagnosticTests.cs New tests validating analyzer diagnostics for common invalid/valid registration patterns
tests/Injectio.Tests/Injectio.Tests.csproj Switch test dependencies to xUnit v3 and Verify.XunitV3
tests/Injectio.Acceptance.Tests/LocalServiceTests.cs Update constructor/base usage to align with updated test host pattern
tests/Injectio.Acceptance.Tests/LibraryServiceTests.cs Update constructor/base usage to align with updated test host pattern
tests/Injectio.Acceptance.Tests/Injectio.Acceptance.Tests.csproj Switch acceptance tests to xUnit v3
tests/Injectio.Acceptance.Tests/DependencyInjectionBase.cs Remove ITestOutputHelper plumbing and update TestHostBase inheritance
src/Injectio.Generators/SymbolHelpers.cs New shared symbol/type helper utilities extracted for generator/analyzer use
src/Injectio.Generators/ServiceRegistrationGenerator.cs Replace in-file helpers with SymbolHelpers usages
src/Injectio.Generators/ServiceRegistrationAnalyzer.cs New analyzer implementing service/module registration validation diagnostics
src/Injectio.Generators/Injectio.Generators.csproj Include analyzer release tracking files as AdditionalFiles
src/Injectio.Generators/DiagnosticDescriptors.cs New centralized diagnostic descriptors for analyzer rules
src/Injectio.Generators/AnalyzerReleases.Unshipped.md New (unshipped) analyzer rule tracking entries
src/Injectio.Generators/AnalyzerReleases.Shipped.md New shipped analyzer rule tracking stub
Directory.Packages.props Update package versions for Verify + xUnit v3 and remove xUnit v2 packages

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +105 to +125
public static bool IsStringCollection(IParameterSymbol parameterSymbol)
{
var type = parameterSymbol?.Type as INamedTypeSymbol;

return type is
{
Name: "IEnumerable" or "IReadOnlySet" or "IReadOnlyCollection" or "ICollection" or "ISet" or "HashSet",
IsGenericType: true,
TypeArguments.Length: 1,
TypeParameters.Length: 1,
ContainingNamespace:
{
Name: "Generic",
ContainingNamespace:
{
Name: "Collections",
ContainingNamespace.Name: "System"
}
}
};
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsStringCollection only checks that the parameter type is a generic collection type, but it never verifies that the generic argument is string. This will incorrectly treat IEnumerable<int> (etc.) as a valid tags parameter for [RegisterServices], and both the generator and analyzer will then accept/assume string tags.

Copilot uses AI. Check for mistakes.
if (context.Symbol is not INamedTypeSymbol classSymbol)
return;

if (classSymbol.IsAbstract || classSymbol.IsStatic)
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AnalyzeNamedType returns early for classSymbol.IsAbstract, which makes the INJECT0008 (abstract implementation type) check in AnalyzeRegistrationAttribute unreachable for attributes applied to abstract types. If the intent is to warn on abstract registrations, remove/adjust this early return and ensure the diagnostic is emitted for abstract types without a factory.

Suggested change
if (classSymbol.IsAbstract || classSymbol.IsStatic)
if (classSymbol.IsStatic)

Copilot uses AI. Check for mistakes.
location,
factoryMethodName,
className));
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ValidateFactoryMethod enforces parameter count and that the first parameter is IServiceProvider, but it does not validate the type of the optional second parameter (the descriptor message says it must be object?). As written, Factory(IServiceProvider, int) would be accepted; please validate the second parameter type when Parameters.Length == 2.

Suggested change
}
}
if (method.Parameters.Length == 2 &&
method.Parameters[1].Type.SpecialType != SpecialType.System_Object)
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.FactoryMethodInvalidSignature,
location,
factoryMethodName,
className));
}

Copilot uses AI. Check for mistakes.
Comment on lines +275 to +305
foreach (var method in factoryMethods)
{
if (!method.IsStatic)
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.FactoryMethodNotStatic,
location,
factoryMethodName,
className));
return;
}

if (method.Parameters.Length is not (1 or 2))
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.FactoryMethodInvalidSignature,
location,
factoryMethodName,
className));
return;
}

if (!SymbolHelpers.IsServiceProvider(method.Parameters[0]))
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.FactoryMethodInvalidSignature,
location,
factoryMethodName,
className));
}
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Factory method validation reports diagnostics based on the first overload encountered and can false-positive when multiple overloads share the same name (e.g., one non-static overload plus one valid static overload). Consider changing the logic to: (1) find at least one overload that is static and has a valid signature, and only (2) report a diagnostic if no valid overload exists.

Suggested change
foreach (var method in factoryMethods)
{
if (!method.IsStatic)
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.FactoryMethodNotStatic,
location,
factoryMethodName,
className));
return;
}
if (method.Parameters.Length is not (1 or 2))
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.FactoryMethodInvalidSignature,
location,
factoryMethodName,
className));
return;
}
if (!SymbolHelpers.IsServiceProvider(method.Parameters[0]))
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.FactoryMethodInvalidSignature,
location,
factoryMethodName,
className));
}
}
var hasStaticOverload = false;
foreach (var method in factoryMethods)
{
if (!method.IsStatic)
continue;
hasStaticOverload = true;
if (method.Parameters.Length is not (1 or 2))
continue;
if (!SymbolHelpers.IsServiceProvider(method.Parameters[0]))
continue;
return;
}
context.ReportDiagnostic(Diagnostic.Create(
hasStaticOverload
? DiagnosticDescriptors.FactoryMethodInvalidSignature
: DiagnosticDescriptors.FactoryMethodNotStatic,
location,
factoryMethodName,
className));

Copilot uses AI. Check for mistakes.
Comment on lines +230 to +237
// validate abstract implementation type without factory
if (classSymbol.IsAbstract && implementationFactory.IsNullOrWhiteSpace() && implTypeName == classSymbol.ToDisplayString(SymbolHelpers.FullyQualifiedNullableFormat))
{
context.ReportDiagnostic(Diagnostic.Create(
DiagnosticDescriptors.AbstractImplementationType,
location,
implTypeName));
}
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There’s no test exercising INJECT0008 (abstract implementation type without a factory). Since this analyzer rule is newly introduced, it should have a dedicated test case to prevent regressions once the abstract-type analysis logic is fixed/updated.

Copilot uses AI. Check for mistakes.
- Remove IsAbstract early return in AnalyzeNamedType so INJECT0008 is
  reachable for abstract types with registration attributes
- Rewrite factory validation to find any valid overload before reporting,
  avoiding false positives with multiple overloads
- Validate factory second parameter is object? (not arbitrary types)
- Add string type argument check to IsStringCollection (reject
  IEnumerable<int> etc.)
- Add tests for INJECT0008 (abstract type with/without factory)
- Fix TestHostBase constructor for XUnit.Hosting v4 (single fixture
  parameter, no ITestOutputHelper)

https://claude.ai/code/session_01FpKv78qmKTTnNZAGyXxGwv
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants