kellpossible / cargo-i18n

A Rust Cargo sub-command and libraries to extract and build localization resources to embed in your application/library
MIT License
119 stars 24 forks source link

use fl! macro to pass a string slice to a function #112

Closed wiiznokes closed 6 months ago

wiiznokes commented 8 months ago

I need to use the String generated from fl! macro to be used in a fonction that take &str as parameter.

let error_text = fl!("already_use");
name.error(&error_text);

This is the error i get:

error[E0308]: mismatched types
  --> ui/src/localize.rs:26:9
   |
26 |         i18n_embed_fl::fl!($crate::localize::LANGUAGE_LOADER, $message_id)
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `&str`, found `String`
   |
  ::: ui/src/item.rs:94:27
   |
94 |         name = name.error(fl!("already_use"));
   |                           ------------------ in this macro invocation
   |
   = note: this error originates in the macro `fl` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider borrowing here
   |
26 |         &i18n_embed_fl::fl!($crate::localize::LANGUAGE_LOADER, $message_id)
   |         +

For more information about this error, try `rustc --explain E0308`.
error: could not compile `ui` (lib test) due to previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `ui` (lib) due to previous error
make: *** [Makefile:22 : fix] Erreur 101
wiiznokes commented 8 months ago

I imagine that to be able to do this, we would have to generate all the strings in a static array, and that fl! return a &'static str.

Any plan or thought on that ?

kellpossible commented 8 months ago

Hi @wiiznokes this is slightly related to #109

It looks like the code in your example is different to the one referenced in the error message?

let error_text = fl!("already_use");
name.error(&error_text);
  ::: ui/src/item.rs:94:27
   |
94 |         name = name.error(fl!("already_use"));
   |                           ------------------ in this macro invocation
   |

I think it should work what you have provided in the example, but to be sure, what is the type signature of .error() function? Another thing you can try is the .as_str() method on String.

kellpossible commented 8 months ago

I imagine that to be able to do this, we would have to generate all the strings in a static array, and that fl! return a &'static str.

Any plan or thought on that ?

The error message doesn't mention anything about lifetimes, does .error() require a &'static str ?

wiiznokes commented 7 months ago

I have the impression that the error message I provided was not the correct one. I get it with this code:

name = name.error(fl!("already_used_error"));

Which is obviously not good.

With this code:

let error_text = fl!("already_used_error");
name = name.error(&error_text);

this is the error message:

 cargo run
   Compiling ui v0.1.0 (C:\Users\wiiz\Documents\fan-control\ui)
error[E0515]: cannot return value referencing local variable `error_text`
   --> ui\src\headers.rs:110:5
    |
64  |         name = name.error(&error_text);
    |                           ----------- `error_text` is borrowed here
...
110 |     elems
    |     ^^^^^ returns a value referencing data owned by the current function

For more information about this error, try `rustc --explain E0515`.
error: could not compile `ui` (lib) due to previous error

I think the error would disappear if I stored the string in my app state, but that wouldn't be intuitive, which is why I would have preferred an api that directly returns a reference to me.

Also, rust analyser seems confused with the return type of the macro: image

kellpossible commented 7 months ago

I think that the problem here is the ownership of the message returned by the fl!() macro, for instances where messages are being formatted with dynamic variables, it will need to be owned in the local variable. Ideally we would probably have the output of fl!() be Cow<str> but we still have the problem that there is no way to differentiate between dynamically formatted messages and those that are simply references to messages stored statically in the program. Perhaps there needs to be an alternate version of fl!() to support this use case. Or alternatively, you can change the signature of name.error() to take ownership of the error text, so that when you return it, it is not holding on to a reference to a local variable.

kellpossible commented 7 months ago

Just out of interest, what is the type signature for the function name.error()? @wiiznokes

wiiznokes commented 7 months ago

Just out of interest, what is the type signature for the function name.error()?

/// Sets the error message of the [`TextInput`].
    pub fn error(mut self, error: &'a str) -> Self {
        self.error = Some(error);
        self
    }

It comes from this file: https://github.com/pop-os/libcosmic/blob/efe4ce2f5b514e4d553ab82c0c873dca7585c028/src/widget/text_input/input.rs#L267

kellpossible commented 6 months ago

The lifetime for the messages stored in FluentLoader makes it difficult to return a reference to them, even if there is no message formatting going on. This is because it's possible to unload the current bundles without destroying the loader itself. For now I'm pushing a breaking change in v0.8.0 to i18n-embed-fl which fixes #109 and ensures fl!() always returns String as was the original intention. With this change, if it's possible for you to store an owned value for error instead of &str this should hopefully resolve your problem?

I recognise that only returning String can result in a lot of extra allocations if your application primarily relies on static strings (which I guess to be realistic is most applications). We could perhaps modify the return value of fl!() to be a struct that contains a smart pointer to the bundle from which the message is contained, and some kind of self reference to that pointer for the message itself, but this is quite a bit of extra work that I don't have time to do at the moment and makes the API and code more complicated.

kellpossible commented 6 months ago

Let me know if you thinks this issue should still be open, otherwise I think it might be resolved for now. We can open a new one if we need the feature for returning references to messages within the loader.