-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix TagList.CopyTo to not throw when TagList is empty #122849
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: stephentoub <2642209+stephentoub@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-system-diagnostics-activity |
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 PR fixes a bug where TagList.CopyTo(array, arrayIndex) incorrectly throws an ArgumentOutOfRangeException when called on an empty TagList with an empty array, making it inconsistent with standard collection semantics and with ActivityTagsCollection.CopyTo.
Key Changes:
- Modified
CopyTo(KeyValuePair<string, object?>[] array, int arrayIndex)to skip bounds checking when_tagsCount == 0 - Added comprehensive test coverage for empty
TagListoperations includingCopyTovariants andToArray() - Extended existing
TestCopyToto exerciseICollection<T>.CopyTointerface method for all sizes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/TagList.netcore.cs |
Wraps the arrayIndex bounds check in a conditional that only executes when the TagList is non-empty |
src/libraries/System.Diagnostics.DiagnosticSource/tests/TagListTests.cs |
Adds TestEmptyTagListCopyTo test for empty TagList scenarios and enhances TestCopyTo to test ICollection<T>.CopyTo |
...raries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/TagList.netcore.cs
Show resolved
Hide resolved
...raries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/TagList.netcore.cs
Show resolved
Hide resolved
|
@copilot, please address the review feedback |
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Addressed in ece9db1:
|
|
@copilot need to apply the change to the file |
Co-authored-by: tarekgh <10833894+tarekgh@users.noreply.github.com>
Applied the same fix to |
|
/ba-g dead letter |
Description
TagList.CopyTo(array, arrayIndex)throwsArgumentOutOfRangeExceptionwhen the TagList is empty and an empty array is passed. This is inconsistent withActivityTagsCollection.CopyToand standard collection semantics.Fix: Allow
arrayIndex == array.Lengthwhen the TagList is empty (since nothing will be copied), while still validating thatarrayIndexis within valid bounds (non-negative and not greater than array length) regardless of whether the TagList is empty.Also fixed a typo in the XML documentation ("greater that" → "greater than") in both
TagList.netcore.csandTagList.netfx.cs.Customer Impact
Users cannot safely call
CopyToor use LINQ'sToArray()on emptyTagListinstances without catching exceptions.Regression
No.
Testing
TestEmptyTagListCopyTotest covering empty TagList withCopyTo(array, index),CopyTo(span), andToArray()for both empty and non-empty destination arraysarrayIndexvalues (negative or exceeding array length) still throwArgumentOutOfRangeExceptioneven when the TagList is emptyTestCopyToto exerciseICollection<T>.CopyTofor all sizes including 0System.Diagnostics.DiagnosticSource.TestspassRisk
Low. The change only affects the empty case and aligns behavior with existing collection types. The fix is applied consistently to both
TagList.netcore.csandTagList.netfx.cs.Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.