Fix FluentBundle::format_pattern lifetimes

[MathieuTricoire]

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.

fluent-rs/fluent-bundle/src/resolve.rs

Lines 146 to 148 in 43f1f59

	ast::PatternElement::Placeable { ref expression } => {
	scope.maybe_track(self, expression)
	}

but current code only return reference to the resource when there is only 1 TextElement i.e.

fluent-rs/fluent-bundle/src/resolver/pattern.rs

Lines 94 to 101 in 8e17ee6

	if len == 1 {
	if let ast::PatternElement::TextElement { value } = self.elements[0] {
	return scope
	.bundle
	.transform
	.map_or_else(|| value.into(), |transform| transform(value).into());
	}
	}

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]

(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]

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:

fluent-rs/fluent-bundle/src/resolver/pattern.rs

Lines 83 to 107 in df81ab8

	impl<'p> ResolveValue for ast::Pattern<&'p str> {
	fn resolve<'source, 'errors, R, M>(
	&'source self,
	scope: &mut Scope<'source, 'errors, R, M>,
	) -> FluentValue<'source>
	where
	R: Borrow<FluentResource>,
	M: MemoizerKind,
	{
	let len = self.elements.len();
	if len == 1 {
	if let ast::PatternElement::TextElement { value } = self.elements[0] {
	return scope
	.bundle
	.transform
	.map_or_else(|| value.into(), |transform| transform(value).into());
	}
	}
	let mut result = String::new();
	self.write(&mut result, scope)
	.expect("Failed to write to a string.");
	result.into()
	}

[MathieuTricoire]

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]

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.

[gregtatum]

Thanks for changes, it looks like a great addition. I left one question I'd like answered before merge, but I'm happy to approve it. Also, please note that my review is using conventional comments which uses labels for which comments are actionable. I believe I didn't include any suggestion: or nitpick: comments, so things look good.

[MathieuTricoire]

Thanks @gregtatum for all the feedback on my PRs 👍

[gregtatum]

Thanks @gregtatum for all the feedback on my PRs 👍

Thanks for the contributions!

[MathieuTricoire]

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]

I'll look at cutting a release this week.