Using @Virtual with method returning @StdString generates function returning dangling reference

[equeim]

For example, for this Java code:

@Namespace("NativeLibrary") @Properties(inherit = org.example.NativeLibraryConfig.class)
public class NativeClass extends Pointer {
        static { Loader.load(); }
        /** Default native constructor. */
        public NativeClass() { super((Pointer)null); allocate(); }
        private native void allocate();

        @Virtual public native @StdString BytePointer get_property();
}

Following C++ class is generated:

class JavaCPP_hidden JavaCPP_NativeLibrary_0003a_0003aNativeClass : public NativeLibrary::NativeClass {
public:
    jobject obj;
    static jmethodID get_1property__;

    JavaCPP_NativeLibrary_0003a_0003aNativeClass() : NativeLibrary::NativeClass(), obj(NULL) { }
    using NativeLibrary::NativeClass::get_property;
    virtual std::basic_string< char >& get_property();
    std::basic_string< char >& super_get_property() { return NativeLibrary::NativeClass::get_property(); }
};

Where JavaCPP_NativeLibrary_0003a_0003aNativeClass::get_property() is:

std::basic_string< char >& JavaCPP_NativeLibrary_0003a_0003aNativeClass::get_property() {
    // Skip boring stuff

    // Dangling reference!
    return StringAdapter< char >(rptr, rsize, rowner);
}

JavaCPP_NativeLibrary_0003a_0003aNativeClass::get_property() should return by value, not reference. It also means that generated override actually overrides nothing, since NativeLibrary::NativeClass::get_property() returns by value.
I suspect that it is caused by StdString.java having @Cast({"std::basic_string", "&"}) annotation, but I'm not sure why it is there.

[saudet]

Yes, by default it assumes we're passing by reference because that's what is most widely used, but when if we need to pass by value, we can override the cast with something like this:

@Virtual public native @Cast({"", "std::string"}) @StdString BytePointer get_property();

BTW, when passing by value BytePointer is probably not useful, and using String like this should work just as well:

@Virtual public native @Cast({"", "std::string"}) @StdString String get_property();

[equeim]

But why is explicit cast is neccessary here? If you pass @StdString BytePointer to a function that takes std::string& it will compile without explicit cast by using implicit conversion via StringAdapter<T>::operator std::basic_string<T>&().

[saudet]

We're not doing conversions, we're implementing a function.

[saudet]

You're talking about other use cases? There are a lot of functions with const char * overloads and such, so that makes most calls ambiguous unless we do explicit conversions.

[equeim]

Yeah, you are right.
I think it would be cleaner if adapters had explicit conversion functions, e.g. toPointer() and toContainer(). But that would obviously break compatibility for people that make their own adapters.

BTW, that NativeClass Java class is what Parser generated for me. Meaning that it knew that get_property() returns by value but didn't add @Cast({"", "std::string"}). Not sure how easy that would be to fix though.

[equeim]

Also, how does one use this function:

virtual std::string wtf(std::string str) { return str; }

With following Java code

@Virtual native @Cast({"", "std::string"}) @StdString String wtf(@Cast({"", "std::string"}) @StdString String str);

I get ambiguous call error:

/home/alexey/javacpp/target/test-classes/org/bytedeco/javacpp/jniAdapterTest.cpp: In function '_jstring* Java_org_bytedeco_javacpp_AdapterTest_00024VirtualData_wtf(JNIEnv*, jobject, jstring)':
/home/alexey/javacpp/target/test-classes/org/bytedeco/javacpp/jniAdapterTest.cpp:852:104: error: call of overloaded 'basic_string(StringAdapter<char>&)' is ambiguous
  852 |         StringAdapter< char > radapter(((JavaCPP__0003a_0003aVirtualData*)ptr)->super_wtf((std::string)adapter0));
      |                                                                                                        ^~~~~~~~                                              

[saudet]

Yeah, you are right.
I think it would be cleaner if adapters had explicit conversion functions, e.g. toPointer() and toContainer(). But that would obviously break compatibility for people that make their own adapters.

The idea with cast operators is that we can have the compiler make some guesses. We can't do that with normal functions, so it's not clear that it's better...

BTW, that NativeClass Java class is what Parser generated for me. Meaning that it knew that get_property() returns by value but didn't add @Cast({"", "std::string"}). Not sure how easy that would be to fix though.

Ah yes, that's right, the Parser doesn't have any information about whether any given annotation is an adapter or not. It only outputs a cast like that when it encounters template types for which no information was provided, in which case it knows that we're probably going to have an adapter for those, but it can't guess that for normal types like std::string.

[saudet]

I get ambiguous call error:

Yeah, the problem in that case is that the adapter doesn't return std::string, and the compiler is not able to decide if it should call the const std::string& or the const char* cast operator. So we would need to able to specify somehow that we want to override a method that returns a std::string, but to use the adapter that returns a const std::string&. It gets really complicated, and I haven't gotten around to work out something for that...

[equeim]

I see two options:

1. Call operators explicitly, like adapter.operator std::basic_string<char>&() or adapter.operator const char*()
   This solution is ugly and we also will need to somehow make sure that these operators actually exist, but it won't break compatibility.

2. Replace implicit conversion operators with explicit conversion methods. Something like that:

class StringAdapter {
    /// ...
    const char* toPointer();
    char* toPointer();
    const unsigned short* toPointer();
    unsigned short* toPointer();
    std::basic_string<T>& toContainer();
    std::basic_string<T>* toContainer();
    /// ...
}

class VectorAdapter {
    /// ...
    const P* toPointer();
    P* toPointer();
    std::vector<T>& toContainer();
    std::vector<T>* toContainer();
    /// ...

As far as I understand, Generator knows when it wants to convert adapter to pointer and when to container, so it shouldn't be hard to implement. It will break user code, but it would be easy to migrate.

[saudet]

We can't have 2 functions with the same signature, that won't work.

[equeim]

Oh lol, I don't know how I missed that.
Ok, variant 2.1:
Separate Adapter class in two (nested?) classes, one for container -> pointer conversion and other for pointer -> container.

template<typename T = char> class StringAdapter {
public:
    class ToContainer {
    public:
        ToContainer(const T* ptr, typename std::basic_string<T>::size_type size, void* owner);
        ToContainer(const unsigned short* ptr, typename std::basic_string<T>::size_type size, void* owner);
        // ...
        operator std::string&();
        operator std::string*();
        // ...
    };
    class ToPointer {
    public:
        ToPointer(const std::basic_string<T>& str);
        ToPointer(std::basic_string<T>& str);
        ToPointer(std::basic_string<T>* str);
        // ...
        operator const T*();
        operator T*();
        operator const unsigned short*();
        operator unsigned short*();
        // ...
    };
};

This way if we want to pass const char* from Java to C++ function taking std::string by value we create StringAdapter::ToContainer and convert it to std::string. Since StringAdapter::ToContainer doesn't have operator const char* it will work without ambiguous call.

On the slightly unrelated note: I think that it would be good idea to use static_cast for adapter conversions. It will catch some cases when conversions are not possible at compile time. Using C-style cast will do reinterpet_cast instead of proper conversion in these cases resulting in undefined behaviour. For example, it is possible when casting to references.

[saudet]

Separate Adapter class in two (nested?) classes, one for container -> pointer conversion and other for pointer -> container.

Yes, that solves some issues, but at the cost of added complexity, and it doesn't change anything for cases like yours with std::string& and std::string. We still need an explicit conversion for that.

BTW, you could create your own custom adapter that converts only to std::string. That's basically the way we can get this case working well. We don't need to change everything.

On the slightly unrelated note: I think that it would be good idea to use static_cast for adapter conversions. It will catch some cases when conversions are not possible at compile time. Using C-style cast will do reinterpet_cast instead of proper conversion in these cases resulting in undefined behaviour. For example, it is possible when casting to references.

Ideally, yes, but that also adds complexity because static_cast() can't, for example, "unconst" references and such. We'd have to do a lot more explicit conversions that will need to appear in the Java declarations even though many of those C++ concepts do not map to anything useful in Java.