Type-safer Diff.Compare at compile time

[Therzok]

Currently, we're allowing Compare with T being any kind of class.

Should we introduce an IDiffResult interface with from which the classes we support as convertible from DiffSafeHandle? Then Diff.Compare<T> would be T : IDiffResult.

This would improve a few aspects:

• No more runtime issues like this
• Type-safe and semantic completion support! I always had to check the sources for the types I can get.
• Customizable from user-side.
• No need to manually maintain the list (which is also outdated) in the exception message and the transformation list.

I know this would bring an implication that DiffSafeHandle would become public, or we can expose a proxy class with methods which interface the native methods.

[nulltoken]

@Therzok 🆒 idea.

However, I'm not sure I fully understand why we should make DiffSafeHandle public.

How about just creating a marker interface, make Tree, Patch implement it and update Diff.Compare<T> definition with T : IDiffResult.?

Customizable from user-side.

You lost me there, mate 😉

[Therzok]

Well, the user could take the native diff format and transform it how they want.

IDiffResult would have a FromNative(<somethinghere>) method which would transform the native format to the managed one.

Without something like that, an empty IDiffResult could be used by the user without any problem and they could just shoot themselves in the foot, and we wouldn't have any way to programatically get rid of the mapping dictionary and the runtime type check.

That's why the DiffSafeHandle proxy which could be used by LibGit2Sharp or a consumer to create their own DiffResult type.

[Therzok]

Idea in the clear:

class DiffSafeHandleProxy //give me better name
{
    internal DiffSafeHandleProxy(DiffSafeHandle handle) {}
// Expose git_diff_* methods here.
}

interface IDiffResult<T>
{
    T FromNative(DiffSafeHandleProxy proxy);
}

class Patch : IDiffResult<Patch>
{
     Patch FromNative(DiffSafeHandleProxy proxy)
     {
           return CreatePatchHere();
     }
}

[Therzok]

And we would end up with

Diff.Compare<T> (params here) where T:IDiffResult<T>
{

}

[carlosmn]

I'm not sure I get why you're so worried about a user adding IDiffResult<T> to their own classes. Why would they do that, if it's a marker for the compiler to be able to only accept the right classes?

And even if a user did try to somehow make use of DiffSafeHandle or DiffSafeHandleProxy, how would they? They'd need to know the layout of the version which they're using, at which point they're as much consumers of libgit2 as we are.

[Therzok]

So, let me write up what I thought about. The initial IDiffResult constraint is insufficient.

interface IDiffResult
{
}

Would be methodless, we wouldn't have any way to have a common denominator that says we can transform a DiffSafeHandle to a concrete user-facing implementation. The only way to introduce a fully type-safe implementation would be something that would roughly translate to:

interface IDiffResult<T>
{
    T FromNative();
}

Using this way, we get rid of this and this ugly implementations.

But we can't construct the T from nothing, so we have to expose the DiffSafeHandle in a nicer way. That's where DiffSafeHandleProxy comes to life and exposes LibGit2Sharp's interop Proxy class' methods to operate with it, in a similar way that each individual implementation. Like here, here and here.

class DiffSafeHandleProxy
{
    internal DiffSafeHandle handle;

    // Methods which operate on DiffSafeHandle and expose what it does. Otherwise, if we don't want user-side implementations, an opaque object for consumers.
}

interface IDiffResult<T>
{
     T FromNative(DiffSafeHandleProxy proxy);
}

class Patch : IDiffResult<Patch>
{
     // Explicitly hide it from API.
     Patch IDiffResult.FromNative(DiffSafeHandleProxy proxy)
     {
         // Set native props.
     }
}

class Diff
{
     public T Compare(...) where T : IDiffResult<T>, new()
     {
          // No more type checking.
          T something = new T();
          // Get diff proxy.
          something.FromNative(proxy);
          return something;
     }
}

LibGit2Sharp will continue having its own implementations, but the users can do their own things (if DiffSafeHandleProxy is not opaque).

The initial scope of this is to get rid of the runtime checks. The bonus scope is that we can possibly allow user-defined DiffResult types with git_diff methods exposed in the proxy.

[carlosmn]

The dictionary of types is certainly a hack, and getting rid of it in favour of letting the compiler do its job is certainly Good. But exposing the handle to a consumer of the library... if you want your own data type, I'd much rather you inherit from one of the existing types.

The need for an instance method instead of a constructor which knows how to read the native data is a bit annoying, but it looks like this is the way it'll have to be.

Can we keep the interface internal to the library? Can the compiler perform its type checks with the interface even if a consumer of the library cannot see it?

[Therzok]

Can we keep the interface internal to the library?

It's public facing API by becoming a constraint.

Can the compiler perform its type checks with the interface even if a consumer of the library cannot see it?

The consumer will have to know about the interfaces structure, we just have to hide the method it exposes with explicit interface implementations.

[carlosmn]

I suppose it makes sense you can't describe a generic method with secret constraints. The wrapper types are a bit annoying, but on the whole better than the type dictionary (what is this, ruby?), and looking into it, I can't find a better way to describe the constraints than what you describe.

👍 from me, FWIW.

[nulltoken]

@Therzok Let's try this. Do you feel like sending a PR?

[Therzok]

Sure.

[Therzok]

This is taking a turn for the worst. The new() constraint is only allowed on classes with public constructors, that means new Patch() will be valid afterwards.

I'll push what I have for now, but it's not something I'm glad with.