[PreserveSig] inconsistently applied in IPicture

[JeremyKuhne]

Actual behavior

When generating IPicture we get the following errors in the latest build:

Windows.Win32.IPicture.g.cs(87,36): error CS0082: Type 'IPicture' already reserves a member called 'set_hPal' with the same parameter types
Windows.Win32.IPicture.g.cs(379,37): error CS0082: Type 'IPicture.Interface' already reserves a member called 'set_hPal' with the same parameter types

It's generating the following:

winmdroot.System.Ole.OLE_HANDLE hPal
{
    get
    {
        fixed (IPicture* pThis = &this)
        {
            winmdroot.System.Ole.OLE_HANDLE __result;
            ((delegate *unmanaged [Stdcall]<IPicture*,winmdroot.System.Ole.OLE_HANDLE* ,winmdroot.Foundation.HRESULT>)lpVtbl[4])(pThis, &__result).ThrowOnFailure();
            return __result;
        }
    }
}

It isn't doing [PreserveSig], while the setter is. Maybe something to do with the introduction of OLE_HANDLE?

Expected behavior

All methods are [PreserveSig] when "allowMarshalling" is false.

Repro steps

1. NativeMethods.txt content:

   IPicture

2. NativeMethods.json content:

   {
   "$schema": "https://aka.ms/CsWin32.schema.json",
   "allowMarshaling": false,
   "useSafeHandles": false
   }

Context

• CsWin32 version: 0.2.123-beta
• Target Framework: net8.0

[AArnott]

Very odd, because we do a full code generation with allow marshaling off in our automated tests and didn't hit this.

[AArnott]

I can repro with a test. It's probably a regression from #738.

[AArnott]

The compilation break is due to https://github.com/microsoft/win32metadata/issues/1367. I still don't understand how this bug is caught by a new, special purpose test but not caught in our catch-all full generation tests.

[AArnott]

All methods are [PreserveSig] when "allowMarshalling" is false.

Hmmm... so you don't want specialname get_ and set_ methods to be declared as properties on structs?

[AArnott]

Our 'full generation' test apparently skips at least some COM interfaces. 😲

[JeremyKuhne]

Hmmm... so you don't want specialname get and set methods to be declared as properties on structs?

The question is what you do with HRESULT codes. As long as we have both so we can check them manually, I'm fine.

[AArnott]

HRESULT codes. As long as we have both so we can check them manually, I'm fine.

@jeremykuhne C# properties cannot expose those. But we'll throw if they are error codes. Is that sufficient?

[JeremyKuhne]

C# properties cannot expose those. But we'll throw if they are error codes. Is that sufficient?

The idea is that we can save the try .. catch when we don't want to propagate an exception or deal with non-error return codes other than S_OK. There is also an issue of consistency in our interaction with the APIs.

And I wasn't thinking too clearly on this one. Clearly the issue is that you cannot have both because of the special name of getters and setters (the whole reason for the break here).

I'd say the minbar for me is that we can at least force some getters and setters to [PreserveSig] when the need warrants. Given that things are partial we can work around things I suppose so it isn't completely blocking...

[AArnott]

the minbar for me is that we can at least force some getters and setters to [PreserveSig] when the need warrants

The mechanism for that is the nativemethods.json file's list of preserveSigMethods. I can make sure that can apply to properties.

[JeremyKuhne]

@AArnott in the drop we currently have (0.2.98-beta) these were all generated as get_ & set_.

[AArnott]

Closing this as fixed given IPicture now works.

787 tracks the PreserveSig issue with struct methods.