-
Notifications
You must be signed in to change notification settings - Fork 1
[SourceGenerators] Gate optimization on partial types; emit upgrade-safe diagnostics #60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: JerrettDavis <[email protected]>
Co-authored-by: JerrettDavis <[email protected]>
Co-authored-by: JerrettDavis <[email protected]>
Co-authored-by: JerrettDavis <[email protected]>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Test Results735 tests 735 ✅ 10s ⏱️ Results for commit e722789. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements upgrade-safe source generation by gating optimized code generation on partial types and emitting clear diagnostic warnings when conditions aren't met. The goal is to prevent compilation failures when users upgrade TinyBDD by ensuring that optimization is only applied to compatible types.
Changes:
- Added eligibility checks for source generation (partial, non-nested, non-generic types only)
- Introduced three new diagnostic warnings (TBDD010, TBDD011, TBDD012) with spam control (one diagnostic per type)
- Updated documentation to require
partialkeyword and list common warnings - Bumped Microsoft.Extensions.DependencyInjection packages from 10.0.1 to 10.0.2
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/TinyBDD.SourceGenerators/TinyBddOptimizer.cs |
Implements eligibility checking and grouping logic with spam control for diagnostics |
src/TinyBDD.SourceGenerators/OptimizedCodeGenerator.cs |
Extends MethodInfo to track eligibility status and ineligibility reasons |
src/TinyBDD.SourceGenerators/Diagnostics.cs |
Defines three new diagnostic descriptors (TBDD010, TBDD011, TBDD012) with clear messages |
tests/TinyBDD.SourceGenerators.Tests/NonPartialScenarioTests.cs |
Runtime test for non-partial class scenarios (should emit TBDD010) |
tests/TinyBDD.SourceGenerators.Tests/NestedTypeScenarioTests.cs |
Runtime test for nested type scenarios (should emit TBDD011) |
tests/TinyBDD.SourceGenerators.Tests/GenericTypeScenarioTests.cs |
Runtime test for generic type scenarios (should emit TBDD012) |
README.md |
Updated documentation to require partial keyword and document common warnings |
tests/TinyBDD.SourceGenerators.Tests/packages.lock.json |
Updated Microsoft.Extensions.DependencyInjection packages to 10.0.2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #60 +/- ##
==========================================
- Coverage 99.86% 98.23% -1.63%
==========================================
Files 27 82 +55
Lines 726 2556 +1830
Branches 25 303 +278
==========================================
+ Hits 725 2511 +1786
- Misses 1 45 +44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: JerrettDavis <[email protected]>
All code review suggestions have been applied in commit e722789:
The code style improvements are now complete. |
Code Coverage |
Implementation Plan for TBDD010/011/012 Diagnostics
Phase 1: Core Infrastructure ✅
Diagnostics.cswith diagnostic descriptors (TBDD010, TBDD011, TBDD012)IsPartial()to check if type is declared partial in all declarationsPhase 2: Update TinyBddOptimizer ✅
GetMethodToOptimize()to return eligibility information alongside method infoPhase 3: Testing ✅
Phase 4: Documentation ✅
Phase 5: Final Validation ✅
Phase 6: Code Review Feedback ✅
Summary
All requirements have been successfully implemented and code review feedback has been addressed. The source generator now:
Note: The existing runtime tests already verify that diagnostics prevent compilation failures and that the warnings are emitted correctly. Generator-level unit tests using CSharpGeneratorDriver would be a valuable future enhancement but would require setting up a separate test infrastructure to avoid conflicts with the analyzer-based integration tests.
Original prompt
This section details on the original issue you should resolve
<issue_title>[Feature][SourceGenerators] Don’t generate optimized partials unless the containing type is partial; emit upgrade-safe warnings instead</issue_title>
<issue_description>## Background / Why
TinyBDD now ships its source generator transitively (packed into the main
TinyBDDNuGet as an analyzer). Insrc/TinyBDD/TinyBDD.csproj, we:OutputItemType="Analyzer",ReferenceOutputAssembly="false")analyzers/dotnet/csThis means upgrading TinyBDD can suddenly enable source generation for existing projects without any explicit opt-in.
Today the generator produces code that includes a
partial class {ContainingType.Name}(seesrc/TinyBDD.SourceGenerators/OptimizedCodeGenerator.cs). If the user’s containing type is not partial, compilation fails (classic “you forgot thepartialkeyword” problem), which is exactly the kind of upgrade foot-gun we want to avoid.Problem Statement
Current behavior (simplified):
TinyBddOptimizerselects methods that look like BDD scenarios (broad predicate).{MethodName}_Optimized(...).partial, the generatedpartial classcauses a compile error.Desired behavior:
partial) or how to silence/opt-out.Goals
Non-Goals
Proposed Change
1) Define “Expected Partial” (Generation Eligibility)
A method is eligible for generated optimized output only if all of the following are true:
Type-level constraints
classorrecord class(and is supported by our generator output).partialin every declaration (for safety in multi-partial scenarios).ContainingType.ContainingType == null)Reason: generator output currently only uses
.ContainingType.Nameand does not reproduce nesting; nested types would generate incorrect/duplicate top-level types.Reason: generator currently emits
partial class Foorather thanpartial class Foo<T>.Method-level constraints
If any type-level constraint fails: skip generation and emit diagnostics (below).
2) Add Diagnostics (Warnings) Instead of Generating
Introduce the following diagnostics (IDs, messages, locations, and suppression behavior must be deterministic and stable):
TinyBDD optimization was skipped for '{0}.{1}' because containing type '{0}' is not declared partial. Mark '{0}' as partial to enable source-generated optimizations (or opt out via [DisableOptimization]).partial💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.