8000 Type-safer Diff.Compare at compile time · Issue #1176 · libgit2/libgit2sharp · GitHub
[go: up one dir, main page]
Skip to content

Type-safer Diff.Compare at compile time #1176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
Therzok opened this issue Aug 20, 2015 · 12 comments
Closed

Type-safer Diff.Compare at compile time #1176

Therzok opened this issue Aug 20, 2015 · 12 comments
Milestone
v0.22

Comments

@Therzok
Copy link
Member
Therzok commented Aug 20, 2015

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
Copy link
Member

@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
Copy link
Member Author
Therzok commented Aug 20, 2015

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
Copy link
Member Author
Therzok commented Aug 20, 2015

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
Copy link
Member Author
Therzok commented Aug 20, 2015

And we would end up with

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

}

@carlosmn
Copy link
Member

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
Copy link
Member Author
Therzok commented Aug 20, 2015

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
Copy link
Member

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
Copy link
Member Author
Therzok commented Aug 21, 2015

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
Copy link
Member

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
Copy link
Member

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

@Therzok
Copy link
Member Author
Therzok commented Aug 23, 2015

Sure.

@Therzok
Copy link
Member Author
Therzok commented Aug 23, 2015

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.

Therzok added a commit that referenced this issue Aug 23, 2015
@Therzok
Initial change to type-safe Diff.Compare
8ebafe7 
Fixes #1176.
@Therzok Therzok mentioned this issue Aug 23, 2015
Merged
Therzok added a commit that referenced this issue Aug 23, 2015
@Therzok
Initial change to type-safe Diff.Compare
f07006d 
Fixes #1176.
Therzok added a commit that referenced this issue Aug 23, 2015
@Therzok
Initial change to type-safe Diff.Compare
16e2dcc 
Fixes #1176.
Therzok added a commit that referenced this issue Aug 24, 2015
@Therzok
Decorate Diff.Compare generics with interface
8a673bb 
This allows for a typesafe Diff.Compare model.

This commit introduces a new IDiffResult interface which marks classes
which can be passed to Diff.Compare<T> so we get rid of the runtime
exception.

Fixes #1176.
@nulltoken nulltoken added this to the v0.22 milestone Aug 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.22
Development

No branches or pull requests
3 participants
@nulltoken @Therzok @carlosmn
0