Unable to conditionally enable uniffi::constructor macro

[stan-stately]

I have a simple class that im trying to expose with proc macros like below:

#[derive(uniffi::Object)]
pub struct Client {
    api_endpoint: String,
}

#[uniffi::export]
impl Client {
    #[uniffi::constructor]
    pub fn new(api_endpoint: String) -> Self {
        Self { api_endpoint }
    }
}

my problem is that i only want uniffi scaffolding if my target is compiling for mobile - not wasm. In the past I have used conditional compilation for this with a lot of success. But when i try the following:

#[cfg_attr(not(target_family = "wasm"), derive(uniffi::Object))]
pub struct Client {
    api_endpoint: String,
}

#[cfg_attr(not(target_family = "wasm"), uniffi::export)]
impl Client {
    #[cfg_attr(not(target_family = "wasm"), uniffi::constructor)]
    pub fn new(api_endpoint: String) -> Self {
        Self { api_endpoint }
    }
}

I get the error associated functions are not currently supported:

error: associated functions are not currently supported
  --> <redacted>/lib.rs:32:9
   |
32 |     pub fn new(api_endpoint: String) -> Self {

For now I'm just putting a #[cfg(not(target_family = "wasm"))] on the whole block as a workaround, but i think this could be a bug in one of the macros

[jplatte]

Yeah, it is infeasible for uniffi to include its own parser + interpreter for cfg expressions such that it could know whether a cfg_attr should apply or not. I've wanted to solve this with a uniffi fake-cfg, but I don't think that's implemented yet.

[rafaelbeckel]

I came here to report this bug and I found this issue had already been opened.

The conditional works for wasm_bindgen macros but not for uniffi.

I expected this to work similarly in both APIs, even with cfg aliases:

#[cfg_attr(wasm, wasm_bindgen)]
#[cfg_attr(not(wasm), uniffi::export)]
impl SomeObject {
    #[cfg_attr(wasm, wasm_bindgen(constructor))] // works as expected, compiles
    #[cfg_attr(not(wasm), uniffi::constructor)] // this issue
    pub fn new() {
        ...
    }
}

The error string comes from uniffi_macros/src/export/scaffolding.rs, line 72.

[rafaelbeckel]

A workaround is possible with the cfg_if crate:

cfg_if::cfg_if! {
if #[cfg(not(wasm))] {
    #[uniffi::export]
    impl MyObject {
        #[uniffi::constructor] // the macro is parsed correctly because it's standalone now
        pub fn new() {
            ...
        }
    }
}
}

[rafaelbeckel]

The actual parsing happens at uniffi_macros/src/export/attributes.rs.

The ExportedImplFnAttributes constructor on line 260 ensures that it won't parse any attribute with an argument, which is the case for cfg_attr().

Maybe a full-blown parser isn't necessary. We could optimistically ignore the first cfg_attr argument and apply the current logic to whatever we find in the second argument. This would be a quick-and-dirty solution for this specific case, but I'll investigate further how wasm_bindgen does it before I suggest a proper fix.

[rafaelbeckel]

It seems wasm_bindgen just loops through all attributes until it finds wasm_bindgen ident. Because wasm_bindgen itself has arguments, everything else is built from there.

This is the relevant bit at wasm_bindgen/crates/macro-support/src/parser.rs, line 204:

fn find(attrs: &mut Vec<syn::Attribute>) -> Result<BindgenAttrs, Diagnostic> {
        let mut ret = BindgenAttrs::default();
        loop {
            let pos = attrs
                .iter()
                .enumerate()
                .find(|&(_, m)| m.path().segments[0].ident == "wasm_bindgen")
                .map(|a| a.0);
            ... // some validation i.e. checks if the parenthesis closes
        }
    ...
}

[jplatte]

That code you quoted does not look for wasm_bindgen inside cfg_attr. I think their attributes that go on functions inside impl blocks likely do the actual work (whereas the uniffi macros inside impl blocks are just dummies that exist so that they can be detected by the macro invocation on the impl.

[rafaelbeckel]

They don't look inside cfg_attr, indeed. They loop through all symbols in the attribute annotation until they find wasm_bindgen( ... ), ignoring everything else. I agree their attributes likely do the actual work.

In our case, the parser expects a path with exactly two symbols, the first being "uniffi" and the second "constructor" or "method".

We should not validate the total number of symbols in the attribute, but only the presence of "uniffi::{constructor|method}" and ensure they are not duplicated. This would solve the problem for most use cases and keep the existing behavior.

[rafaelbeckel]

If you agree with this approach, I can write a PR sometime this week.

[jplatte]

What do you think avout the alternative of a fake cfg? I mentioned it above but I realize I didn't provide an example. What I mean is specifically to make the macros look for #[cfg_attr(uniffi, <uniffi-attribute>)]. That cfg should never actually be set, it would just act as a marker that the export macro should look at the rest of the cfg_attr.

[rafaelbeckel]

Looking at the docs, my first impression is that it supports so many variations that we would have to deal with cases we cannot hope to foresee, especially combined with cfg_aliases and whatnot.

We could explicitly detect cfg_attr as the first symbol and ignore its first argument, but cfg_attr can also expand to itself, for example (copied from the docs) #[cfg_attr(target_os = "linux", cfg_attr(feature = "multithreaded", some_other_attribute))], so we would end up having to loop the rest of the symbols anyway.

[jplatte]

No, we wouldn't have to support any of that. I'm pretty sure just cfg_attr(uniffi, constructor | method(args...)) would be enough for >99% of real use cases, and everything else could just require the extra boilerplate of a separate cfg-dependent impl block.

[rafaelbeckel]

The problem with this syntax is that cfg_attr expects the first argument to be a true/false condition, and uniffi's name is not inherently true or false.

In my example, cfg_attr(wasm, ...) is actually an alias that expands to cfg_attr(target_arch = "wasm32", ...) by an external crate before compilation.

Without the alias, this is the actual syntax the compiler expects:

#[cfg_attr(target_arch = "wasm32", wasm_bindgen)]
#[cfg_attr(not(target_arch = "wasm32"), uniffi::export)]
impl SomeObject {
    #[cfg_attr(target_arch = "wasm32", wasm_bindgen(constructor))] // works as expected, compiles
    #[cfg_attr(not(target_arch = "wasm32"), uniffi::constructor)] // this issue
    pub fn new() {
        ...
    }
}

Users can come up with more granular cases. For example, one might want to support WASM, iOS, and Python bindings but use Pyo3 instead of uniffi for Python.

In this case, both examples below would be valid syntax:

#[cfg_attr(target_arch = "wasm32", wasm_bindgen)]
#[cfg_attr(not(target_arch = "wasm32"), cfg_attr(target_os = "ios", uniffi::export))]
#[cfg_attr(not(target_arch = "wasm32"), cfg_attr(not(target_os = "ios"), pymethods))]
impl SomeObject {
    #[cfg_attr(target_arch = "wasm32", wasm_bindgen(constructor))] // wasm-bindgen
    #[cfg_attr(not(target_arch = "wasm32"), cfg_attr(target_os = "ios", uniffi::constructor))] // uniffi
    #[cfg_attr(not(target_arch = "wasm32"), cfg_attr(not(target_os = "ios"), new))] // pyo3
    pub fn new() {
        ...
    }
}

// OR

#[cfg_attr(not(target_arch = "wasm32"), cfg_attr(feature= "pyo3"), pymethods))]
#[cfg_attr(not(target_arch = "wasm32"), cfg_attr(not(feature = "pyo3"), uniffi::export))]

My proposal is that we don't need to care about any of that.

We can safely assume that if uniffi::constructor exists anywhere in any of the annotations, it means the user intends to mark that function as a constructor.

Parsing and interpreting cfg_attr is Rust compiler's job. If the condition is not met, the compiler will discard the whole thing anyway.

[jplatte]

The problem with this syntax is that cfg_attr expects the first argument to be a true/false condition, and uniffi's name is not inherently true or false.

No, any raw identifier in cfg or cfg_attr is just treated to be false if no corresponding --cfg flag is passed to the rustc invocation. You can try it, add #[cfg(foobar)] to any item in a Rust codebase and you will see that that disables the item (likely leading to undefined symbol errors elsewhere) but does not produce an error about the foobar cfg not being defined.

[rafaelbeckel]

I'm sorry if I didn't express my point clearly. It wasn't about it failing. My intention was more about highlighting user's expectations and keeping consistency with similar APIs.

We could have a cfg_attr(uniffi, ...) marker, but wouldn't that limit users' ability to have granular control over their requirements?

// Is the idea to defensively only allow this version? 
#[cfg_attr(uniffi, constructor)]

// Is this also supported? Would we allow, require, or prohibit it?
#[cfg_attr(uniffi, uniffi::constructor)]

// Would it support arbitrarily nested predicates?
#[cfg_attr(foo, cfg_attr(bar, cfg_attr(uniffi, constructor)))]
#[cfg_attr(foo, cfg_attr(bar, cfg_attr(uniffi, uniffi::constructor)))]

// If nesting is supported, wouldn't this be easier to implement? 
// (reuses the existing logic, removes ambiguity)
#[cfg_attr(foo, cfg_attr(bar, uniffi::constructor))]

We can achieve the last version with minor changes to attribute.rs.

[jplatte]

The idea is to only support the first thing. No nesting, probably not even the second version with repeated uniffi.

[rafaelbeckel]

Thanks for clarifying.

I think it would be too restrictive and also would increase the current API surface, which would need to be documented.

The parser is already looping through all annotations and ignoring everything that does not start with uniffi. It already ignores #[cfg] arguments. Why would #[cfg_attr] be treated differently?

Looping inside each annotation until we find uniffi would keep the existing behavior and doesn't need to be documented because it keeps the current API intact.

It's just about wrapping the current loop into another loop.

I believe users should have total control over the predicate (cfg_attr's 1st argument), because it represents their unique requirements. It's outside uniffi's scope to care about that - it doesn't need to be aware of cfg_attr keyword existence at all. That's what the other libs are doing too.

[jplatte]

I think there is some misunderstanding going on here, but I haven't done a great job at digging into what it is exactly.

When you say UniFFI should support #[cfg_attr(not(wasm), uniffi::constructor)], do you mean that UniFFI should only treat the annotated function as a constructor if that cfg is true, or always¹? Your communication so far suggests the latter, which I think would be a huge footgun.

¹ assuming that #[uniffi::export] runs in both cfg(wasm) and cfg(not(wasm)), which is of course not usually the case

[jplatte]

The parser is already looping through all annotations and ignoring everything that does not start with uniffi. It already ignores #[cfg] arguments. Why would #[cfg_attr] be treated differently?

Looping inside each annotation until we find uniffi would keep the existing behavior and doesn't need to be documented because it keeps the current API intact.

It's just about wrapping the current loop into another loop.

I believe users should have total control over the predicate (cfg_attr's 1st argument), because it represents their unique requirements. It's outside uniffi's scope to care about that - it doesn't need to be aware of cfg_attr keyword existence at all. That's what the other libs are doing too.

You seem to think that uniffi currently special-cases cfg_attr somehow, but in fact it doesn't do that anywhere. The reason other attributes from other crates work with cfg_attr but UniFFI's method and constructor macros don't is that they are no-ops. They only exist for #[uniffi::export] to be able to see that they're being used, and act accordingly. This would either have to change (which I don't think is very realistic given the implementation constraints), or UniFFI has to grow support for cfg_attr by adding extra code.

[rafaelbeckel]

When you say UniFFI should support #[cfg_attr(not(wasm), uniffi::constructor)], do you mean that UniFFI should only treat the annotated function as a constructor if that cfg is true, or always¹? Your communication so far suggests the latter, which I think would be a huge footgun.

Yes, I suggest it always.

Semantically, if the user includes uniffi::constructor anywhere in the annotation (whatever the condition) they're telling uniffi: "This function right here is a uniffi's constructor".

If they include cfg_attr(condition, ...) they're telling the Rust pre-compiler, not uniffi, to "please evaluate this condition, and expand this macro if it's true".

This isn't a footgun because we're dealing with pre-compile time. Whatever the user does with their predicates is their power and responsibility, and should not be our concern. It's the compiler's job to tell them they're doing something wrong or inconsistent, not uniffi's.

Unless the lib was specifically designed to target the predicate (ex. cfg_alias crate), it is out of scope to care about it.

---

You seem to think that uniffi currently special-cases cfg_attr somehow

No; I know it currently does not special-cases cfg_attr, and I'd like to keep it this way. However, a fake cfg_attr change would have to special-case it to match the custom uniffi predicate.

---

The reason other attributes from other crates work with cfg_attr but UniFFI's method and constructor macros don't is that they are no-ops

That's not the reason. The reason is our parser expects it to be exactly #[uniffi::method|constructor]. We can keep them no-ops, the only difference would be that we'd detect them anywhere in the annotation instead of forcing it to be the only thing in existence.

This wouldn't break any existing codebase, and add support for custom conditions.

[jplatte]

What do you mean by "add support for custom conditions"? They would not be supported, they would just be accepted, but ignored as if they were always true. Your suggestion would be equally powerful to mine - allowing uniffi attributes to be used when the export macro is used conditionally, but not independently have their effects apply conditionally - while syntactically suggesting to users that the first cfg_attr argument is actually meaningful, which it wouldn't be.

[rafaelbeckel]

Thanks for your insights and patience so far!

The first argument is meaningful in the sense of explicitly adding or removing uniffi from scope.

// This is the normal uniffi's use case:

#[uniffi::export]
impl SomeObject {
    #[uniffi::constructor]
    pub fn new() { ... }
}

So cfg_attr users expect this to be valid:

// This is the normal `cfg_attr` use case

#[cfg_attr(my_condition = "true", uniffi::export)]
impl SomeObject {
    #[cfg_attr(my_condition = "true", uniffi::constructor)]
    pub fn new() { ... }
}


// Both conditions will usually match cargo.toml:
// ----------------------------------------------
[target.'cfg(my_condition = "true")'.dependencies]
uniffi = { version = "0.27" }

In this use case, all conditions must always match the dependency, and the compiler will enforce it. If the conditions are inconsistent, the program won't compile because it will eventually try to use a module that is out of scope.

I can't imagine a use case where someone would need to conditionally mark a function as a constructor. Normally, the goal of cfg and cfg_attr is to tell the compiler to add or remove a piece of code before compilation. If uniffi adds support to associated methods in the future, I'd prefer to annotate it with #[uniffi::static] instead of using cfg_attr to do so.

In summary, cfg and cfg_attr are part of the compiler's API and I'd expect uniffi to ignore both.

Risks of ignoring cfg_attr

In the current implementation, we already ignore #[cfg] and can't prevent users from doing this:

#[uniffi::export]
impl SomeObject {
    #[cfg(false_condition)] // uniffi currently ignores it
    #[uniffi::constructor]
    pub fn new() { ... } // the function won't be compiled, but I assume uniffi still exports its binding,
}                        // causing a runtime panic if called

Ignoring cfg_attr is less risky in comparison:

#[uniffi::export]
impl SomeObject {
    #[cfg_attr(my_condition = "false", uniffi::constructor)] // false, but uniffi would ignore it ¯\_(ツ)_/¯
    pub fn new() { ... } // function will be compiled and recognized as a constructor, no panic at runtime
}

Comparing the Alternatives

This is how the actual usage would look like after the change, assuming the normal cfg_attr use case:

Alternative 1: ignoring all external APIs (my suggestion)

#[cfg_attr(my_condition = "true", uniffi::export)]
#[cfg_attr(my_condition = "false", some::other_lib)]
impl SomeObject {
    #[cfg_attr(my_condition = "true", uniffi::constructor)]
    #[cfg_attr(my_condition = "false", some::other_lib)]
    pub fn new() { ... }
}

Alternative 2: fake cfg_attr predicate (your suggestion)

#[cfg_attr(my_condition = "true", uniffi::export)]
#[cfg_attr(my_condition = "false", some::other_lib)]
impl SomeObject {
    #[cfg_attr(uniffi, constructor)]
    #[cfg_attr(my_condition = "false", some::other_lib)]
    pub fn new() { ... }
}

Similarities

• Both solutions would work for my use case (and OP's).
• Both are WAY preferable to being forced to duplicate definitions.

So, while I prefer Alternative 1 for consistency and the other reasons above, I'm willing to compromise.

[jplatte]

Okay, so at least we have the same basic understanding :)

I agree that nobody would want to conditionally mark something as a c'tor, but maybe some users would want to conditionally rename a method for certain bindings (which could be approximated with doing it for a specific raget OS, e.g. android), which is a thing with #[uniffi::method(...)].

[rafaelbeckel]

Sorry for my late answer, I've got dragged into other priorities last week.

You mean conditionally using #[uniffi::constructor(name = "new_name")] and #[uniffi::method(name = "new_name")] depending on OS? For example:

#[uniffi::export]
impl SomeObject {
    #[cfg_attr(target_os = "android", uniffi::constructor(name = "new"))] // renames it for Android
    #[cfg_attr(target_os = "ios", uniffi::constructor)] // does not rename it for iOS
    pub fn some_static_method() -> Self {
        ...
    }
}

This could also be a possible use case for nested predicates, i.e.:

#[cfg_attr(wasm, wasm_bindgen)]
#[cfg_attr(not(wasm), uniffi::export)]
impl SomeObject {
    #[cfg_attr(wasm, wasm_bindgen(constructor))]
    #[cfg_attr(not(wasm), cfg_attr(target_os = "android", uniffi::constructor(name = "new")))]
    #[cfg_attr(not(wasm), cfg_attr(target_os = "ios", uniffi::constructor))]
    pub fn some_static_method() -> Self {
        ...
    }
}

Well, not actually useful (target_os = "..." is never WASM), but users would expect this syntax to be valid.

[rafaelbeckel]

Anyway, I cloned the repo and tried to run the tests both locally and on Docker (trunk without changes), but could not get them to work for Kotlin.

I implemented my suggested change but because I couldn't run the entire test suite, I didn't submit a PR yet. Can I submit it without the Kotlin tests running locally? My results with and without the changes are exactly the same for all the other platforms. Or should I have the entire suite running first?

[bendk]

Coming in late to this one, and I'm not sure I understand it all, but I wonder if it would be possible to rewrite each method by convert the cfg_attr to a cfg outside the impl block and stripping out any cfg_attrs that didn't refer to a uniffi attribute.

Note: In order to support UDL, we already have code to parse an item definition, generate the UniFFI code needed for it but throw away the actual item.

So, if UniFFI processed:

#[uniffi::export)]
impl SomeObject {
    #[cfg_attr(wasm, wasm_bindgen(constructor))]
    #[cfg_attr(not(wasm), cfg_attr(target_os = "android", uniffi::constructor(name = "new")))]
    #[cfg_attr(not(wasm), cfg_attr(target_os = "ios", uniffi::constructor))]
    pub fn some_static_method() -> Self {
        ...
    }
}

...it would generate this additional code:

#[cfg(all(not(wasm), target_os = "android")]
#[uniffi::export_and_throw_away_item)]
impl SomeObject {
    #[uniffi::constructor(name = "new")))]
    pub fn some_static_method() -> Self { }
}

#[cfg(all(not(wasm), target_os = "ios")]
#[uniffi::export_and_throw_away_item)]
impl SomeObject {

    #[uniffi::constructor]
    pub fn some_static_method() -> Self {  }
}

This would require a some parsing to handle nested cfg_attrs. However, I don't think we need to handle more complicated cases, like cfg_attrs that conditionally enable cfg attributes. I can't really think of a use-case other than and-ing together a bunch of predicates.

[bendk]

Maybe we could simplify the parsing, if we told users they had to use our syntax for conditionals:

#[uniffi::export)]
impl SomeObject {
    #[cfg_attr(wasm, wasm_bindgen(constructor))]
    #[uniffi::constructor(name="new", cfg_if=all(not(wasm), target_os = "android"))]
    #[uniffi::constructor(cfg_if=all(not(wasm), target_os = "ios"))]
    pub fn some_static_method() -> Self {
        ...
    }
}

[jplatte]

That sounds like a really good idea, if it can work reliably! (haven't thought it through completely, but sounds reasonable)

[rafaelbeckel]

Hey guys, sorry for my delay. I have moved to other issues and postponed the fix in my original PR, but #2113 represents exactly my original intention, so I closed my draft PR in favor of it.

I'm against introducing new syntax to deal with this problem. This would create the need to build a special parser and document the new behavior. If we keep our API intact and play along with the rest of the ecosystem, there's no need to document it. It'd favor the principle of least astonishment.

PS: I left a comment in the PR. Let's continue the discussion there.