Is there a reason we can't have "deriving (eq, ord)" for interfaces?

[AndrewAtAvenza]

We use djinni quite a bit (big thanks!) but one frustration I keep running into is the inability to easily compare two objects of the same type on my terms. Since my djinni objects are almost always wrappers around lower level C++ objects I'd love to be able to have to djinni objects in Java compare using a method defined by my C++ objects.

For example, I have a djinni object representing an object that has an ID. I don't want to share that ID with the Java code, but it is there under the hood. When I compare to of these objects I assume Java is looking at the references and deciding they're not equal even though their IDs are -- being able to implement equals() and hashCode() in C++ would solve this.

The deriving () operators specifier for Record works really well. It feels like it could do the same thing for interfaces, but instead of generating all the code, why couldn't it just call abstract methods like it does with any any other generated method? I can see how it would work pretty easily with Java, so maybe the problem is other languages?

[a4z]

For better understanding, can I summarize it like that?

Equality is sometimes context-specific.
For some use cases, it has to be the same instance (same memory address),
for other use cases, all attributes need to be the same
In your use case, it is just one attribute of a class, an ID field
Is this summary legit?

If it is, my reasoning would be:
Records are clear, since they represent value types, and we know how to compare those (all attributes should match)
With reference types, it seems more difficult. That is why some languages have == and === or special equals functions.
I am unsure if a general solution is possible.
but if you think is is, please feel free to make a suggestion, probably paired with a PR (can be draft) to showcase a possible solution

[a4z]

Probably, the Attribute should be a Record? Hard to reason without seeing code and knowing the context.
But you talk about 'values' when talking about Attributes, so that might also be an option worth exploring.

[AndrewAtAvenza]

No, the Attribute object is too complex to be a record. It has various methods on it as well so it really can't be a Record -- this is why I was trying not to go into too much detail because then I'd spend four pages describing the whole system 😄

My point was just that as it stands, using reference objects with things like HashMap (and presumably many other containers) where the key is a reference object are going to work badly. It just feels like having the option to fill in the implementation if you need it would be a useful thing, especially since would obviously be opt-in. Maybe I'll give this a go and it'll be obviously a terrible idea, hard to say, but I'm interested to see how it plays out. If it's complex and clumsy than you wouldn't accept it and you obviously shouldn't. This should -- if it works -- be clean and easy.

If it wasn't for working with built-in containers I'd just use my own Compare() or Equals() function. But those don't help me look up the object in a HashMap -- I had a colleague more versed than I in Kotlin & Java look at it and we tried to trick Java by faking the functions but we just can't quite manage it. And it wouldn't be a great implementation anyways, since then you'd end up with those Java-specific methods landing in Obj-C or whatever. The Record 'derives' approach is quite elegant and I was an immediate fan of it when I understood it.

[artwyman]

Drive-by comment from a former maintainer still listening in: I think what you want is not for Djinni to "derive" the comparison functions automatically, but for Djinni to create the right comparison functions in the Java proxy, then bridge them over to your C++ implementation like any other interface method. It's something of a hybrid since you want the auto-declaration of "deriving" but not the auto-implementation. That's only necessary because the specifics of the needed comparison functions are different in each language, and may not match the normal declaration style of a Djinni interface method. I think that would be a reasonable feature to have, but not one which already exists. It would certainly be something you'd have to use carefully (you need to be careful about which parts of the system need reference comparison vs. value comparison semantics) but for specific use cases like the one you mention it could make sense.

The proxy cache was mentioned earlier, and it would definitely want to continue to use reference comparisons, but I don't remember the details of precisely what the key and value types are in the proxy cache. That is to say, I think implementing this feature might require some care to maintain correctness in each language. I encourage you to try, just want you to know what to watch out for.

I think the path of least resistance here would be to handle the issue entirely in Java as you're already doing. You could do that with a wrapper object around your Djinni interface. You could hide the details of that if you make a wrapper around the HashMap itself which wraps and unwraps the Djinni interfaces so external callers.

Another workaround to consider if you haven't already would be using external type support via a yaml file: https://github.com/dropbox/djinni?tab=readme-ov-file#modularization-and-library-support

You could use that to define your own custom type with its own comparisons, and allow Djinni to use it in other interfaces. It could be hand-written, or it could be a hand-modified form of a Djinni-generated type. Modifying generated files is tricky, but not too annoying so long as you don't need to constantly re-generate. You could optimize for that if you make your hash-key type small, but move all remaining methods to another interface returned by getMoreMethods() or something similar. As best I can remember, there's no problem with having multiple Djinni interfaces exposed on the same underlying C++ object.

[a4z]

Thanks @artwyman! Just for reference, the still editable doc to the modularization-and-library-support on djinni.xlcpp.dev , the source is here in the doc folder.

[a4z]

@AndrewAtAvenza Given what you know now, how would you design your system if you were starting from scratch, keeping in mind the current limitations and possibilities Djinni offers? I'm asking this to encourage a 'keep-it-simple' approach. In my experience, it's easy to go down a rabbit hole without realizing it, and simplicity often becomes clearer in hindsight.

[AndrewAtAvenza]

I appreciate the notes @artwyman!

I believe we're already using the modularization part of Djinni -- we have over a dozen .djinni files and some of them depend on types from other .djinni files. I had a bit more trouble following the hand-rolled translator part though if that's what you were suggesting I use. I'll look at that more closely on Monday.

And I should say that I used 'deriving' as an example of the kind of notation to use, your description of how it should actually work is pretty much exactly what I was suggesting: generate the equality stubs and the developer fills them in with the behaviour they want. In practice, it wouldn't be deriving(eq) but rather something like requires(eq) (not sure what the appropriate keyword would be) since you'd want it to be abstract in the djinni description so it could generate whatever hooks are required per-platform (much as deriving does, but without the final implementation).

And a wrapper can be used -- at least in Java -- to workaround this for things like HashMap -- mostly. It's a bit cumbersome and annoying, but it would work. My colleague whipped out a quick generic that would do the trick, but that won't help with Djinni itself. When I return a map<key, value>, if the key is a djinni reference object I can't inject a wrapper in there, unless I'm missing something. So a wrapper will make it at least possible to work structures expecting equality or ordering, but only outside of the djinni interface. So that leaves a fairly large hole.

@a4z The system in question is actually one I started building about a month ago and I alluded to the only way to avoid this problem -- namely keeping the djinni objects I create for methods in the object that spawns them. That can obviously be a good thing, but in a multi-threaded environment like ours, there could very well be two copies of a Layer object floating about and keeping them in synch is going to be a nightmare. The Djinni objects are, in the end, vessels for the underlying data so while I could design it (or even pivot now since its not in production) I think the solution to the equality problem creates a much harder synchronization problem.

And in the end, it just feels like being able to define equality would be a good thing. All the languages Djinni supports (that I know, which is most) support the idea so why not have it as feature? Clearly lots can be done without it given the number of users, but I think it would make the the generated objects more powerful to have the feature.

At any rate, the main purpose of this thread was to make sure there wasn't either some major technical hurdle I didn't know about that would preclude it, or if it would be a feature that would be unwelcome. It doesn't sound like either of those things is true so I'm happy to take a stab at it when I have time (which could be a month or two). Even if it's not ultimately accepted, at least I know I wouldn't be wasting my time in the attempt!

I really appreciate the robust & rapid feedback!

[a4z]

I did not want to argue against comparing interfaces; I just thought I would ask if there might be another way for you to solve the situation.

requires(eq) sounds good to me, for more than one reason.

If you can do a PR so we can experiment with that and let other users check if that would break something for them, that would be a great first step. Please feel free to focus on Java. We could decide on the other languages as we go on and see what we learn by the first implementation.

[AndrewAtAvenza]

I don't mind the question, it's a reasonable one. I don't actually have a major problem with my current system since I replaced the return value of map<Attribute, Value> with MyAttributeValueMap. I'd be happy to switch back if I have requires(eq) though 😄 It's good to have a goal!

Your gameplan sounds good to me. Better to kick the tires on a single language rather than all at once, and I don't have the means to check more than Java at the moment anyways (I have access to an iOS team but someone closer to the Djinni project could be better). I might have a few days to scratch the surface this week, but I imagine it'll mostly be spent figuring out to how to build it, and then how to modify it. Going to be a bit of a learning curve before I can even start tinkering!

[AndrewAtAvenza]

OK, so I had some time earlier than I thought I would and I couldn't let this go so I went ahead and I've managed a rough implementation. PR is #169.

I've tested it with my own code and using this I was able to drop that HashMap replacement I wrote in favour of the normal map<Attribute, Value> and it worked fine. This Kotlin unit test in my project passed when I used this updated Djinni generator whereas it fails if I use the current stable one (and remove the requires(eq) and comment out the override so it compiles):

val firstAttribute1 = layer.attributes.first()
val firstAttribute2 = layer.attributes.first()
assertEquals(firstAttribute1, firstAttribute2)

val map = hashMapOf(
  firstAttribute1 to 1,
  firstAttribute2 to 2
)

assertEquals(1, map.size)

When you get a chance, please take a look. I still don't know scala very well, but I was able to cobble this together by looking at the existing code since in many ways I'm not actually writing new logic, just new versions of existing logic. I'm sure it can be done more elegantly than I've done here so I would be happy to take suggestions on style or approach in addition to whatever other criticisms you can offer!

Thanks!

[a4z]

But maybe you can review it, @AndrewAtAvenza
(PS, reason why we need the fix described in #171 )

[AndrewAtAvenza]

I do have the ability to review & approve the PR, but I have very little experience with actions (dabbled ab it in our company repo, but just for PR labeling). I can see the checks worked so it looks like it's good. I can hit approve if you want, but it will be 100% based on the CI check and faith that you know what you're doing 😄 If that's okay, I'll hit approve, but if you want someone who actually knows GitHub actions, you'll have to get someone else to check it.

Just let me know if I should approve it blindly or not!

[a4z]

Yes, please just approve. Trust me, I am a developer:-)

Joke aside, the reason and description for the PR are in the issue and comment
And yes, the fact that sbt is not again available proves the change to work

So please approve so I can merge

[AndrewAtAvenza]

I've approved it but since I'm not a reviewer with write access, it's still blocked. So you'll need someone else to approve it before merging is allowed to continue. Which does make some sense I guess!

[a4z]

ok, thanks, I merged it now, pls rebase

[AndrewAtAvenza]

Sorry for the silence for a week, but I finally have some more time to put into this.

Publishing C++ to Java (and I think Obj-C) seems to be satisfactory at the moment (pending unit tests) so I was mulling publishing Java to C++. First, I wasn't sure if this was a requirement or not of the feature. If it isn't possible or if it feels too awkward, is that the end of it or is it okay for requires(eq) to exist for +c but not +j/+o? 'const' is not allowed for +j/+o (presumably that comes from the resolver) so I think its okay for it not to be allowed for anything but +c? I'm just trying to figure out how much energy I should devote to publishing Java to C++ or if that's a 'nice to have' rather than a requirement. I have some thoughts on it, but they're not nearly as clean as the reverse is.

To make +j work with requires(eq) is problematic because every object published to C++ is necessarily a shared_ptr. So the operator==() override -- which seems like the logical thing to implement back to Object::equals() -- won't help. To achieve the same functionality that requires(eq) manages in this case involves defining a bunch of global methods like:

namespace std {
    template<>
    class hash<shared_ptr<ClassName>> {
    public:
        size_t operator()(const shared_ptr<ClassName>& a) const 
        {
            // invoke Java object's hashCode() method via JNI
        }
    };
}

// does not help with std::map but is needed with above hash for unordered_map
bool operator==(const std::shared_ptr<ClassName>& left, const std::shared_ptr<ClassName>& right)
{
    return left && right && // call ClassName.equals() through JNI
}

This obviously comes with some baggage, but maybe because it's opt-in, that's fine. Ordering would be similar:

// this is sufficient to make the map look at IDs
bool operator<(const std::shared_ptr<ClassName>& left, const std::shared_ptr<ClassName>& right)
{
    return left && right && // call ClassName.compare() through JNI and return true if < 0 
}

I wrote up some Godbolt test cases to make sure this would work, and it does. Not sure if this is too ugly to use though. I'm pretty sure there's no other way to do it though. You can see my rough work here:

https://godbolt.org/z/d765zbMax

As always, thoughts appreciated! If we don't need to publish Java or Obj-C to C++ for the feature to be approved, I'll just move on to unit tests and circle back to this later if we're interested. If it is required, I can fiddle with an implementation of the above if that's deemed satisfactory.

[a4z]

What about starting with +c and ignoring +j and +o until someone requests this feature?

[AndrewAtAvenza]

I'm 100% okay with that, I was just making sure that it was okay to theoretically ship a release without +j/+o. I kind of think no one will ever want that, or at least I struggle to think of a use case.

Sounds like I can quiet the whirling thoughts about Java -> C++ and just do the unit test. I'll be much happier to do that 😄 I'll see if I can knock that out this week.

[artwyman]

I think overloading operator== on a shared_ptr to mean something different than direct pointer comparison is probably too big a foot gun to be worth doing. Note that even in Java there's a distinction between reference equality (a == b) and object equality (a.equals(b)). No opinion on other approaches to this feature for +j/+o, but I think leaving it for +c only is fine for now. Maybe allow it on objects which are +c and also +o +j but ignore it in the other direction for now.

For testing the +c feature, I'd suggest some tests which make sure the proxy cache is working correctly. I believe there are (or were) tests to confirm proper proxy-cache operation. Augmenting or duplicating them to run on a type with requires(eq) might be a good approach. The most specific case to cover would be ensuring that 2 separate objects which compare equal can be created, and receive 2 distinct proxy objects. The possible bug to avoid would be if the overridden equals() method resulted in getting back the proxy from the wrong object. I don't think that bug will occur based on my recollection of how the proxy cache works, but testing is the way to be sure.

[AndrewAtAvenza]

Thanks for the advice @artwyman , I was going to ask at some point if the resolver should be flagging any use of requires other than for +c but if it's okay to just ignore the others (and document it of course) that works for me. At an earlier point in my PR it was generating the interface in the C++ class for +j and I had to fix it, so I'm pretty sure it's already ignoring the others now but I'll double-check.

And I appreciate the tip for the unit test. That's the next step, figuring out how to do that properly. I'll keep an eye on the proxy cache in particular just to be sure.