Nullable Reference Types & ICustomMarshaler

[jonpryor]

Background and Motivation

The ICustomMarshaler interface should be re-reviewed in the context of Nullable Reference Types, in that null should be allowed everywhere. This is because ICustomMarshaler is an interface which dates to .NET 1.0, and existing interface implementations already assume that null can be used as parameters or return values.

Proposed API

--- a/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ICustomMarshaler.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ICustomMarshaler.cs
@@ -6,17 +6,13 @@ namespace System.Runtime.InteropServices
 namespace System.Runtime.InteropServices
 {     // This the base interface that must be implemented by all custom marshalers.
     public interface ICustomMarshaler
     {
-        object MarshalNativeToManaged(IntPtr pNativeData);
+        object? MarshalNativeToManaged(IntPtr pNativeData);
 
-        IntPtr MarshalManagedToNative(object ManagedObj);
+        IntPtr MarshalManagedToNative(object? ManagedObj);
 
         void CleanUpNativeData(IntPtr pNativeData);
 
-        void CleanUpManagedData(object ManagedObj);
+        void CleanUpManagedData(object? ManagedObj);
 
         int GetNativeDataSize();
     }

Usage Examples

Mono.Posix has had a type implementing ICustomMarshaler since 2005 which allows null values to be used:

https://github.com/mono/mono/blob/2020-02/mcs/class/Mono.Posix/Mono.Unix.Native/FileNameMarshaler.cs#L59-L75

	partial class FileNameMarshaler : ICustomMarshaler {
		public IntPtr MarshalManagedToNative (object obj)
		{
			string s = obj as string;
			if (s == null)
				return IntPtr.Zero;
			return UnixMarshal.StringToHeap (s, UnixEncoding.Instance);
		}

		public object MarshalNativeToManaged (IntPtr pNativeData)
		{
			return UnixMarshal.PtrToString (pNativeData, UnixEncoding.Instance);
		}
	}

Note: Above code is C# 1, and doesn't use nullable reference types, hence object and not object?.

Mono.Posix is not unique here; a cursory search through GitHub finds numerous examples which return or accept null:

• TestLucene.CrapLord.Utf8ConstCustomMarshaler: allows MarshalManagedToNative(null).
• Script.DebugMarshaler: MarshalNativeToManaged(IntPtr.Zero) can return null, via Marshal.PtrToStringAuto(IntPtr.Zero).
• FFmpeg.AutoGen.UTF8Marshaler returns null on MarshalNativeToManaged(IntPtr.Zero), and allows MarshalManagedToNative(null).
• Microsoft.Win32.Security.SecurityAttributesMarshaler: returns null for MarshalNativeToManaged(IntPtr.Zero) and accepts MarshalManagedToNative(null).

Alternative Designs

If ICustomMarshaler isn't updated to allow the long-standing practice of accepting and returning null values, then existing code will need to be "uglified" to disable the warning around appropriate blocks of code, e.g.

https://github.com/mono/mono.posix/blob/25c94683fb9c5ec667d38382904a85d813aaaadf/src/Mono.Unix/Mono.Unix.Native/FileNameMarshaler.cs#L69-L86

Risks

This will cause new warnings to be emitted for any newly written code which implements the ICustomMarshaler interface, and such new code doesn't allow/permit `nulls.

This change will make it easier to "reasonably" migrate existing code which already supports null values.

[stephentoub]

cc: @jeffhandley

[jeffhandley]

Thanks for filing this, @jonpryor, and for providing all of these details!

We've been treating changes to nullable annotations as bug fixes and documenting them in dotnet/docs#21202 by adding (unchecked) items into the issue description to capture the aggregated changes. If the changes are agreed upon, we can bypass the API review process and go straight to PR.

[jonpryor]

The Mono & .NET runtime marshaler will never pass null or IntPtr.Zero to the ICustomMarshaler methods, and thus the interface doesn't need to be changed.

[stephentoub]

Regardless, thanks for raising and discussing this, @jonpryor.