NordSecurity / uniffi-bindgen-go

Uniffi bindings generator for Golang
Mozilla Public License 2.0
75 stars 21 forks source link

v0.24: generated bindings don't always compile #21

Closed kegsay closed 10 months ago

kegsay commented 11 months ago

Using https://github.com/NordSecurity/uniffi-bindgen-go/pull/13 - in general it works great, but the generated bindings sometimes do not compile. The compile errors include stuff like:

cannot use variantValue.Error (value of type func() string) as string value in argument to FfiConverterStringINSTANCE.Write

Because the struct has func Error() string (to meet the error interface) and has a public field called Error which isn't valid.

In addition, autogenned Some(Bytes) seems to not correctly set the zero value for []byte, causing:

invalid composite literal type byte

the code being:

    if err != nil { // Some(Bytes)
        done <- struct {
            val []byte
            err error
        }{
            val: []byte{{}}, // <-- does not compile
            err: err,
        }
        return
    }

This also seems to be true for the zero value of interfaces, which seems to be set to the literal 0 which is not valid:

        done <- struct {
            val MessageType
            err error
        }{
            val: 0,
            err: err,
        }

where:

type MessageType interface {
    Destroy()
}

In general it should be safe to just omit zero values rather than explicitly setting them to something (which is error-prone), but I don't exactly know how practical that is.

dignifiedquire commented 11 months ago

Thanks for the report, it would be super helpful if you could PR a test case that triggers this to the 0.24 branch (or just post it here and I'll add it)

kegsay commented 11 months ago
git diff
diff --git a/fixtures/errors/src/errors.udl b/fixtures/errors/src/errors.udl
index c8fc765..fcbd984 100644
--- a/fixtures/errors/src/errors.udl
+++ b/fixtures/errors/src/errors.udl
@@ -27,6 +27,11 @@ interface ValidationError {
   UnknownError();
 };

+[Error]
+interface ErrorNamedError {
+  Error(string error);
+};
+
 [Error]
 interface ComplexError {
     Struct(Vec2 position_a, Vec2 position_b);
diff --git a/fixtures/errors/src/lib.rs b/fixtures/errors/src/lib.rs
index 7b07383..6379c06 100644
--- a/fixtures/errors/src/lib.rs
+++ b/fixtures/errors/src/lib.rs
@@ -12,6 +12,12 @@ pub enum BoobyTrapError {
     HotDoorKnob,
 }

+#[derive(Debug, thiserror::Error)]
+pub enum ErrorNamedError {
+    #[error("Error {error}")]
+    Error { error: String },
+}
+
 #[derive(Debug, thiserror::Error)]
 pub enum ValidationError {
     #[error("Invalid user_id {user_id}")]

produces:

// Variant structs
type ErrorNamedErrorError struct {
    Error string
}

func NewErrorNamedErrorError(
    error string,
) *ErrorNamedError {
    return &ErrorNamedError{
        err: &ErrorNamedErrorError{
            Error: error,
        },
    }
}

func (err ErrorNamedErrorError) Error() string {
    return fmt.Sprint("Error",
        ": ",

        "Error=",
        err.Error,
    )
}

which is wrong as field and method with the same name Error.

Figuring out a test case for the other one..

kegsay commented 11 months ago

So the problem with the other failure is in uniffiFutureCallbackHandler... style functions. In these functions, async-ness is done via a Go channel, with a single struct sent on completion/failure. The failure condition is the problem, and they look a bit like:

        done <- struct {
            val Foo
            err error
        }{
            val: 0,
            err: err,
        }

In cases where the return type is Some(Enum(Foo)) where Foo implements some trait (and thus needs an interface), the default value of 0 is used (because it's an enum I guess?). But this isn't valid Go, as 0 does not implement the interface. In these cases, nil would be more appropriate. Better yet would be to omit it entirely and let the default value take place.

The same thing happens when working with bytes:

    if err != nil { // Some(Bytes)
        done <- struct {
            val []byte
            err error
        }{
            val: []byte{{}},
            err: err,
        }
        return
    }

likely because the generator is trying to make a 0-length slice as the default value, but this isn't valid syntax for byte slices (or int slices, etc).

dignifiedquire commented 11 months ago

the second issue should be fixed now

arg0d commented 10 months ago

The async code issue is fixed, and I created a fresh issue for Error variant. This can be closed, right?

kegsay commented 10 months ago

Yup this can be closed now as #30 is the only outstanding issue. Thanks!