projectfluent / fluent-rs

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

Implement Send for fluent/fluent-bundle API types #170

Closed JohnnyJayJay closed 4 years ago

JohnnyJayJay commented 4 years ago

I am currently working on Java bindings for this implementation of fluent and essentially mirroring the fluent-bundle API, as requested here.

I have encountered a rather fatal issue, though: many, if not most of the API structs do not implement Send, which effectively prevents any binding to those structs. The reason for that is that in order to move values between Java and Rust, the values need to be Send. There is no way around it that I am aware of.

The structs/types that currently lack a Send implementation and that I know are critical for JNI so far, are the following:

So, it would make my life much easier if you added an unsafe Send implementation where required. If you can't do this because people might get the idea that it's thread safe then, I'm all open for other ideas or solutions.

zbraniecki commented 4 years ago

@JohnnyJayJay does https://docs.rs/fluent-bundle/0.11.0/fluent_bundle/concurrent/type.FluentBundle.html solve it for you?

JohnnyJayJay commented 4 years ago

@JohnnyJayJay does https://docs.rs/fluent-bundle/0.11.0/fluent_bundle/concurrent/type.FluentBundle.html solve it for you?

No, since there are still parts of the API left where making bindings is impossible, even if the concurrent FluentBundle works (I'm not at this point yet, but it should). FluentArgs and FluentValue are not Send and without them I can't make proper bindings, as explained above.

zbraniecki commented 4 years ago

I see. @Manishearth - can you advise on approach to FluentArgs and FluentValue here? I suspect that we could add concurrent::* versions and use them in concurrent::FluentBundle, but I'm not sure.

Manishearth commented 4 years ago

FluentValue can be made Send with a +Send on the custom branch.

that will automatically fix FluentArgs

zbraniecki commented 4 years ago

@JohnnyJayJay

diff --git a/fluent-bundle/src/types/mod.rs b/fluent-bundle/src/types/mod.rs
index 42447e7..a510060 100644
--- a/fluent-bundle/src/types/mod.rs
+++ b/fluent-bundle/src/types/mod.rs
@@ -107,7 +107,7 @@ impl<'source> From<&ast::InlineExpression<'source>> for DisplayableNode<'source>
 }

 pub trait FluentType: fmt::Debug + AnyEq + 'static {
-    fn duplicate(&self) -> Box<dyn FluentType>;
+    fn duplicate(&self) -> Box<dyn FluentType + Send>;
     fn as_string(&self, intls: &intl_memoizer::IntlLangMemoizer) -> Cow<'static, str>;
     fn as_string_threadsafe(
         &self,
@@ -115,7 +115,7 @@ pub trait FluentType: fmt::Debug + AnyEq + 'static {
     ) -> Cow<'static, str>;
 }

-impl PartialEq for dyn FluentType {
+impl PartialEq for dyn FluentType + Send {
     fn eq(&self, other: &Self) -> bool {
         self.equals(other.as_any())
     }
@@ -155,7 +155,7 @@ impl<T: Any + PartialEq> AnyEq for T {
 pub enum FluentValue<'source> {
     String(Cow<'source, str>),
     Number(FluentNumber),
-    Custom(Box<dyn FluentType>),
+    Custom(Box<dyn FluentType + Send>),
     Error(DisplayableNode<'source>),
     None,
 }
@@ -176,7 +176,7 @@ impl<'s> Clone for FluentValue<'s> {
             FluentValue::String(s) => FluentValue::String(s.clone()),
             FluentValue::Number(s) => FluentValue::Number(s.clone()),
             FluentValue::Custom(s) => {
-                let new_value: Box<dyn FluentType> = s.duplicate();
+                let new_value: Box<dyn FluentType + Send> = s.duplicate();
                 FluentValue::Custom(new_value)
             }
             FluentValue::Error(e) => FluentValue::Error(e.clone()),

Would that solve it for you then?

JohnnyJayJay commented 4 years ago

@zbraniecki I'm not entirely sure, could you maybe push that to a separate branch so I can clone, install and see for myself?

zbraniecki commented 4 years ago

Here you go https://github.com/zbraniecki/fluent-rs/tree/custom-send

JohnnyJayJay commented 4 years ago

I appreciate the effort. Will have a look, try around with it and get back here when I have results.

JohnnyJayJay commented 4 years ago

So far, those changes indeed do the job for me. I haven't implemented custom functions and types yet, but I reckon that is doable, since I can implement Send myself there.

zbraniecki commented 4 years ago

Closing then, please, reopen if needed!