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

External Errorsupport #1605

Open
stan-irl opened this issue Jun 19, 2023 · 7 comments
Open

External Errorsupport #1605

stan-irl opened this issue Jun 19, 2023 · 7 comments
Labels

Comments

@stan-irl
Copy link
Contributor

stan-irl commented Jun 19, 2023

I have a common error that I want to return from a few APIs called ApiError. I have defined this in my error crate:

[Error]
enum ApiError {
    "Unknown",
    "Internal",
    ...
};

When I include this error as an external in another crate, the generated kotlin code tries to import ApiError, but when the kotlin class is created it has been renamed to ApiException so my kotlin package doesnt compile.

It looks like this is intended behaviour that was introduced in 0.13.0

The docs mention that errors should work as externals here so I'm not really sure what is the best way to deal with this.

For now i'm working around this by just naming my enum ApiErr

┆Issue is synchronized with this Jira Task
┆Issue Number: UNIFFI-286

@stan-irl stan-irl changed the title trailing Error being replaced with Exception in kotlin generation breaks externals Trailing Error being replaced with Exception in kotlin generation breaks externals Jun 19, 2023
@mhammond
Copy link
Member

ExternalTypes are proving problematic in some cases, because we don't know what they are. A solution to this specific problem is probably to add a new variant here so that the bindings generator knows the type is an error. Longer term I wonder if leaning into "library mode" is the way to go so that we can learn about the actual type of all "external" types.

@bendk bendk changed the title Trailing Error being replaced with Exception in kotlin generation breaks externals External Errorsupport Jun 22, 2023
@bendk
Copy link
Contributor

bendk commented Jun 22, 2023

I think #1600 maybe addresses part of the issue here, but after that I see this error:

thread 'main' panicked at 'unsupported type for error: External { name: "UniffiOneError", crate_name: "uniffi_one", kind: DataClass }', uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs:238:18

Once that's merged I can take a look and try to fix this one. I don't think it's too hard.

Did we ever support external errors? I don't see any code in fixtures/ext-types that tests that

@mhammond
Copy link
Member

Did we ever support external errors?

I don't think so, but more by omission than intent.

@threema-lenny
Copy link

I think #1600 maybe addresses part of the issue here, but after that I see this error:

thread 'main' panicked at 'unsupported type for error: External { name: "UniffiOneError", crate_name: "uniffi_one", kind: DataClass }', uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs:238:18

Once that's merged I can take a look and try to fix this one. I don't think it's too hard.

Did we ever support external errors? I don't see any code in fixtures/ext-types that tests that

I'm now running into this, too. What would need to be done to address this?

@mhammond
Copy link
Member

mhammond commented Nov 1, 2023

I'm now running into this, too. What would need to be done to address this?

I doubt this will be a trivial change. Off the top of my head, I suspect ErrorMetadata would need to grow a variant for the external type, but I suspect you'd then hit a blocker which is that you will struggle to work out what the type actually is - ie, you will probably be unable to differentiate an enum with the error attribute from a regular enum from a record.

@threema-lenny
Copy link

Thanks, that does sound rather involved, yeah. It seems to be a common thing when using a multi-crate workspace. Is there a way to avoid this issue besides moving everything into a single crate (which, at least for us, would be a legitimate last resort option)?

@mhammond
Copy link
Member

mhammond commented Nov 6, 2023

I'm afraid I can't think of another work around. While I said the change might not be trivial, I also don't think it would be particularly complicated, and I'd be happy to help with guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants