Improve unit test coverage for GetUninitializedObject #44843
Conversation
GrabYourPitchforks
added
area-System.Runtime
test-enhancement
GrabYourPitchforks
requested review from
marek-safar,
jkotas and
ViktorHofer
|
Note - there are two test failures on my machine (CoreCLR Win10).
I think we can live with the zero-inited |
src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs
Outdated
Show resolved
Hide resolved
| @@ -192,16 +191,102 @@ private static void FillStack(int depth) | |||
| } | |||
| } | |||
|
|
|||
| public static IEnumerable<object[]> GetUninitializedObject_NegativeTestCases() | |||
| { | |||
| // TODO: Test actual function pointer types when typeof(delegate*<...>) support is available | |||
There was a problem hiding this comment.
Yeah. typeof(delegate*<void>) is basically treated the same as typeof(IntPtr), so GetUninitializedObject(typeof(delegate*<void>)) succeeds since it results in a zero-inited boxed IntPtr. Presumably once the runtime and reflection stack treat these as real pointer types then we'll want to block them, same as void* today.
There was a problem hiding this comment.
I would add this as an item to the issue about better support for pointers in reflection.
There was a problem hiding this comment.
I would add this as an item to the issue about better support for pointers in reflection.
Already called out in the top post of #11354.
src/libraries/System.Runtime/tests/System/Runtime/CompilerServices/RuntimeHelpersTests.cs
Outdated
Show resolved
Hide resolved
|
Latest iteration: respond to PR feedback, plus block If somebody wants to hack the runtime into creating boxed voids, that's on them, I guess. But it seemed prudent to block it here since |
GrabYourPitchforks
requested review from
CoffeeFlux,
lambdageek,
thaystg and
vargaz
as code owners
|
Latest iterations are getting these tests to pass on mono. The mono runtime previously allowed calling
|
|
cc @CoffeeFlux for the Mono changes. |
| yield return new[] { typeof(IDisposable), typeof(MemberAccessException) }; // interface type | ||
|
|
||
| yield return new[] { typeof(List<>), typeof(MemberAccessException) }; // open generic type | ||
| yield return new[] { typeof(List<>).GetGenericArguments()[0], PlatformDetection.IsMonoRuntime ? typeof(MemberAccessException) : typeof(ArgumentException) }; // 'T' placeholder typedesc |
There was a problem hiding this comment.
Is this difference something that's worth fixing?
There was a problem hiding this comment.
Honestly I'd prefer to normalize absolutely everything to ArgumentException, but figured there might be people relying on the old behavior.
There was a problem hiding this comment.
Turns out Activator.CreateInstance and RuntimeHelpers.GetUninitializedObject perform many of the same checks, but they throw different exceptions. For example, when given an abstract class, CreateInstance throws MissingMethodException, but GetUninitializedObject throws MemberAccessException. Unifying these is going to be a bear.
Since the Activator.CreateInstance exceptions are documented on MSDN but the RuntimeHelpers.GetUninitializedObject exceptions aren't, I'm thinking of unifying on the Activator.CreateInstance behavior and taking the break in GetUninitializedObject. Thoughts?
Copy dotnet/runtime#44843 Co-authored-by: GrabYourPitchforks <GrabYourPitchforks@users.noreply.github.com>
ghost
locked as resolved and limited conversation to collaborators
Based on a comment at #32520 (comment) that we should improve unit test coverage for
RuntimeHelpers.GetUninitializedObjectbefore making any changes to the runtime logic. This helps demonstrate that these unit tests pass both before + after any changes to the implementation.