Snapchat / djinni

A tool for generating cross-language type declarations and interface bindings. Djinni's new home is in the Snapchat org.
Apache License 2.0
173 stars 50 forks source link

Compiler error in generated WASM/Protobuf #138

Open guthriec opened 1 year ago

guthriec commented 1 year ago

Hi,

I have an interface that includes some protobuf types (e.g.

my_service = interface + c {
  get_my_proto_type(arg: i64): ProtoType;
}

Generating for WASM works fine, using a command like:

../../../djinni/src/run \
    --java-out java \
    --wasm-out wasm \
    --wasm-include-cpp-prefix ../cpp/ \
    --wasm-base-lib-include-prefix ../../../../djinni/support-lib/wasm/ \
    --ts-out ts \
    --ts-module core \
...

However, the generated code runs into compilation errors when I try to compile using Emscripten: emcc -o test.html wasm/my_service.cpp -I../../my_dependency yielding an error:

wasm/my_service.cpp:27:26: error: use of class template '::djinni::Protobuf' requires template arguments
        return ::djinni::Protobuf::fromCpp(r);
                         ^
wasm/../../../../djinni/support-lib/wasm/djinni_wasm.hpp:272:7: note: template is declared here
class Protobuf {
      ^
wasm/concern_service.cpp:30:60: error: use of class template '::djinni::Protobuf' requires template arguments
        return ::djinni::ExceptionHandlingTraits<::djinni::Protobuf>::handleNativeException(e);
                                                           ^
wasm/../../../../djinni/support-lib/wasm/djinni_wasm.hpp:272:7: note: template is declared here
class Protobuf {

I notice I did not set --wasm-include-prefix, nor do I really understand what it means -- am I missing something there? Or is this a bug?

li-feng-sc commented 1 year ago

The codegen for a protobuf return value should be like:

return ::djinni::Protobuf<...>::fromCpp(r);

Looking at the code I don't see how it could generate Protobuf without the template arguments.

Could you provide a minimal case(just the .djinni file and the .yaml protobuf declaration file, djinni does not actually use the protogen during codegen) that reproduces this problem(Protobuf without template arguments) so that I could have a closer look and debug it if there is indeed a bug?

guthriec commented 1 year ago

Edit: I updated the errors and the repo to reflect some issues with bad imports in a prior version of the error repo.

I uploaded a repo with a minimal repro:

https://github.com/guthriec/djinni-error-repro

I'm on an Apple Silicon MacBook, in case that's relevant.

Codegen steps: Core/API/CoreAPIProto/proto_gen.sh Core/API/djinni_gen.sh

Then from the Core directory, running bazel build wasm results in errors:

Core/API/wasm/concern_service.cpp:27:26: error: 'Protobuf' is not a class, namespace, or enumeration
        return ::djinni::Protobuf::fromCpp(r);
                         ^
external/djinni/support-lib/wasm/djinni_wasm.hpp:272:7: note: 'Protobuf' declared here
class Protobuf {
      ^
Core/API/wasm/concern_service.cpp:30:60: error: use of class template '::djinni::Protobuf' requires template arguments
        return ::djinni::ExceptionHandlingTraits<::djinni::Protobuf>::handleNativeException(e);
                                                           ^
external/djinni/support-lib/wasm/djinni_wasm.hpp:272:7: note: template is declared here
class Protobuf {
      ^
2 errors generated.
li-feng-sc commented 1 year ago

Thanks! I will take a look.

li-feng-sc commented 1 year ago

So your problem is that you did not provide a ts: section in your yaml file, this is why Djinni does not know how to generate the code. See an example here https://github.com/Snapchat/djinni/blob/master/test-suite/djinni/vendor/third-party/proto.yaml#L9

We can probably make this a bit less confusing by providing better error message than generating incorrect code.

guthriec commented 1 year ago

Ah ok that seems plausible. I haven't had a chance to verify yet but thanks!

guthriec commented 1 year ago

I did get back to checking and got my setup working. A couple of more things that I think might be worth calling out in the documentation:

(1) Djinni expects the protobuf JS to be generated by https://github.com/protobufjs/protobuf.js/ (there are some other projects out there, including an official Google project that's not well-maintained). (2) The need to call registerProtobufLib on the generated WASM module.

I'll leave this open just in case you want to update the docs.