feat: Add analyzer for real-time IDE feedback #29
Conversation
WalkthroughAdds a new Roslyn analyzer project and corresponding unit-test project, implementing a base analyzer and a TypeStructureAnalyzer that validates CompositeKey-annotated types. Introduces analyzer/test infrastructure, comprehensive analyzer tests, solution and packaging updates, central package references, and lockfiles. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Compilation as Roslyn/Compiler
participant Base as CompositeKeyAnalyzerBase
participant TSA as TypeStructureAnalyzer
participant Common as TypeValidation (Common)
note over Compilation: Types compiled with [CompositeKey] attributes
Compilation->>Base: Register syntax-node actions
Base->>Base: On type declaration (class/record)
Base->>Base: FindCompositeKeyAttribute(typeSymbol)
alt CompositeKey attribute present
Base->>TSA: AnalyzeCompositeKeyType(context, typeDecl, typeSymbol, attr)
TSA->>Common: ValidateTypeForCompositeKey(typeSymbol, decl, semanticModel, ctorAttr)
Common-->>TSA: TypeValidationResult(success/failure, descriptor, args)
alt Failure
TSA->>Base: ReportDiagnostic(context, result, typeDecl, attr, diagType)
Base->>Compilation: Report diagnostic at precise location (type or attribute)
else Success
note right of TSA: No diagnostic reported
end
else No attribute
note right of Base: Skip analysis
end
sequenceDiagram
autonumber
participant TestRunner as xUnit/TestHost
participant Harness as CompositeKeyAnalyzerTestBase
participant RoslynTest as CSharpAnalyzerTest
participant TSA as TypeStructureAnalyzer
TestRunner->>Harness: Configure test (TFM, refs, Directory.Build.props)
Harness->>RoslynTest: Add AdditionalReferences (CompositeKey.dll) and files
TestRunner->>RoslynTest: Provide test source and expected diagnostics
RoslynTest->>TSA: Execute analyzer on source
TSA-->>RoslynTest: Return diagnostics (if any)
RoslynTest-->>TestRunner: Verify diagnostics vs expectations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #29 +/- ##
==========================================
- Coverage 93.88% 93.29% -0.60%
==========================================
Files 28 30 +2
Lines 1096 1163 +67
Branches 160 173 +13
==========================================
+ Hits 1029 1085 +56
- Misses 33 36 +3
- Partials 34 42 +8 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Directory.Packages.props (1)
20-31: Proactively remediate CVE-2024-38095 by centrally pinning System.Formats.Asn1 to a patched version.Your unit-test dependency graph currently resolves System.Formats.Asn1 5.0.0 via older NuGet.* and crypto packages, which is vulnerable to CVE-2024-38095 (high severity). The patched line for .NET 8 is System.Formats.Asn1 8.0.1 or newer. Please add a central pin to a secure version to force transitive upgrade and silence scanners across all TFMs. References: Microsoft advisories confirming 8.0.1+ as fixed. (github.com, nuget.org, advisories.gitlab.com)
Apply this diff to centralise the fix (exact placement within any ItemGroup under this file is fine):
<!-- Testing --> <ItemGroup> <PackageVersion Include="AutoFixture.Xunit2" Version="4.18.1" /> <PackageVersion Include="coverlet.collector" Version="6.0.4" /> <PackageVersion Include="JunitXml.TestLogger" Version="6.1.0" /> <PackageVersion Include="Microsoft.CodeAnalysis" Version="$(AnalyzerRoslynVersion)" /> <PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing" Version="1.1.2" /> <PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.14.1" /> <PackageVersion Include="Shouldly" Version="4.3.0" /> <PackageVersion Include="xunit" Version="2.9.3" /> <PackageVersion Include="xunit.runner.visualstudio" Version="3.1.3" /> + <!-- Security: CVE-2024-38095 patched in 8.0.1+ --> + <PackageVersion Include="System.Formats.Asn1" Version="8.0.2" /> </ItemGroup>If you prefer latest LTS train, 9.0.x is also safe; pick per your repo’s policy (8.0.x for net8-first, 9.0.x if you’re standardising on .NET 9 packages).
🧹 Nitpick comments (17)
src/Directory.Packages.props (1)
26-26: Pinning Analyzer Testing package: consider cross-checking Roslyn version alignment.Good call centralising Microsoft.CodeAnalysis.CSharp.Analyzer.Testing. Given you pin Roslyn to $(AnalyzerRoslynVersion) 4.8.0 elsewhere (Line 25), please validate there are no subtle assembly binding mismatches at runtime/test-time. The lockfile shows this package pulling Workspaces 1.0.1 as a dependency, but NuGet resolves to 4.8.0 via central transitive. That’s fine as long as all assemblies unify to 4.8.0 during test execution. See my verification script in the lockfile comment.
CompositeKey.slnx (1)
6-6: Solution entries look good; double-check folder grouping vs on-disk layout.Adding the analyzer and its unit tests is correct. Minor: the “/tests/” solution folder points at a project that physically lives under src. That’s harmless in .slnx, but consider mirroring folder structure on disk in future to avoid confusion for contributors and tooling.
Also applies to: 12-12
src/CompositeKey.Analyzers.UnitTests/CompositeKey.Analyzers.UnitTests.csproj (1)
16-19: Optional: consider using the xUnit-flavoured analyzer testing package.If your test base doesn’t need it, ignore this; otherwise, Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.XUnit can reduce boilerplate when integrating with xUnit’s assertions and test discoverer.
- <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing" /> + <PackageReference Include="Microsoft.CodeAnalysis.CSharp.Analyzer.Testing.XUnit" />Note: Keep whichever you actually use; don’t include both.
src/CompositeKey/CompositeKey.csproj (1)
34-41: Nice dedup of analyzer payload; small polish suggestion.RemoveDuplicates before authoring Content and _TargetPathsToSymbols is a solid improvement. Optional: add a Condition to guard when PackagePath metadata is missing (defensive for future changes to GetPackAsAnalyzerFiles), to avoid undefined target paths.
- <_TargetPathsToSymbols Include="@(DeduplicatedPackAsAnalyzerFile->WithMetadataValue('IsSymbol', 'true'))" TargetPath="/%(DeduplicatedPackAsAnalyzerFile.PackagePath)" /> + <_TargetPathsToSymbols Include="@(DeduplicatedPackAsAnalyzerFile->WithMetadataValue('IsSymbol', 'true'))" + Condition=" '%(DeduplicatedPackAsAnalyzerFile.PackagePath)' != '' " + TargetPath="/%(DeduplicatedPackAsAnalyzerFile.PackagePath)" />src/CompositeKey.Analyzers.UnitTests/Infrastructure/CompositeKeyAnalyzerTestBase.cs (1)
2-5: Missing using for CSharp types used in the refactor aboveTo compile the proposed change, bring CSharp symbols into scope.
using CompositeKey.Analyzers.Common.Diagnostics; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; using Microsoft.CodeAnalysis.CSharp.Testing; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Testing;src/CompositeKey.Analyzers.UnitTests/Infrastructure/MetadataReferences.cs (2)
1-3: Import System.IO explicitlyPath and File live in System.IO. Add the import to avoid relying on implicit usings.
using Microsoft.CodeAnalysis; using System.Reflection; +using System.IO;
28-36: Harder failure if CompositeKey.dll still isn’t foundMake the fallback more deterministic and produce a clear error when the file truly doesn’t exist.
- if (!File.Exists(compositeKeyPath)) - { - // Fallback: try to find it relative to the test assembly - var compositeKeyAssembly = typeof(CompositeKeyAttribute).Assembly; - compositeKeyPath = compositeKeyAssembly.Location; - } + if (!File.Exists(compositeKeyPath)) + { + // Fallback: try to find it relative to the CompositeKey assembly + var compositeKeyAssembly = typeof(CompositeKeyAttribute).Assembly; + compositeKeyPath = compositeKeyAssembly.Location; + if (!File.Exists(compositeKeyPath)) + { + throw new FileNotFoundException( + $"Could not locate CompositeKey.dll. " + + $"Searched '{testDirectory}' and '{compositeKeyAssembly.Location}'."); + } + }src/CompositeKey.Analyzers/CompositeKey.Analyzers.csproj (2)
29-47: Verify multi-target packaging doesn’t duplicate analyzer binariesPacking analyzers from both net8.0 and netstandard2.0 into analyzers/dotnet[/roslynX.Y]/cs can collide on file names. Consider only packaging the netstandard2.0 output or deduplicating.
Suggested approaches (pick one):
- Include only netstandard2.0 analyzer output in PackAsAnalyzerFile.
- Or, run a dedup step to keep a single copy before packing.
Manual check:
- Run dotnet pack, then list the nupkg contents and ensure a single copy of CompositeKey.Analyzers.dll under analyzers/dotnet…/cs.
16-23: Trim dependency surface for the shipped analyzerDepending on Microsoft.CodeAnalysis.CSharp.Workspaces is often unnecessary for analyzers and increases binding surface. If you don’t use Workspaces APIs, prefer Microsoft.CodeAnalysis and Microsoft.CodeAnalysis.CSharp only.
src/CompositeKey.Analyzers.UnitTests/Infrastructure/TestCodeTemplates.cs (1)
12-17: Optional: allow namespace parameter to avoid type name collisions across testsWhen multiple templates are combined in a single test, repeating UserKey can clash. Consider adding a namespace placeholder or making the record internal in templates.
Example tweak:
- Add a {ns} placeholder and wrap the snippets in namespace {ns};, or change public to internal.
src/CompositeKey.Analyzers/Analyzers/TypeStructureAnalyzer.cs (3)
41-43: Avoid hard-coded metadata name; centralise or cache the symbolMinor maintainability nit: the metadata name string is duplicated knowledge. Consider introducing a well-known constant (shared in a central place) or caching the symbol in a CompilationStartAction to avoid repeated lookups.
Example minimal change (local constant):
+[System.Diagnostics.CodeAnalysis.SuppressMessage("Performance", "CA1802:Use literals where appropriate")] +private const string CompositeKeyCtorAttribute = "CompositeKey.CompositeKeyConstructorAttribute"; ... - var compositeKeyConstructorAttributeType = context.Compilation - .GetTypeByMetadataName("CompositeKey.CompositeKeyConstructorAttribute"); + var compositeKeyConstructorAttributeType = context.Compilation + .GetTypeByMetadataName(CompositeKeyCtorAttribute);If you’d like, I can sketch a CompilationStartAction variant that caches this symbol once per compilation.
52-63: Null-safe reporting flowToday, TypeValidation returns a descriptor on failure, but the guard here assumes it’s non-null. To future-proof, derive the DiagnosticType only when a descriptor exists.
Apply:
- if (!validationResult.IsSuccess) - { - var diagnosticType = GetDiagnosticTypeForDescriptor(validationResult.Descriptor); + if (!validationResult.IsSuccess) + { + var descriptor = validationResult.Descriptor; + var diagnosticType = descriptor is not null + ? GetDiagnosticTypeForDescriptor(descriptor) + : DiagnosticType.Type; ReportDiagnostic( context, validationResult, typeDeclaration, compositeKeyAttribute, diagnosticType); }
70-79: Prefer descriptor identity over string IDs for location mappingRelying on magic strings (“COMPOSITE000x”) is brittle. Since all diagnostics originate from DiagnosticDescriptors in DiagnosticDescriptors, reference-compare against those instances. This is easier to maintain if IDs or messages change.
- private static DiagnosticType GetDiagnosticTypeForDescriptor(DiagnosticDescriptor descriptor) - { - return descriptor.Id switch - { - "COMPOSITE0002" => DiagnosticType.Type, // UnsupportedCompositeType - target type identifier - "COMPOSITE0003" => DiagnosticType.Type, // CompositeTypeMustBePartial - target type identifier - "COMPOSITE0004" => DiagnosticType.Attribute, // NoObviousDefaultConstructor - target attribute since it's about constructor selection - _ => DiagnosticType.Type - }; - } + private static DiagnosticType GetDiagnosticTypeForDescriptor(DiagnosticDescriptor descriptor) + { + if (ReferenceEquals(descriptor, DiagnosticDescriptors.NoObviousDefaultConstructor)) + return DiagnosticType.Attribute; // Attribute location is more actionable for ctor-selection issues + + // UnsupportedCompositeType / CompositeTypeMustBePartial (and any unknowns) default to type identifier + return DiagnosticType.Type; + }src/CompositeKey.Analyzers/Infrastructure/CompositeKeyAnalyzerBase.cs (2)
70-76: Stronger attribute identification (optional)Comparing by ToDisplayString works but is stringly-typed. A sturdier approach is to resolve the CompositeKey attribute symbol once per compilation (CompilationStartAction) and compare with SymbolEqualityComparer.Default. This removes dependency on display formatting and avoids extra allocations.
If you want, I can provide a small refactor that wires a CompilationStartAction into the base to cache well-known symbols and pass them to node actions.
144-148: Ensure compatibility with pre-C# 12 toolchains
The analyzer project doesn’t explicitly set<LangVersion>and will default to the SDK’s C# version (which may be older than 12). C# 12’s collection expression[]inDiagnostic.Create(…, validationResult.MessageArgs ?? [])could fail on toolchains targeting C# 11 or earlier. To maximise compatibility, either:• Replace the
[]fallback withSystem.Array.Empty<object>().
• Or explicitly pin the analyzer’s LangVersion to 12 inCompositeKey.Analyzers.csproj.Updated snippet:
- var diagnostic = Diagnostic.Create( - validationResult.Descriptor, - location, - validationResult.MessageArgs ?? []); + var diagnostic = Diagnostic.Create( + validationResult.Descriptor, + location, + validationResult.MessageArgs ?? System.Array.Empty<object>());src/CompositeKey.Analyzers.UnitTests/Analyzers/TypeStructureAnalyzerTests.cs (2)
176-196: Add test: containing type must also be partialTypeValidation requires every containing type in the chain to be partial. You already test a non-partial nested record; add a case where the nested record is partial but the outer container is not.
Apply:
@@ public async Task NestedNonPartialRecord_ReportsPartialRequired() { @@ } + [Fact] + public async Task PartialNestedRecordInsideNonPartialContainer_ReportsPartialRequired() + { + // Arrange - Container is non-partial; nested record is partial + var test = new TypeStructureAnalyzerTest + { + TestCode = """ + using CompositeKey; + + public class Container + { + [CompositeKey("User_{UserId}")] + public partial record {|COMPOSITE0003:UserKey|}(string UserId); + } + """ + }; + + // Act & Assert + await test.RunAsync(); + }
357-379: Add test: attribute-location precision with multiple attributesEnsure COMPOSITE0004 is anchored to the CompositeKey attribute even when other attributes precede it. This defends the location logic and prevents regressions.
@@ public async Task RecordWithMultiplePublicConstructorsAndNoParameterless_ProducesNoObviousDefaultConstructorDiagnostic() { @@ } + [Fact] + public async Task MultipleAttributes_ConstructorDiagnosticTargetsCompositeKeyAttribute() + { + // Arrange - Obsolete + CompositeKey; diagnostic should point to CompositeKey attribute + var test = new TypeStructureAnalyzerTest + { + TestCode = """ + using System; + using System.Diagnostics.CodeAnalysis; + using CompositeKey; + + [Obsolete] + [{|COMPOSITE0004:CompositeKey("{First}#{Second}")|}] + public partial record Example(Guid First, Guid Second) + { + public Example(Guid first) : this(first, Guid.NewGuid()) { } + } + """ + }; + + // Act & Assert + await test.RunAsync(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
CompositeKey.slnx(1 hunks)src/CompositeKey.Analyzers.UnitTests/Analyzers/TypeStructureAnalyzerTests.cs(1 hunks)src/CompositeKey.Analyzers.UnitTests/CompositeKey.Analyzers.UnitTests.csproj(1 hunks)src/CompositeKey.Analyzers.UnitTests/Infrastructure/CompositeKeyAnalyzerTestBase.cs(1 hunks)src/CompositeKey.Analyzers.UnitTests/Infrastructure/MetadataReferences.cs(1 hunks)src/CompositeKey.Analyzers.UnitTests/Infrastructure/TestCodeTemplates.cs(1 hunks)src/CompositeKey.Analyzers.UnitTests/packages.lock.json(1 hunks)src/CompositeKey.Analyzers/Analyzers/TypeStructureAnalyzer.cs(1 hunks)src/CompositeKey.Analyzers/CompositeKey.Analyzers.csproj(1 hunks)src/CompositeKey.Analyzers/Infrastructure/CompositeKeyAnalyzerBase.cs(1 hunks)src/CompositeKey.Analyzers/packages.lock.json(1 hunks)src/CompositeKey/CompositeKey.csproj(2 hunks)src/Directory.Packages.props(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/CompositeKey.Analyzers/Analyzers/TypeStructureAnalyzer.cs (3)
src/CompositeKey.Analyzers/Infrastructure/CompositeKeyAnalyzerBase.cs (4)
CompositeKeyAnalyzerBase(14-153)AnalyzeCompositeKeyType(40-44)AttributeData(70-76)ReportDiagnostic(133-150)src/CompositeKey.Analyzers.Common/Diagnostics/DiagnosticDescriptors.cs (1)
DiagnosticDescriptors(10-101)src/CompositeKey.Analyzers.Common/Validation/TypeValidation.cs (1)
TypeValidation(10-124)
src/CompositeKey.Analyzers.UnitTests/Infrastructure/CompositeKeyAnalyzerTestBase.cs (1)
src/CompositeKey.Analyzers.UnitTests/Infrastructure/MetadataReferences.cs (1)
MetadataReferences(9-37)
src/CompositeKey.Analyzers.UnitTests/Infrastructure/MetadataReferences.cs (1)
src/CompositeKey/CompositeKeyAttribute.cs (1)
CompositeKeyAttribute(13-44)
src/CompositeKey.Analyzers/Infrastructure/CompositeKeyAnalyzerBase.cs (2)
src/CompositeKey.Analyzers/Analyzers/TypeStructureAnalyzer.cs (3)
DiagnosticAnalyzer(15-80)AnalyzeCompositeKeyType(34-63)DiagnosticType(70-79)src/CompositeKey.Analyzers.Common.UnitTests/TestHelpers/CompilationTestHelper.cs (1)
SemanticModel(39-54)
src/CompositeKey.Analyzers.UnitTests/Analyzers/TypeStructureAnalyzerTests.cs (2)
src/CompositeKey.Analyzers.UnitTests/Infrastructure/TestCodeTemplates.cs (1)
TestCodeTemplates(6-35)src/CompositeKey.Analyzers.UnitTests/Infrastructure/CompositeKeyAnalyzerTestBase.cs (2)
CompositeKeyAnalyzerTestBase(14-47)CompositeKeyAnalyzerTestBase(20-30)
🪛 OSV Scanner (2.1.0)
src/CompositeKey.Analyzers.UnitTests/packages.lock.json
[HIGH] 1-1: System.Formats.Asn1 5.0.0: Microsoft Security Advisory CVE-2024-38095 | .NET Denial of Service Vulnerability
[HIGH] 1-1: System.Formats.Asn1 5.0.0: Microsoft Security Advisory CVE-2024-38095 | .NET Denial of Service Vulnerability
🔇 Additional comments (10)
src/CompositeKey.Analyzers.UnitTests/packages.lock.json (1)
33-42: Roslyn dependency versions are consistentThe verification script confirms that across all
packages.lock.jsonfiles—includingsrc/CompositeKey.Analyzers.UnitTests/packages.lock.json—the only resolved versions for
Microsoft.CodeAnalysis.CommonMicrosoft.CodeAnalysis.Workspaces.CommonMicrosoft.CodeAnalysis.CSharp.Workspacesare
4.8.0. There are no lingering 1.x versions at runtime, so no assembly‐mismatch risk remains and no further changes are required.src/CompositeKey.Analyzers.UnitTests/CompositeKey.Analyzers.UnitTests.csproj (2)
4-9: Multi-targeting and test toggles look good.net9.0/net8.0, IsPackable=false and IsTestProject=true are set correctly. EmitCompilerGeneratedFiles should help when debugging generated code.
36-40: Project references look correct for a black-box analyzer test approach.Referencing CompositeKey, CompositeKey.Analyzers, and Common directly enables end-to-end verification of diagnostics. Good.
src/CompositeKey/CompositeKey.csproj (2)
21-23: Correct packaging wiring for analyzers via ProjectReference.Using PackAsAnalyzer with ReferenceOutputAssembly=false is the right way to ship analyzers alongside the package without polluting the compile graph.
38-41: Ensure analyzer packaging aligns with all target frameworksThe analyzer project at
src/CompositeKey.Analyzers/CompositeKey.Analyzers.csprojis multi-targeted:
<TargetFrameworks>net8.0;netstandard2.0</TargetFrameworks>However, in
src/CompositeKey/CompositeKey.csprojyou’ve gated the analyzer assets to only include items where'%(DeduplicatedPackAsAnalyzerFile.TargetFramework)' == 'netstandard2.0'. This will exclude thenet8.0build outputs. Please confirm which assemblies you intend to ship:
- If you only need the netstandard2.0 outputs (for maximum IDE compatibility), no change is required.
- If you also want to include the net8.0 analyzers, update the
Conditionon your<Content Include=…>element to cover both TFMs (for example:) or remove theCondition=" '%(DeduplicatedPackAsAnalyzerFile.TargetFramework)' == 'netstandard2.0' or '%(DeduplicatedPackAsAnalyzerFile.TargetFramework)' == 'net8.0' "Conditionentirely to include all targets.Please adjust the gating logic (or confirm the current filter) so that your NuGet package contains exactly the analyzer assemblies you intend.
src/CompositeKey.Analyzers.UnitTests/Infrastructure/CompositeKeyAnalyzerTestBase.cs (1)
29-29: Net80 reference assemblies: good default for testsPinning ReferenceAssemblies to .NET 8 avoids binding mismatches and is appropriate here.
src/CompositeKey.Analyzers.UnitTests/Infrastructure/TestCodeTemplates.cs (1)
22-27: LGTM for minimal valid templateClear and concise baseline to exercise the analyser happy-path.
src/CompositeKey.Analyzers/packages.lock.json (1)
1-391: Lockfile looks consistent with the project referencesNo action required. Keep this committed for reproducible restores; update only via restore/upgrade flows.
src/CompositeKey.Analyzers/Analyzers/TypeStructureAnalyzer.cs (1)
21-26: Supported diagnostics look complete and coherentThe three declared descriptors align with the stated scope (unsupported type, partial requirement, constructor selection). No concerns here.
src/CompositeKey.Analyzers.UnitTests/Analyzers/TypeStructureAnalyzerTests.cs (1)
801-832: Sanity checks on SupportedDiagnostics are solidThe API test verifies count and IDs; this will quickly catch accidental regressions. Nicely done.
src/CompositeKey.Analyzers.UnitTests/CompositeKey.Analyzers.UnitTests.csproj
Show resolved
Hide resolved
src/CompositeKey.Analyzers.UnitTests/Infrastructure/CompositeKeyAnalyzerTestBase.cs
Show resolved
Hide resolved
Adds a Roslyn Analyzer along with the source generator to provide near real-time feedback in your IDE for any errors with CompositeKey usage. Currently, the analyzer supports detecting and reporting the following error diagnostics: - COMPOSITE0002 - Unsupported composite type - COMPOSITE0003 - Missing partial modifier on composite types - COMPOSITE0004 - Inaccessible default constructor
DrBarnabus
force-pushed
the
feat/add-initial-roslyn-analyzer
branch
from
3d3c30b to
28b3250
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
81-86: Make Codecov pick up all coverage files and fail fast on upload errorsThe current glob only matches one directory level. Codecov often places coverage under nested GUID folders. Also consider failing the job if upload fails.
- name: Upload Coverage uses: codecov/codecov-action@v4 with: token: ${{ secrets.CODECOV_TOKEN }} - files: ./unit-test-results/*/coverage.opencover.xml + files: ./unit-test-results/**/coverage.opencover.xml + fail_ci_if_error: true
♻️ Duplicate comments (2)
src/CompositeKey.Analyzers/CompositeKey.Analyzers.csproj (1)
37-39: Fix PackAsAnalyzerFile metadata application (currently creates an item with no identity)This line declares a new item that only contains metadata and won’t apply PackagePath to the previously included items. Use Update to mutate the items you just included.
- <PackAsAnalyzerFile PackagePath="$(PackAsAnalyzerPath)/%(TargetPath)" /> + <PackAsAnalyzerFile Update="@(PackAsAnalyzerFile)" PackagePath="$(PackAsAnalyzerPath)/%(TargetPath)" />src/CompositeKey.Analyzers/Infrastructure/CompositeKeyAnalyzerBase.cs (1)
100-122: Use ApplicationSyntaxReference for precise attribute locationRelying on name scanning can misfire with aliases or multiple attributes. AttributeData carries the exact syntax node.
- private static Location? GetAttributeLocation(TypeDeclarationSyntax typeDeclaration, AttributeData attributeData) - { - // Find the attribute syntax that corresponds to this attribute data - var attributeLists = typeDeclaration.AttributeLists; - - foreach (var attributeList in attributeLists) - { - foreach (var attribute in attributeList.Attributes) - { - // Check if this attribute syntax matches our CompositeKey attribute - var name = attribute.Name.ToString(); - if (name.Contains("CompositeKey") || name.Contains("CompositeKeyAttribute")) - { - return attribute.GetLocation(); - } - } - } - - return null; - } + private static Location? GetAttributeLocation(TypeDeclarationSyntax typeDeclaration, AttributeData attributeData) + { + // Best-effort: use the exact syntax reference when available + var syntax = attributeData.ApplicationSyntaxReference?.GetSyntax(); + if (syntax is not null) + return syntax.GetLocation(); + + // Fallback: defensive name-based scan + foreach (var attributeList in typeDeclaration.AttributeLists) + { + foreach (var attribute in attributeList.Attributes) + { + var name = attribute.Name.ToString(); + if (name.Contains("CompositeKey", StringComparison.Ordinal) + || name.Contains("CompositeKeyAttribute", StringComparison.Ordinal)) + { + return attribute.GetLocation(); + } + } + } + return null; + }
🧹 Nitpick comments (5)
.github/workflows/ci.yml (1)
106-111: Use github.ref for branch gating on workflow_dispatchFor workflow_dispatch, github.event.ref can be absent or inconsistent. github.ref is reliably set to refs/heads/.
- - name: Push - if: github.event_name == 'workflow_dispatch' && github.event.ref == 'refs/heads/main' + - name: Push + if: github.event_name == 'workflow_dispatch' && github.ref == 'refs/heads/main'src/CompositeKey.Analyzers/CompositeKey.Analyzers.csproj (1)
49-56: Prefer Item output for GetCommonAssemblyPath to avoid accidental string flatteningMSBuild’s MSBuild task returns TargetOutputs as ITaskItem[]. Assigning to a Property risks semicolon-flattening and losing metadata. Emit an Item and then consume it in the packaging ItemGroup.
- <Target Name="GetCommonAssemblyPath"> - <MSBuild Projects="@(ProjectReference)" - Condition="'%(ProjectReference.Identity)' == '../CompositeKey.Analyzers.Common/CompositeKey.Analyzers.Common.csproj'" - Targets="GetTargetPath" - Properties="TargetFramework=netstandard2.0"> - <Output TaskParameter="TargetOutputs" PropertyName="CommonAssemblyPath" /> - </MSBuild> - </Target> + <Target Name="GetCommonAssemblyPath"> + <MSBuild Projects="@(ProjectReference)" + Condition="'%(ProjectReference.Identity)' == '../CompositeKey.Analyzers.Common/CompositeKey.Analyzers.Common.csproj'" + Targets="GetTargetPath" + Properties="TargetFramework=netstandard2.0"> + <Output TaskParameter="TargetOutputs" ItemName="CommonAssemblyPathItem" /> + </MSBuild> + <PropertyGroup> + <CommonAssemblyPath>@(CommonAssemblyPathItem->'%(FullPath)')</CommonAssemblyPath> + </PropertyGroup> + </Target>Follow-up: the existing Include="$(CommonAssemblyPath)" in the packaging ItemGroup will continue to work after this change.
src/CompositeKey.Analyzers.UnitTests/packages.lock.json (1)
33-41: Mixed CodeAnalysis.Workspaces versions via Analyzer.Testing 1.1.2Microsoft.CodeAnalysis.CSharp.Analyzer.Testing brings an older 1.0.1 Workspaces line while you also depend on 4.8.0 via Microsoft.CodeAnalysis. This is a supported pattern, but it increases restore graph complexity. If you hit binding issues, prefer aligning Analyzer.Testing to the latest minor that targets Roslyn 4.8.
No action required now; just a heads-up for flaky test host failures. If you see “Could not load file or assembly … Workspaces.Common” at runtime, we can bump testing packages accordingly.
Also applies to: 165-177, 961-973
src/CompositeKey.Analyzers/Infrastructure/CompositeKeyAnalyzerBase.cs (2)
30-31: Analyse record structs as wellComposite types declared as record struct won’t be analysed because RecordStructDeclaration isn’t registered.
- context.RegisterSyntaxNodeAction(AnalyzeTypeDeclaration, SyntaxKind.RecordDeclaration, SyntaxKind.ClassDeclaration); + context.RegisterSyntaxNodeAction( + AnalyzeTypeDeclaration, + SyntaxKind.RecordDeclaration, + SyntaxKind.RecordStructDeclaration, + SyntaxKind.ClassDeclaration);
70-76: Attribute discovery is fine; consider fully-qualified matching as a fallbackExact ToDisplayString() equality is good. If you later handle global:: qualifiers, consider using FullyQualifiedFormat and/or SymbolEqualityComparer to be ultra-defensive.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
.github/workflows/ci.yml(2 hunks)CompositeKey.slnx(1 hunks)src/CompositeKey.Analyzers.UnitTests/Analyzers/TypeStructureAnalyzerTests.cs(1 hunks)src/CompositeKey.Analyzers.UnitTests/CompositeKey.Analyzers.UnitTests.csproj(1 hunks)src/CompositeKey.Analyzers.UnitTests/Infrastructure/CompositeKeyAnalyzerTestBase.cs(1 hunks)src/CompositeKey.Analyzers.UnitTests/Infrastructure/MetadataReferences.cs(1 hunks)src/CompositeKey.Analyzers.UnitTests/Infrastructure/TestCodeTemplates.cs(1 hunks)src/CompositeKey.Analyzers.UnitTests/packages.lock.json(1 hunks)src/CompositeKey.Analyzers/Analyzers/TypeStructureAnalyzer.cs(1 hunks)src/CompositeKey.Analyzers/CompositeKey.Analyzers.csproj(1 hunks)src/CompositeKey.Analyzers/Infrastructure/CompositeKeyAnalyzerBase.cs(1 hunks)src/CompositeKey.Analyzers/packages.lock.json(1 hunks)src/CompositeKey/CompositeKey.csproj(2 hunks)src/Directory.Packages.props(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CompositeKey.slnx
🚧 Files skipped from review as they are similar to previous changes (9)
- src/CompositeKey.Analyzers.UnitTests/Analyzers/TypeStructureAnalyzerTests.cs
- src/CompositeKey.Analyzers.UnitTests/Infrastructure/TestCodeTemplates.cs
- src/CompositeKey.Analyzers.UnitTests/Infrastructure/MetadataReferences.cs
- src/CompositeKey.Analyzers/packages.lock.json
- src/CompositeKey.Analyzers.UnitTests/CompositeKey.Analyzers.UnitTests.csproj
- src/CompositeKey/CompositeKey.csproj
- src/CompositeKey.Analyzers.UnitTests/Infrastructure/CompositeKeyAnalyzerTestBase.cs
- src/CompositeKey.Analyzers/Analyzers/TypeStructureAnalyzer.cs
- src/Directory.Packages.props
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/CompositeKey.Analyzers/Infrastructure/CompositeKeyAnalyzerBase.cs (1)
src/CompositeKey.Analyzers/Analyzers/TypeStructureAnalyzer.cs (3)
DiagnosticAnalyzer(15-80)AnalyzeCompositeKeyType(34-63)DiagnosticType(70-79)
🪛 OSV Scanner (2.1.0)
src/CompositeKey.Analyzers.UnitTests/packages.lock.json
[HIGH] 1-1: System.Formats.Asn1 5.0.0: Microsoft Security Advisory CVE-2024-38095 | .NET Denial of Service Vulnerability
[HIGH] 1-1: System.Formats.Asn1 5.0.0: Microsoft Security Advisory CVE-2024-38095 | .NET Denial of Service Vulnerability
🔇 Additional comments (3)
.github/workflows/ci.yml (1)
72-75: Nice split of test suites with per-assembly JUnit logsThe explicit test invocations line up with the new projects and keep result files distinct via {framework}. LGTM.
src/CompositeKey.Analyzers/CompositeKey.Analyzers.csproj (1)
31-34: Good: analyser asset pathing accounts for Roslyn version and languageDynamic PackAsAnalyzerPath layout (analyzers/dotnet[/roslynX][/]) matches the standard pattern. Nice.
src/CompositeKey.Analyzers/Infrastructure/CompositeKeyAnalyzerBase.cs (1)
133-150: LGTM: diagnostic reporting with targeted locationsThe location selection and message args handling is clean, and the fallback to the identifier is sensible.
Adds a Roslyn Analyzer along with the source generator to provide near real-time feedback in your IDE for any errors with CompositeKey usage.
Currently, the analyzer supports detecting and reporting the following error diagnostics:
Summary by CodeRabbit
New Features
Tests
Chores