projectfluent / fluent-rs

Rust implementation of Project Fluent
https://projectfluent.org
Apache License 2.0
1.04k stars 95 forks source link

Fix FluentBundle::format_pattern lifetimes #264

Closed MathieuTricoire closed 1 year ago

MathieuTricoire commented 1 year ago

Hi! First of all, thank you for all the work put into these crates!

I have faced the issue where a formatted pattern cannot outlive its arguments (same issue raised by #190 and #260)

It would be useful to allow it, since the returned Cow doesn't seems to be tied to any arguments references.

Aside note It was the case before at the time of the issue #190 i.e. https://github.com/projectfluent/fluent-rs/blob/43f1f59534487ca539164a160498138a6ae641b2/fluent-bundle/src/resolve.rs#L146-L148 but current code only return reference to the resource when there is only 1 TextElement i.e. https://github.com/projectfluent/fluent-rs/blob/8e17ee622bb98aad2ff2382755633148637fede9/fluent-bundle/src/resolver/pattern.rs#L94-L101 End of the aside note

The problem is this code doesn't compile:

use fluent_bundle::{FluentArgs, FluentBundle, FluentResource};
use std::borrow::Cow;
use unic_langid::langid;

fn main() {
    let res = FluentResource::try_new("greeting = Hello, { $name }".to_string()).unwrap();
    let en_us = langid!("en-US");
    let mut bundle = FluentBundle::new(vec![en_us]);
    bundle.add_resource(res).unwrap();

    let result = greeting("World!", &bundle);
    println!("{result}");
}

fn greeting<'a, 'b>(name: &'a str, bundle: &'b FluentBundle<FluentResource>) -> Cow<'b, str> {
    let mut errors = vec![];
    let value = bundle.get_message("greeting").unwrap().value().unwrap();

    let mut args = FluentArgs::new();
    args.set("name", name);

    bundle.format_pattern(value, Some(&args), &mut errors)
}

Error:

error[E0623]: lifetime mismatch
  --> src/main.rs:22:5
   |
15 | fn greeting<'a, 'b>(name: &'a str, bundle: &'b FluentBundle<FluentResource>) -> Cow<'b, str> {
   |                           -------                                               ------------
   |                           |
   |                           this parameter and the return type are declared with different lifetimes...
...
22 |     bundle.format_pattern(value, Some(&args), &mut errors)
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ...but data from `name` is returned here

So the only issues is about lifetimes and this PR is a proposition to fix that.

Let me know if I'm correct, if you agree with the lifetimes naming and the tests.

Thank again!


FYI I tried to differentiate more lifetimes in Scope i.e. &'bundle FluentBundle and ast::Pattern<&'source str> because theorically they are different lifetimes but it requires too much work because we don't have access to 'source lifetime from FluentResource and it's most of time obfuscated as R when referred to. It could have been interesting because the format_pattern should return Cow<'source, str> and could theorically outlives FluentBundle but not the FluentResource.

pub fn format_pattern<'bundle, 'source, 'args>(
   &'bundle self,
   pattern: &'bundle ast::Pattern<&'source str>,
   args: Option<&'args FluentArgs>,
   errors: &mut Vec<FluentError>,
) -> Cow<'source, str>;
juliancoffee commented 1 year ago

(not a maintainer here, but I was able to fix my code by passing args outside, your code can be fixed like that)

use fluent_bundle::{FluentArgs, FluentBundle, FluentResource};
use std::borrow::Cow;
use unic_langid::langid;

fn main() {
    let res = FluentResource::try_new("greeting = Hello, { $name }".to_string()).unwrap();
    let en_us = langid!("en-US");
    let mut bundle = FluentBundle::new(vec![en_us]);
    bundle.add_resource(res).unwrap();

    let mut args = FluentArgs::new();
    args.set("name", "World!");
    let result = greeting(&args, &bundle);

    println!("{result}");
}

fn greeting<'a>(args: &'a FluentArgs, bundle: &'a FluentBundle<FluentResource>) -> Cow<'a, str> {
    let mut errors = vec![];
    let value = bundle.get_message("greeting").unwrap().value().unwrap();

    bundle.format_pattern(value, Some(args), &mut errors)
}
MathieuTricoire commented 1 year ago

Yes I know, sorry if my example is trivial, I gave a simple example to illustrate the problem and of course it can be solved as you suggest but my use case is more complex and cannot be solved this way.

My point is that there is actually no reason for this limitation, the returned Cow will never contain a reference to an argument (if my understanding of the code is correct).

The only time the returned Cow will contain a reference to a str will be if the given pattern contains only one element and that element is an ast::PatternElement::TextElement, then it will directly return the reference to the value of the ast element which is a great thing for performance reasons to not allocate a new String But in other situations a String will be allocated and the given pattern will be interpolated and returned, so the Cow will contain the allocated String.

So no reason to tie the returned lifetime to the arguments lifetime, a fix that will open up some interesting new possibilities IMHO.

The code is actually very easy to read to understand the issue: https://github.com/projectfluent/fluent-rs/blob/df81ab8bd772dd5a54f2b84378b2a33197e94ace/fluent-bundle/src/resolver/pattern.rs#L83-L107

MathieuTricoire commented 1 year ago

Hi @zbraniecki, would it be possible for someone to have a look at this PR and also #271?

I need this for a crate I have published: https://github.com/MathieuTricoire/l10n

Don't hesitate to let me know your thoughts on these 2 PRs and also on my crate if you want and have time.

Thanks again for all your work!

zbraniecki commented 1 year ago

Hi, I'm sorry for the delay. I haven't had a lot of time to devote to fluent-rs lately due to ICU4X 1.0 release.

@eemeli @nordzilla @gregtatum @dminor - would any of you have a moment to look at this?

if not, I'll try to get to it in a couple weeks.

MathieuTricoire commented 1 year ago

Thanks @gregtatum for all the feedback on my PRs 👍

gregtatum commented 1 year ago

Thanks @gregtatum for all the feedback on my PRs 👍

Thanks for the contributions!

MathieuTricoire commented 1 year ago

I didn't want to open an issue for that so I'm writing here. Just wanted to know if you have a timeline to release fluent-bundle with the latest PRs?

gregtatum commented 1 year ago

I'll look at cutting a release this week.