Remove null check that is used later to handle size request scenario.

[AaronRobinsonMSFT]

Fixes#13500

/cc @dotnet/dotnet-diag

[ghost]

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #13500

/cc @dotnet/dotnet-diag

Author:	AaronRobinsonMSFT
Assignees:	-
Labels:	area-Diagnostics-coreclr
Milestone:	7.0.0

[hoyosjs]

This technically changes the behavior of this in a way that it's visible to callers. Although I am not a fan of this, this is not documented and preserving old behavior feels safer (it's been there since framework). I.E: return E_POINTER and delete the other path

[AaronRobinsonMSFT]

This technically changes the behavior of this in a way that it's visible to callers.

Is that the bar for these kind of changes?

There is definitely risk here. I recently hit this very issue since the common pattern in these API is to pass nullptr and ask for the size. I was inclined to leave it but this evening I found this issue and clearly someone else also noticed this. If the preference is to keep the behavior I can remove the other check, but I do think it is worth following the most common pattern.

[hoyosjs]

My engineering head tells me I like the common pattern. My gut tells me that every time we break the return of a method that's been established for a while, things break. The more common case to get the count of fields on a type is ICorDebugProcess5::GetTypeLayout which uses similar logic. Given that there's customer facing APIs to get this information with roughly similar costs (they are on the same interface, and using the same inputs, with little added overhead), I lean for not going through breaking changes.

[noahfalk]

I'd put the bar at "is this likely to break debuggers?" Unlike BCL APIs where there can be millions of usages, most debugger APIs only have a few call sites in the whole world so the odds of a corner case usage is lower. I'd be surprised if any debugger ever intentionally passed NULL to this API because making a call that returns a predictable error isn't very useful. In the unlikely case some debugger's code depended on that behavior it is reasonable for us to ask them to make a tiny change to resolve the issue.

To add a little more confidence here, I'd confirm with @gregg-miskelly that VS doesn't rely on GetTypeFields() returning E_POINTER in their code. A github search in https://github.com/Samsung/netcoredbg shows no usage of the API so all good there. Once that is done + all the tests pass I think we've done enough due diligence.

[AaronRobinsonMSFT]

Thanks @hoyosjs and @noahfalk . Appreciate the frank discussion about concerns. Let me touch base with @gregg-miskelly and company offline to get their thoughts. I will circle back when that is done.

@hoyosjs Does what @noahfalk offered change your thoughts or do you still have strong concerns here?

[hoyosjs]

In my head, go for it. Undoing the break is easy enough if they can just tell their users to upgrade.