Fixed codegen for IEnumerable<T> binding (#123422)#123663
Fixed codegen for IEnumerable<T> binding (#123422)#123663tarekgh merged 3 commits intodotnet:mainfrom
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-extensions-configuration |
|
@dotnet-policy-service agree company="Growings RobotX Ltd." |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a regression in the configuration binder's source generator where binding IEnumerable<T> in nested positional records incorrectly threw an InvalidOperationException. The issue occurred because the code generation logic for handling empty collections and error conditions only checked for ArraySpec types, but not for IEnumerable<T> types.
Changes:
- Updated the condition for emitting exception-throwing code to handle both
ArraySpecand types withIsExactIEnumerableOfTproperty - Added a test case to verify that nested
IEnumerable<T>in positional records can be bound successfully
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs | Fixed the condition that determines when to emit if (member is null) vs else for exception throwing, now including IEnumerable<T> types alongside arrays |
| src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs | Added test case TestBindingNestedIEnumerable to verify the fix works for the reported scenario |
| src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs | Added test classes ContainingIEnumerable and NestedWithIEnumerable to support the new test case |
...libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs
Show resolved
Hide resolved
dmitry-kandiner
force-pushed
the
bugfix/issue-#123422
branch
2 times, most recently
from
88da365 to
ee73a21
Compare
jkotas
changed the title
Binding of an IEnumerable<T> in a nested positional record resulted in throwing of an InvalidOperationException from source generated code. The condition for emitting the code throwing the exception only worked with Arrays, and this fix applies the same logic for IEnumerable<T> as well. Fix #123422 Signed-off-by: Dmitry Kandiner <[email protected]>
Signed-off-by: Dmitry Kandiner <[email protected]>
dmitry-kandiner
force-pushed
the
bugfix/issue-#123422
branch
from
ee73a21 to
c23c554
Compare
|
@dmitry-kandiner please don't force push again because this reset the review. Thanks! |
Oops! I assumed not being lined up to the target branch will prevent merging. My mistake. |
No worries. We don't have to do that except if there is a merge conflict. Otherwise, every change in the PR will reset the CI run and takes longer to finish. |
|
@tarekgh could you please trigger re-build of the timed out tests? |
|
/azp run |
|
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
Supported commands
See additional documentation. |
|
/azp run runtime |
|
Commenter does not have sufficient privileges for PR 123663 in repo dotnet/runtime |
|
@dmitry-kandiner don't worry about the CI and don't have to merge your branch to main as you did, Anyway, I'll follow up and get this merged. I just need to do some local testing before merging it. Thanks again! |
|
@tarekgh Thank you! Apparently, the checks fail due to timeout, but I have no means to check what's going on exactly. |
|
Yes, the timeout is known issue to us. CC @markwilkie |
|
/ba-g the timeout on NativeAOT is unrelated and known issue. |
|
/backport to release/10.0 |
|
Started backporting to |
…23720) Backport of #123663 to release/10.0 /cc @tarekgh @dmitry-kandiner ## Customer Impact - [x] Customer reported - [ ] Found internally Using configuration source generator, when a user binds configuration to a class or record with a primary constructor, and one of the constructor parameters is exact of type `IEnumerable<T>`, the binding throws a `System.InvalidOperationException`, even though the binding is expected to succeed. The issue #123422 shows the customer report and more details. ## Regression - [x] Yes - [ ] No This is a regression in .NET 10.0.1. A previous servicing fix addressed several crashes, but one case was missed. That omission surfaced as this regression. The change that introduced the regression is #121325. ## Testing Manually verified the failing scenario and confirmed the fix resolves the issue. Added a regression test covering the previously failing case and ran the full test suite successfully. Reviewed related code paths to ensure no additional cases were missed. ## Risk Low, the change is targeting the specific case with specific type. The fix is localized to the failing scenario. --------- Signed-off-by: Dmitry Kandiner <[email protected]> Co-authored-by: Dmitry Kandiner <[email protected]>
3 participants
Binding of an IEnumerable in a nested positional record resulted in throwing of an InvalidOperationException from source generated code. The condition for emitting the code throwing the exception only worked with Arrays, and this fix applies the same logic for IEnumerable as well.
Fix #123422