MetadataTypedef attribute does not work

[kennykerr]

I took the MetadataTypedef attribute for a spin but it's useless as the metadata doesn't actually tell me what it replaced. I don't know for example that CreateCompatibleDC actually returns an HDC. Ideally the metadata would use the native types (e.g. HDC in this case) and use attributes to indicate alternatives that may optionally be used but as it stands, we can tell that something's changed but not what it changed from.

Originally posted by @kennykerr in #561 (comment)

[kennykerr]

Has this been addressed? Seems like 3608e3f just removed that one metadata type but didn't address the original issue where there are various such types where the native type is not preserved.

[mikebattista]

The native types all seem to be preserved unless you're referring to scenarios where the MetadataTypedef wraps a NativeTypedef like HANDLE. Today, the NativeTypedef would be lost. Is that what you're referring to?

[kennykerr]

Yes.

[mikebattista]

Ok. I'll reactivate and see if we can set the value of the MetadataTypedef to a NativeTypedef.

[mikebattista]

We could update the Value field like below, but if you expose just HANDLE, that might suggest that you should close the handle with CloseHandle which would be incorrect. I know you ignore RAIIFree right now @kennykerr, but how would you imagine handling this if you did?

[RAIIFree("DeleteTimerQueueEx")]
[InvalidHandleValue(0L)]
[MetadataTypedef]
public struct TimerQueueHandle
{
	public HANDLE Value;
}

[RAIIFree("CloseHandle")]
[InvalidHandleValue(-1L)]
[InvalidHandleValue(0L)]
[NativeTypedef]
public struct HANDLE
{
	public IntPtr Value;
}

[kennykerr]

I'd associate the free function with the function rather than the return type.

[mikebattista]

I was thinking the same. I'll try that.

[riverar]

Alternatively, we could establish a rule that states RAIIFree shall not appear on NativeTypedef types. APIs that accept or return HANDLE values that can be closed would need to use a specialized ClosableHandle metadata typedef instead.

[riverar]

Example:

// Theoretical APIs
public unsafe HANDLE GetASharedHandle();
public unsafe ClosableHandle GetAFreshHandle();
public unsafe TimerQueueHandle GetATimerQueueHandle();

// Untouched "raw" typedef
[NativeTypedef]
public struct HANDLE
{
	public IntPtr Value;
}

// A specialization with a free function
[RAIIFree("CloseHandle")]
[InvalidHandleValue(-1L)]
[InvalidHandleValue(0L)]
[MetadataTypedef]
public struct ClosableHandle
{
	public HANDLE Value;
}

// A specialization with an overriding free function
[RAIIFree("DeleteTimerQueueEx")]
[InvalidHandleValue(0L)]
[MetadataTypedef]
public struct TimerQueueHandle
{
	public HANDLE Value;
}

[kennykerr]

Just my 2 cents, but I think that's too prescriptive. It's not the metadata's job to invent types and associations that don't exist in the API. CreateFileA returns a HANDLE type that should be closed with CloseHandle and returns INVALID_HANDLE_VALUE on failure. All of that is information that describes CreateFileA and thus can and should be associated with the CreateFileA function in metadata, rather than other real or invented types. 😊

[riverar]

So it sounds like Kenny's proposal is to delete MetadataTypedefs and produce the following instead:

[return: RAIIFree("CloseHandle")]
public unsafe static extern HANDLE CreateFileA(...);

public unsafe static extern BOOL CreateTimerQueueTimer(
  [Out]
  [RAIIFree("DeleteTimerQueueEx")]
  [InvalidHandleValue(0L)]
  HANDLE* phNewTimer,

  [Optional]
  [In]
  [InvalidHandleValue(0L)]
  HANDLE TimerQueue,

   ...
);

[kennykerr]

I'd put the invalid value and the free function on the same attribute since they're inextricably linked.

[mikebattista]

I was thinking the HANDLE NativeTypedef has CloseHandle and InvalidHandleValues like it does today. For scenarios where HANDLEs must be closed with a more specific function, that is attributed on the HANDLE return value or out parameter like you showed above.

[kennykerr]

It seems a little convoluted to have to support two different approaches to retrieve the same information.

[mikebattista]

It seems redundant to decorate every HANDLE reference in the SDK with mostly the same attributes. I'd rather only decorate the deviations from the norm.

[riverar]

I was thinking the HANDLE NativeTypedef has CloseHandle and InvalidHandleValues like it does today. For scenarios where HANDLEs must be closed with a more specific function, that is attributed on the HANDLE return value or out parameter like you showed above.

So you're proposing something like this:

[RAIIFree("CloseHandle")]
[InvalidHandleValue(-1L)]
[InvalidHandleValue(0L)]
[NativeTypedef]
public struct HANDLE
{
	public IntPtr Value;
}

public unsafe static extern HANDLE CreateFileA(...);

public unsafe static extern BOOL CreateTimerQueueTimer(
  [Out]
  [RAIIFree("DeleteTimerQueueEx")]
  HANDLE* phNewTimer,
  [Optional]
  [In]
  HANDLE TimerQueue,
   ...
);

Hm. I see what you're trying to do there, but am wondering if some APIs may return a HANDLE that should not be freed at all. If one is found, we'll need another form of RAIIFree to signal to projection authors "do not free this handle". At which point, we have to ask ourselves if maybe the RAIIFree on the HANDLE itself is the right design.

[mikebattista]

but am wondering if some APIs may return a HANDLE that should not be freed at all. If one is found, we'll need another form of RAIIFree to signal to projection authors "do not free this handle". At which point, we have to ask ourselves if maybe the RAIIFree on the HANDLE itself is the right design.

We have [DoNotRelease] for scenarios like that. GetProcessHeap(s) have that attribute today.