Skip to content
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.

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

Add proper inheritance support for remote objects in clients #8

Open
Genhis opened this issue May 28, 2020 · 5 comments
Open

Add proper inheritance support for remote objects in clients #8

Genhis opened this issue May 28, 2020 · 5 comments
Labels
enhancement New feature or request protocol

Comments

@Genhis
Copy link

Genhis commented May 28, 2020

In my service, I have classes which share common functionality, so I set up an inheritance hierarchy. However, this hierarchy is not preserved when I generate client code. Instead, the common functionality is duplicated for each derived class annotated with KRPCClass which prevents me from using benefits of inheritance on the client side, such as creating a list of objects with the same base class. Although not all languages may support inheritance, it would be nice to have it at least in languages which support it.

After doing some research, I believe the following parts would need changing to support this feature:

  • ClassSignature.cs: Add the closest parent class annotated with KRPCClass
  • clientgen: Edit template files with one of these options:
    • Add abstract class hierarchy and make all methods abstract, and change concrete classes to derive from this hierarchy, still having the duplicate code (may be easier to implement)
    • Add proper hierarchy where methods would be implemented at their correct positions without unnecessary abstract definitions
  • docgen: Make use of parent classes and point to their correct location in the documentation

To conclude my examples below, if a base class have KRPCClass annotation, all classes derived from it should be exported and create a correct hierarchy. The derived classes do not need to have KRPCClass annotation. This is just my opinion on how I would use the API, and I welcome further discussion.

I would love to help with the implementation but currently I am having issues with making this project compile on Windows. I think I am missing some dependencies but I don't know which ones exactly.


Examples:

// This class is internal and should not be exported to the client
// It should not contain any KRPC properties/methods, although the current implementation allows it
public class InternalBase {
    public void InternalMethod() {}
}

[KRPCClass(Service = "MyService")]
public class Base : InternalBase {
    [KRPCMethod]
    public void NormalMethod() {}
}

[KRPCClass(Service = "MyService")] // Is this necessary if parent is annotated?
public abstract class Base2 : Base {
    [KRPCMethod]
    public abstract void AbstractMethod();
}

// It is not necessary to export this class because it doesn't have any KRPC properties/methods
// but there is nothing wrong with doing so
public abstract class Base3 : Base2 {
    public overrite void NormalMethod() { ProtectedAbstractMethod(); }
    protected abstract void ProtectedAbstractMethod();
}

[KRPCClass(Service = "MyService")] // Is this necessary if parent is annotated?
public class Derived : Base3 {
    // No need to have annotation here, the abstract method is annotated.
    public void AbstractMethod() {}

    public void ProtectedAbstractMethod() {}
}

How I would use it in Java:

List<Base> list = new ArrayList<>();
list.add(service.getBase()); // Base object
list.add(service.getDerived()); // Derived object

Base2 base = service.getDerived(); // Derived object
base.abstractMethod(); // run Derived implementation
@djungelorm
Copy link
Member

Duplicate of krpc/krpc#63

@djungelorm djungelorm marked this as a duplicate of krpc/krpc#63 Mar 10, 2023
@djungelorm
Copy link
Member

I agree this would be a good addition. I will draw up a design for this.

This was referenced Mar 26, 2023
@djungelorm
Copy link
Member

if a base class have KRPCClass annotation, all classes derived from it should be exported and create a correct hierarchy.

I think we should require explicit KRPCClass annotations on all classes that you want to be exposed to clients. That way you have complete control over what's exposed. For example, you might have base class A, and classes B, C and D that derive from it. You might want to expose A, B and C to the client but not D.

What we need to implement is:

  • Being able to add KRPCClass attribute to base classes
  • The scanner needs to only associate a method with the class in which it is defined. Currently you would get multiple copies of the method.
  • KRPCMethod applied to a method defined in a class without the KRPCClass annotation should produce an error
  • The generated service definition JSON needs to contain the inheritance tree. Probably just need to store the name of a classes parent in the definition of the class, or none for a base class.
  • Updates to all clients to have stubs generated with the correct inheritance
  • Updates to documentation generator

Open questions:

  • What about multiple inheritance? Do we just forbid it? Some clients might not support it?
  • What about constructing instances of a base class? Should this be permitted? Could be valid in the C# code, so I think yes?
  • What do the KRPC.ProcedureCall messages look like? Possibly just contain the object id and no type information as it's not actually needed?

@Genhis
Copy link
Author

Genhis commented Mar 11, 2023

It's been a long time since I wrote this request, so I don't remember all details about how I imagined certain things to function.

  • Sure, KRPCClass annotations can also be required for derived classes.
  • Multiple inheritance is tricky. I think it should follow C# rules. That is, a class can inherit from one class and multiple interfaces. I am not sure how this could be represented in non-OOP languages.
  • Constructing base class instances should follow C# rules (allowed as long as the class doesn't have unimplemented abstract functions).
  • I can't answer the last question.

@djungelorm
Copy link
Member

I agree we should just try and follow C# rules. I think this will be fine for most clients that support OOP.

The interesting one would be the Cnano client. I think it'll be doable though. I'll have a go at a proof of concept implementation.

@djungelorm djungelorm transferred this issue from krpc/krpc Mar 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request protocol
Projects
None yet
Development

No branches or pull requests

2 participants