Update Marshal.QueryInterface() argument modifier

[AaronRobinsonMSFT]

Fixes #91981

Change in modifier to ref readonly to avoid warnings in existing interop code.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

[ghost]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #91981

Change in modifier to ref readonly to avoid warnings in existing interop code.

Author:	AaronRobinsonMSFT
Assignees:	-
Labels:	area-System.Runtime.InteropServices
Milestone:	9.0.0

[AaronRobinsonMSFT]

/cc @stephentoub

[AaronRobinsonMSFT]

/backport to release/8.0

[github-actions]

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6167832178

[stephentoub]

cc: @tannergooding

[jkotas]

Change in modifier to ref readonly to avoid warnings in existing interop code.

I thought that it is "by design" that the change will produce warnings in existing code. There are other APIs with the same problem. For example, this compiled fine in .NET 7 and it will produce the exact same warning in .NET 8 as the Marshal.QueryInterface case.

var span = new byte[10];
int value = 42;
MemoryMarshal.Write<int>(span, ref value);

If we are not happy with this change, should we roll it back in all APIs that were affected?

[AaronRobinsonMSFT]

If we are not happy with this change, should be roll it back in all APIs that were affected?

I think in general I would agree that introducing new warnings should be avoided, but I do see the benefit in most of these cases. My specific concern here is the legacy nature of the Marshal.QueryInterface() API. I would be surprised if we see many users complain about the MemoryMarshal.Write<int>() APIs, but the Marshal.QueryInterface() API is old enough and in code that people rarely want to touch at all.

My calculus is on the nature of the Marshal.QueryInterface() API, it is very old and used in code that is rarely touched, and it adds no benefit to the interop scenario. Therefore any impact should weight more.

[jkotas]

These two changes from #89736 (comment) can introduce warnings in existing code:

• An API was changed from in to ref readonly, users will now be asked to use in explicitly
• An API was changed from ref to in, users will now be asked to use in or no longer specify ref

[jkotas]

the legacy nature of the Marshal.QueryInterface() API

Ok, makes sense.

It is unfortunate that we will end up with no consistency in how Guids are treated by various QI APIs:

• ICustomQueryInterface.GetInterface - ref Guid
• Marshal.QueryInterface - ref readonly Guid
• IIUnknownStrategy.QueryInterface - in Guid

[jkotas]

cc @Sergio0694

[Sergio0694]

Oh, well this is unfortunate 🥲

I was hoping starting with .NET 8 we'd be able to just pass Type.GUID (or other members that only expose a Guid by value and not by reference) without having to manually copy to a local first, which would've been nice.

[Sergio0694]

If people are actually failing to build, isn't that simply because they're on an older version of Roslyn? The changes needed to properly support ref readonly didn't make it to VS 2022 Preview just yet I think. Like, the build error in #91981 will not happen if you're using an up to date version of Roslyn, it will only generate a warning (see language spec for ref readonly). Considering that, are we sure we actually do need to revert this signature change? The fact some new warnings would be generated was discussed and approved during API review, but there's no actual breaking changes if you have updated tooling 🤔

[hamarb123]

This should be fine to have as in. There should only be an issue if they've converted the warning into an error. We should ask them if they've turned that on (if they've got this on, then it's no different to other new warnings suddenly becoming errors on new TFMs) AND check if they're on an up-to-date compiler, as an older compiler would obviously have issues. And if that's not the case, ping the roslyn team to see if they know what's gone wrong, instead of merging this since it is supposed to be fine.

To be clear: it is my opinion that this should not be merged on any version, because the issue shouldn't exist to start with. There is something else that has gone wrong. Either it's that they've enabled WarningsAsErrors (in which case they're being told about some unideal usage of an API as a warning, which they've opted to convert into an error), they've got an out-of-date compiler (either their fault, or the latest .NET 8 build has an old compiler which doesn't support the feature properly), or there's a bug in roslyn (which would obviously need to be fixed).

On a side note, this is why I suggested to the roslyn team on the issue (iirc I'm pretty sure I did anyway) that there should be no warning when using ref on an in parameter. If the user's issue is caused by WarningsAsErrors or similar, then would it be too late to fix the problem properly by changing this (i.e., by requesting the roslyn team to allow ref to be used for in parameters without a warning)?

[stephentoub]

The fact some new warnings would be generated was discussed and approved during API review

In general, yes. We didn't go API by API and discuss every single one being changed, though. And for things in interop, the folks responsible for interop (aka Aaron) get to weigh in (and apparently hadn't been aware of this).

there's no actual breaking changes if you have updated tooling

Whether or not it's something we consider to be "breaking", it's still friction. And the person responsible for our interop system is saying that friction isn't warranted.

WarningsAsErrors / they've got an out-of-date compiler / or there's a bug in roslyn

Or LangVersion is set to something 11 or lower. It's a supported scenario to upgrade your TFM but not your LangVersion, and in that case, this will end up being an error, not a warning, regardless of WarningsAsErrors.

[Sergio0694]

"Whether or not it's something we consider to be "breaking", it's still friction. And the person responsible for our interop system is saying that friction isn't warranted."

Sorry, let me rephrase that. Of course I do respect what Aaron is saying here, all I'm trying to say is just, given this PR is marked as a fix for an issue which is just a consequence of that user trying to compile with outdated tooling, and not a real break caused by the signature change when using C# 12, is it possible that the apparent need to revert this might've been influenced by that? As in, that considering the additional context (and the fact that user would not in fact see a build error as soon as they update Roslyn to latest), is there a chance undoing this might not actually be needed? 😅

"It's a supported scenario to upgrade your TFM but not your LangVersion, and in that case, this will end up being an error, not a warning, regardless of WarningsAsErrors."

This is more of a side note, but I find this to be a bit confusing. One of the main arguments for including those relaxed combinations of ref modifiers as part of the spec was specifically to allow changing the signature of APIs like this without it being a source breaking change. If se say that upgrading TFM without keeping the language version in sync is a perfectly supported scenario, doesn't that kinda just invalidate all of that? Because you'd still end up in a situation where you'd have users on an "officially supported" scenario getting (unwanted) breaking changes when migrating to .NET 8 🤔

[stephentoub]

doesn't that kinda just invalidate all of that?

No, it's just one more thing to factor in. It needs to be worth it. And Aaron is saying it's not worth it. There's a scale here and with all changes that introduce friction; this isn't something where everything is definitively 100% right or 100% wrong.

[stephentoub]

cc: @jaredpar

[AaronRobinsonMSFT]

@hamarb123 @Sergio0694 Both of your arguments are sound and I tend to agree with them. There are two dimensions I am concerned with.

First, consider the comment made by @jkotas at #91983 (comment). This can also introduce precisely the same error and comes with all the same UX concerns as Marshal.QueryInterface(), but the MemoryMarshal API seems appropriate to change and one could argue there are tangible wins. It has a much shorter life in .NET compared to Marshal.QueryInterface() - so that is the breadth of impact concern, it is just bigger.

Second, the Marshal.QueryInterface() is used in older and largely static interop code. The OP in this case is consuming a supplied file from the CsWinRT tool chain. They may have WarningsAsErrors, which can be typically easily be mitigated, but the LangVersion and/or tool chain may not be upgradable quickly. The point being interop code using this is rarely, if ever touched, and putting that out there for the stated reasons are hard to justify for this particular API.

These concerns are compounded by mucking with an API that doesn't functionally benefit from the change in any obvious way - performance or otherwise. The performance angle is on the interop boundary and any potential copy is going to be heavily out-weighted by the interop call itself and the UX has been established since .NET Framework 1.1. I just don't see the win, other than @jkotas's other comment at #91983 (comment), which I am very sympathetic to.

The update in this PR means that in or ref will work. Let's do the work in the ecosystem to update all places to what we want and then we can consider updating the signature in a manner that doesn't introduce friction.

[tannergooding]

Seems good to me overall.

I'm not particularly a fan of avoiding new warnings, but I understand why there is a desire to do it here in particular.

It being ref readonly notably also doesn't block typeof(T).GUID, it just transfers the warning there instead which the people who want to explicit take advantage of this can suppress instead.

This means by changing from ref to ref readonly (rather than in) here on this very old API means we maintain the status quo for existing code, we improve the experience for new code by removing the need to Unsafe.As to strip the readonlyness, and we allow power users to still pass rvalues if they are willing to suppress a warning.