-
Notifications
You must be signed in to change notification settings - Fork 279
Fixed bug that would cause a struct to not match and throw a runtime error. #895
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
Fixed bug that would cause a struct to not match and throw a runtime error. #895
Conversation
…time error. When a struct had an empty constructor that modified a field/property to a non-default value. The matcher would fail and throw a runtime error. This bug fix attempts to fix that issue by essentially zero initializing the struct the same way that default(T) does through the use of a runtime helper method. Simply, we replace Activator.CreateInstance with GetUninitializedObject. The idea came from this post https://stackoverflow.com/questions/1005336/c-sharp-using-reflection-to-create-a-struct Which contains and deprecated method as of .NET8. Microsoft has a recommended fix for that here. https://learn.microsoft.com/en-us/dotnet/fundamentals/syslib-diagnostics/syslib0050
|
Thanks for the pull request! Could you please add some unit tests? |
- Added test method `Any_on_struct_with_default_constructor_should_work` to ensure method calls with struct arguments do not throw exceptions. - Introduced `StructWithDefaultConstructor` struct: - Contains a property `Value`. - Has a default constructor initializing `Value` to 42. - Created `IWithStructWithDefaultConstructor` interface: - Declares `MethodWithStruct` that accepts `StructWithDefaultConstructor` as an argument.
|
I added a simple test that does fail on the main branch. Not sure there is a whole lot to test here. I did ensure that all tests pass, especially ones that take in nullable primitive types like |
|
Is there anything still blocking this from being merged? |
|
I fixed the format issue. |
Only a review I guess 😅 |
| return BoxedDouble; | ||
| } | ||
|
|
||
| return Activator.CreateInstance(returnType)!; |
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.
I'm doubting if we should use Activator.CreateInstance(returnType)!; for non-stucts, to keep things 100% the same for the previous cases.
Although all tests works, so not sure.
@nsubstitute/core-team any opinion on this?
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.
I'd feel more comfortable if we keep the existing behaviour for non-structs. Never sure how reflection stuff is going to break 😅
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.
Is this fixed? This isn't marked as resolved?
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.
So after some reading, this is large change?
Activator.CreateInstance(returnType)!;Calls the (default) constructor - that is indeed an issue with structs
System.Runtime.CompilerServices.RuntimeHelpers.GetUninitializedObjectWon't call the constructor.
I'm not sure if this is a good idea.
I'm not sure how DefaultValue is used exactly and if this poses issues.
Anyway, without resolving this discussion, I cannot approve this PR.
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.
The method itself is attempting to get the default value for the type. i.e. 0 for int. For reference types, as you know, this would return null when you try to do object o = default;.
For structs, when you set them to default, for example, when the parameter list has something along the lines of void MyMethod(CancellationToken cancellationToken = default) and then you try to create a matcher against it using Arg.Any<CancellationToken>(), it results in two instances of a struct that will not match, because Arg.Any<CancellationToken>() does not equal default(CancellationToken), which is why GetUninitializedObject is needed.
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 runtime error that occurred when using NSubstitute's Arg.Any<T>() matcher with structs that have non-default constructors. The issue arose because Activator.CreateInstance would invoke the constructor, causing the matcher to fail when comparing against the modified struct values.
- Replaces
Activator.CreateInstancewithGetUninitializedObjectto create zero-initialized structs - Adds special handling for
Nullable<T>types to return null - Implements conditional compilation for .NET 5+ vs older frameworks
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/NSubstitute/Core/DefaultForType.cs | Updates struct instantiation logic to use GetUninitializedObject instead of Activator.CreateInstance |
| tests/NSubstitute.Acceptance.Specs/ArgumentMatching.cs | Adds regression test for struct with default constructor scenario |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Updated `GetDefault` methods in `AutoObservableProvider.cs` and `AutoTaskProvider.cs` to use the `DefaultForType` class for determining default values, improving code modularity and reusability.
|
@304NotModified Anything still holding this back that I can help with? I would love to not have to maintain my own branch anymore and it seems that others have faced problems similar to this one. |
dtchepak
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.
LGTM, thanks!
@Romfos, @304NotModified , if this looks ok to use please approve and merge 🙏
|
@304NotModified I am sure you are busy, is there anything I can help with that is holding you back from approving these changes? |
|
Hey! I'm not that active anymore on NSubstitute 😅 I could try to check this PR this week, but I'm not sure if this needs an approval of me? |
|
reviewed. There is still an open discussion in this PR. I don't know the exact side effects of this change, so unfortunately I cannot approve this. @dtchepak maybe enable the check of the discussions so we could not forget this Under settings -> rules -> rulesets
|
zvirja
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.
Looks reasonable to me! I think the usage of Activator was indeed meant to get uninitialized zeroed struct. Especially giving that it's exactly what default() is doing and is not invoking struct constructor.
|
Ok let's merge then! :) |
Thanks @304NotModified . Previously PR rules were applied via branch protection. I've added a ruleset for default branch as well so hopefully this will be sorted now. 🤞 |

When a struct had an empty constructor that modified a field/property to a non-default value. The matcher would fail and throw a runtime error. This bug fix attempts to fix that issue by essentially zero initializing the struct the same way that
default(T)does through the use of a runtime helper method. Simply, we replaceActivator.CreateInstancewithGetUninitializedObject.The idea came from this post
Which contains and deprecated method as of .NET8. Microsoft has a recommended fix for that here.
This resolves the issue in #766