Skip to content
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

COM interop converts VT_BSTR/NULL variants to null instead of string.Empty #71002

Closed
ClearScriptLib opened this issue Jun 20, 2022 · 6 comments
Closed

Comments

@ClearScriptLib
Copy link

Description

Our project relies on COM interop to bring VBScript support to .NET applications. A user has discovered that COM interop converts VT_BSTR/NULL variants to null.

According to this article, a NULL BSTR must be treated as an empty string. In other words, unlike .NET's string, BSTR isn't semantically nullable, so a variant of type VT_BSTR should never be converted to null.

This issue creates problems when invoking functions such as VBScript's InputBox, which returns a variant containing the input string or VT_EMPTY if the user canceled the dialog. Since COM interop yields null for both VT_EMPTY and VT_BSTR/NULL, a managed caller has no way to discern whether the user entered an empty string or canceled the dialog.

This behavior is present in both IReflect argument marshaling and Marshal.GetObjectForNativeVariant.

Reproduction Steps

Invoke Marshal.GetObjectForNativeVariant, passing in a VT_BSTR/NULL variant.

Expected behavior

The call should return string.Empty.

Actual behavior

The call returns null.

Regression?

Unknown

Known Workarounds

If the application has access to the variant, it can detect this case and substitute string.Empty. However, applications that rely on IDispatch-to-IReflect interop have no way to do that.

Configuration

This seems to affect both .NET Framework and .NET Core/5/6.

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 20, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the Future milestone Jun 20, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jun 20, 2022
@AaronRobinsonMSFT
Copy link
Member

@ClearScriptLib Since this behavior was inherited from .NET Framework it is unlikely the built-in system will be changed due to compatibility concerns and relative value to other interop scenarios. The evolution of interop for COM scenarios is in source generation.

We would consider community contribution of this work but it would come with a discussion about the impact of that change. If there is enough community interest for the work, the interop team may reprioritize this work also. All of this however comes down to the cost/benefit of changing long standing behavior in the built-in COM interop system and that is not something we take on lightly.

@AaronRobinsonMSFT
Copy link
Member

/cc @dotnet/interop-contrib

@ClearScriptLib
Copy link
Author

Since this behavior was inherited from .NET Framework it is unlikely the built-in system will be changed due to compatibility concerns and relative value to other interop scenarios.

@AaronRobinsonMSFT That makes perfect sense.

We would consider community contribution of this work but it would come with a discussion about the impact of that change.

Of course. After all, it would be a breaking change. Just curious – is there a standard way in .NET to make such changes opt-in rather than enabled by default? Something like V8's system of flags?

@AaronRobinsonMSFT
Copy link
Member

Of course. After all, it would be a breaking change. Just curious – is there a standard way in .NET to make such changes opt-in rather than enabled by default? Something like V8's system of flags?

Not really. .NET Framework did have a quirks mode that was incredibly complicated and just made our lives difficult. In .NET Core, we make these decisions based on how valuable the change is and if it warrants breaking some compat. In this case, I could see an argument for changing this semantic in our new system but leaving it in the old. I am also open to "fixing" the behavior early in .NET 8 and seeing if anyone complains through the Previews and RCs. If no one is upset, then it could likely stay. The best time for this sort of thing would be in late summer/early fall when we convert main into vNext.

If you're interested in the work, please let me know. I can help with guidance.

@ClearScriptLib
Copy link
Author

Hi @AaronRobinsonMSFT,

I am also open to "fixing" the behavior early in .NET 8 and seeing if anyone complains through the Previews and RCs. [...] If you're interested in the work, please let me know. I can help with guidance.

Thanks. The specific issue our user encountered involves the InputBox return value, and we can work around that easily.

We've confirmed that the same issue applies to IDispatch-to-IReflect arguments, but we aren't aware of anyone affected by it. If the fix will not be backported, then there's little point in implementing it. Besides, if the issue becomes a big problem, we probably could fix it by patching the runtime's IDispatch vtable – yes, that's crazy, but it's something we already do to work around other quirks 😁

Anyway, if it would be possible to fix this in the new interop (source generator?), that would be great.

Cheers!

@AaronRobinsonMSFT
Copy link
Member

Besides, if the issue becomes a big problem, we probably could fix it by patching the runtime's IDispatch vtable – yes, that's crazy,

Not as crazy as you might think. We do it and most people who try to make COM interop better play these games from time to time. You are in good company :-)

Anyway, if it would be possible to fix this in the new interop (source generator?), that would be great.

I think this is a very reasonable request and one I will advocate for. I'm going to close this specific issue, but it is referenced in our COM source generator as something we will work to enforce by default.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants