dotnet / Silk.NET

The high-speed OpenGL, OpenCL, OpenAL, OpenXR, GLFW, SDL, Vulkan, Assimp, WebGPU, and DirectX bindings library your mother warned you about.
https://dotnet.github.io/Silk.NET
MIT License
4.15k stars 397 forks source link

Consider adding ref property overloads in structs for those who have a crippling fear of unsafe #222

Open Perksey opened 4 years ago

HurricanKai commented 4 years ago

I beg you use in/out over ref

Perksey commented 4 years ago

For properties this isn't an option

HurricanKai commented 4 years ago

I'm sorry, but if properties in the title refer to the language construct, what is the current way? Are pointer properties the current way of doing this? (I've never seed a C# property in Silk so far)

Perksey commented 4 years ago

For example:

public struct InstanceCreateInfo
{
    public ApplicationInfo* PApplicationInfo;
    public ref ApplicationInfo PApplicationInfoRef => ...
}

However, now that I think about it, it may be better to generate alternative structs using this ref technique like ImGui.NET does instead of this.

HurricanKai commented 4 years ago

I'd argue both should be a property with a shared underlying field, also, then a ref property seems very reasonable, my apologies

Perksey commented 4 years ago

I'm pretty sure there's no good way to do this until dotnet/csharplang#1147 is in. Could compromise for Span overloads instead?

cc @JakenVeina

HurricanKai commented 4 years ago

We could store a pointer and convert to a ref?

Perksey commented 4 years ago

Ref properties can't have setters.

HurricanKai commented 4 years ago

https://docs.microsoft.com/de-de/dotnet/api/system.runtime.compilerservices.unsafe.asref?view=netcore-3.1#System_Runtime_CompilerServices_Unsafe_AsRef__1_System_Void__

Perksey commented 4 years ago

The problem isn't that we can't convert pointers to refs, it's that the property overloads will only work one-way. For example, you won't be able to:

applicationInfo.PNextRef = ref _nextThing;

Because the C# language forbids ref properties having setters. However, this would work:

ref applicationInfo.PNextRef

But if you haven't assigned the pointer value already, this property would return Unsafe.NullRef

HurricanKai commented 4 years ago

Methods, maybe?

Perksey commented 4 years ago

Could do methods, yeah.

Perksey commented 4 years ago

Oh wait no because there's no guarantee that the ref will be valid without it being within a fixed block. Unsafe.AsPointer isn't safe here.

JakenVeina commented 4 years ago

To me, the concept of "unsafe" in this context means having to build the struct tree that the native APIs call for, because it has to be done via pointers. A "safe" mechanism should be doing this work for you, not just exposing the same work to be done, but without pointers.

E.G. if an outer struct requires a field that points to an inner struct, the outer struct should allocate both, and handle the pointer logic for connecting them together, and simply expose a ref of the inner struct, for the caller to manipulate. I suppose this would probably necessitate that this logic is done in a wrapper struct that manages BOTH different native structs.

I know most of the APIs are actually auto-generated, here, so I don't know how feasible this idea is. If there's an upcoming language mechanism that would solve this problem, you're free to wait for that as well. But to me, that' the kinda "safe" solution that I'd go for.

Perksey commented 4 years ago

I can do just about anything with the generated APIs, so if you could propose what your idea would be in terms of the existing files (perhaps reuse the ApplicationInfo.gen.cs example) then that'd be great, I'm a lot better at reading code than I am English.

HurricanKai commented 4 years ago

I would 100% agree with what @JakenVeina said. A fully save wrapper that does all the initialization work for you is ideal, when you want save wrappers. Currently we are just looking into adding some save-ish overloads, but we could be looking into doing more then that.

Can ref properties have init things? That would somewhat solve the problem too.

Perksey commented 4 years ago

Ref properties, unfortunately, are strictly get-only. If we had ref fields, it would be easy to tackle as we'd be able to just use a union. If either of you have ideas please let me know as I'm really struggling to picture what Jake said in terms of code.

HurricanKai commented 4 years ago

If I understand what he said correctly, this would mean a fully save wrapper. Ie, the constructor initializes all resources, handles updates, handle lifetime of underlying native resources, etc. etc. With some performance loss we could probaby generate this too, but it would be a good amount of work.

Perksey commented 4 years ago

If I had code I could probably fairly easily generate it, but I don't even know what the safe wrapper would look like atm.

HurricanKai commented 4 years ago

Consider a current struct like:

public struct SomeNativeResource
{
    public byte* PApplicationName; // string
    public byte* PWindowTitle; // string
}

a new type like

public struct SomeNativeStructButManaged : IDisposable // idk maybe indicated by a namespace?
{
    public string ApplicationName { get { ... } set { ... } }
    public string WindowTitle { get { ... } set { ... } }

    public SomeNativeStructButManaged(string applicationName, string windowTitle) { ... allocate native strings and assign to some backing field 

    public void Dispose() // dispose pattern, but I can't be bothered to type that out in the web browser...
    {
        ... dispose of backing native resources ...
    }
}

I mean that doesn't solve the ref problem, but I think that's what @JakenVeina ment? Am I correct here? This would of course also work for other native resources. A difficult task. yeah...

Perksey commented 4 years ago

Yeah. It's really just the ref problem I have no idea how to solve, but yeah that looks like something that I could do fairly easily (would require SilkTouch work, though)

HurricanKai commented 4 years ago

I do think that's mostly a thing SilkTouch would handle? (with some BuildTools attribute backing) Would move this into 3.0 if we choose this route. A span compromise would be nice imo :)

JakenVeina commented 4 years ago

For something like that, I don't see that needing to be disposable.

Lemme see if I can re-create a concrete example...

Right now Silk.NET has...

public struct InstanceCreateInfo
{
        public StructureType SType;
        public void* PNext;
        public uint Flags;
        public ApplicationInfo* PApplicationInfo;
        public uint EnabledLayerCount;
        public byte** PpEnabledLayerNames;
        public uint EnabledExtensionCount;
        public byte** PpEnabledExtensionNames;
}

public struct ApplicationInfo
    {
        public StructureType SType;
        public void* PNext;
        public byte* PApplicationName;
        public uint ApplicationVersion;
        public byte* PEngineName;
        public uint EngineVersion;
        public uint ApiVersion;
    }

A "safe wrapper" for this, in my mind (ignoring the strings and extension pointers), would look something like.....

public ref struct SafeInstanceCreateInfo
{
    public uint Flags
    {
        get => _instanceCreateInfo.Flags;
        set => _instanceCreateInfo.Flags = value;
    }

    public uint EnabledLayerCount
    {
        get => _instanceCreateInfo.EnabledLayerCount;
        set => _instanceCreateInfo.EnabledLayerCount = value;
    }

    public uint EnabledExtensionCount
    {
        get => _instanceCreateInfo.EnabledExtensionCount;
        set => _instanceCreateInfo.EnabledExtensionCount = value;
    }

    ...

    public SafeApplicationInfo ApplicationInfo
        => _applicationInfo;

    internal InstanceCreateInfo _instanceCreateInfo;
    internal SafeApplicationInfo _applicationInfo;
}

public ref struct SafeApplicationInfo
{
    public uint ApplicationVersion
    {
        get => _applicationInfo.ApplicationVersion;
        set => _applicationInfo.ApplicationVersion = value;
    }

    public uint EngineVersion
    {
        get => _applicationInfo.EngineVersion;
        set => _applicationInfo.EngineVersion = value;
    }

    public uint ApiVersion
    {
        get => _applicationInfo.ApiVersion;
        set => _applicationInfo.ApiVersion = value;
    }

    ...

    internal ApplicationInfo _applicationInfo;
}

I'm sure I've screwed up some of the technicals here, but that'd be the general idea.

Then, the CreateInstance() API method would accept a ref parameter of this struct, do a few quick finalization operations on the structs (which are already fully allocated) and pass them off to the native calls, as such...

safeCreateInstanceInfo._createInstanceInfo.SType = ...;
safeCreateInstanceInfo._createInstanceInfo.PApplicationInfo = &safeCreateInstanceInfo.ApplicationInfo._applicationInfo;
safeCreateInstanceInfo.ApplicationInfo._applicationInfo.SType = ...;
Perksey commented 3 years ago

Discussed in working group meeting

Perksey commented 3 years ago

The currently approved model was making my cry so we need to take a step back and re-evaluate it. What I'm thinking is:

Postponing to 2.X

HurricanKai commented 3 years ago

We should consider overloading existing ctors similar to how we overload methods already.

Perksey commented 10 months ago

Provisionally moving this into 3.0.