-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix STJ source generator for partial class contexts with JsonSerializable attributes on multiple declarations #123417
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
base: main
Are you sure you want to change the base?
Conversation
…n multiple declarations When a JsonSerializerContext is defined across multiple partial class declarations with [JsonSerializable] attributes on different partials, the source generator now correctly generates a single combined context containing all types from all partial declarations, instead of failing with duplicate hintName errors. The fix introduces IsCanonicalPartialDeclaration() which picks a deterministic "canonical" partial (first alphabetically by file path) to process all attributes, while other partials return null to avoid duplicate generation. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Address code review comments by clarifying edge case behavior for empty file paths and explaining the fallback logic when no attributes are found. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
eiriktsarpalis
left a comment
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.
@copilot please also add a test that validates the run time behavior of the generated code, i.e. checking that all types across all partials have been included.
|
@copilot please continue work on my previous feedback |
|
@copilot try again please |
|
@copilot, try again |
|
@copilot please address the remaining feedback |
|
@copilot can you try again? |
|
@copilot try again |
|
@copilot try again |
|
@copilot you still there? |
…e declarations - Fixed issue number in unit test comment (97460 -> 99669) - Added explicit file paths to unit test syntax trees to properly test canonical partial selection - Added runtime test that validates serialization/deserialization works for types from both partial declarations - Created PartialContextTests.Part1.cs and PartialContextTests.Part2.cs with separate partial declarations - Removed incomplete test types from ContextClasses.cs (they were not being used) Co-authored-by: eiriktsarpalis <2813363+eiriktsarpalis@users.noreply.github.com>
Yes! I've addressed the remaining feedback in commit b1d515d:
|
The test was failing on .NET Framework because it was removing all syntax trees from the base compilation, which also removed the polyfill attributes needed for netfx. Now the test preserves those trees while adding the test-specific syntax trees with explicit file paths. Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Head branch was pushed to by a user without write access
Description
STJ source generator fails when
JsonSerializerContextis split across multiple partial declarations with[JsonSerializable]attributes on each:Root cause:
ForAttributeWithMetadataNametriggers perClassDeclarationSyntaxwith the attribute, butcontextClassSymbol.GetAttributes()returns ALL attributes from ALL partial declarations. This causes duplicateAddSourcecalls with identical hintNames.Fix: Added
IsCanonicalPartialDeclaration()to select a single partial (first alphabetically by file path) to generate code for ALL types. Non-canonical partials return null fromParseContextGenerationSpec.Changes
JsonSourceGenerator.Parser.cs: Added canonical partial detection viaIsCanonicalPartialDeclaration(), integrated check beforeParseJsonSerializerContextAttributesJsonSourceGeneratorTests.cs: AddedPartialContextClassWithAttributesOnMultipleDeclarationsunit test with explicit file paths (File1.cs,File2.cs) to properly test canonical partial selection. The test properly preserves base compilation syntax trees (including netfx polyfill attributes) for cross-platform compatibility.PartialContextTests.Part1.cs/PartialContextTests.Part2.cs: Created separate partial declaration files for runtime testingJsonSerializerContextTests.cs: AddedPartialContextWithAttributesOnMultipleDeclarations_RuntimeBehaviortest that validates serialization/deserialization works correctly for types from both partial declarationsAll source generator unit tests (including Roslyn 3.11 on .NET Framework) and runtime tests pass.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.