Refactor resolver errors to provide better fallbacking and return err…

…ors out of formatting (projectfluent#93) * Refactor fluent-bundle * Add DisplayableNode * Format errors from variables * Add ResolverError::Reference * Migrate more errors to specific error types * Remove ResolverError::None * Add values_ref_test * Fast-path patterns with a single placeable * Rename Env to Scope * Add functions tests * More tests added * Address reviewers feedback and add a couple more tests
kazimuth · Apr 9, 2019 · df5e247 · df5e247
1 parent c4c4745
commit df5e247
Show file tree

Hide file tree

Showing 26 changed files with 1,205 additions and 303 deletions.
diff --git a/fluent-bundle/benches/resolver.rs b/fluent-bundle/benches/resolver.rs
@@ -63,7 +63,7 @@ fn add_functions(name: &'static str, bundle: &mut FluentBundle) {
         "preferences" => {
             bundle
                 .add_function("PLATFORM", |_args, _named_args| {
-                    return Some("linux".into());
+                    return "linux".into();
                 })
                 .expect("Failed to add a function to the bundle.");
         }
@@ -92,10 +92,7 @@ fn resolver_bench(c: &mut Criterion) {
             b.iter(|| {
                 for id in &ids {
                     let (_msg, errors) = bundle.compound(id, args.as_ref()).expect("Message found");
-                    if !errors.is_empty() {
-                        println!("{:#?}", errors);
-                    }
-                    assert!(errors.len() == 0);
+                    assert!(errors.len() == 0, "Resolver errors: {:#?}", errors);
                 }
             })
         },

diff --git a/fluent-bundle/examples/external_arguments.rs b/fluent-bundle/examples/external_arguments.rs
@@ -33,7 +33,7 @@ unread-emails =
     }
 
     let mut args = HashMap::new();
-    args.insert("emailCount", FluentValue::into_number("1.0").unwrap());
+    args.insert("emailCount", FluentValue::into_number("1.0"));
 
     match bundle.format("unread-emails", Some(&args)) {
         Some((value, _)) => println!("{}", value),

diff --git a/fluent-bundle/examples/functions.rs b/fluent-bundle/examples/functions.rs
@@ -15,20 +15,20 @@ fn main() {
     // Test for a simple function that returns a string
     bundle
         .add_function("HELLO", |_args, _named_args| {
-            return Some("I'm a function!".into());
+            return "I'm a function!".into();
         })
         .expect("Failed to add a function to the bundle.");
 
     // Test for a function that accepts unnamed positional arguments
     bundle
         .add_function("MEANING_OF_LIFE", |args, _named_args| {
             if let Some(arg0) = args.get(0) {
-                if *arg0 == Some(FluentValue::Number(String::from("42"))) {
-                    return Some("The answer to life, the universe, and everything".into());
+                if *arg0 == FluentValue::Number("42".into()) {
+                    return "The answer to life, the universe, and everything".into();
                 }
             }
 
-            None
+            FluentValue::None()
         })
         .expect("Failed to add a function to the bundle.");
 
@@ -37,9 +37,9 @@ fn main() {
         .add_function("BASE_OWNERSHIP", |_args, named_args| {
             return match named_args.get("ownership") {
                 Some(FluentValue::String(ref string)) => {
-                    Some(format!("All your base belong to {}", string).into())
+                    format!("All your base belong to {}", string).into()
                 }
-                _ => None,
+                _ => FluentValue::None(),
             };
         })
         .expect("Failed to add a function to the bundle.");

diff --git a/fluent-bundle/examples/simple-app.rs b/fluent-bundle/examples/simple-app.rs
@@ -147,7 +147,7 @@ fn main() {
                 }
                 Err(err) => {
                     let mut args = HashMap::new();
-                    args.insert("input", FluentValue::from(input.to_string()));
+                    args.insert("input", FluentValue::from(input.as_str()));
                     args.insert("reason", FluentValue::from(err.to_string()));
                     let (value, _) = bundle
                         .format("input-parse-error-msg", Some(&args))

diff --git a/fluent-bundle/src/bundle.rs b/fluent-bundle/src/bundle.rs
@@ -4,21 +4,23 @@
 //! internationalization formatters, functions, environmental variables and are expected to be used
 //! together.
 
+use std::borrow::Cow;
 use std::collections::hash_map::{Entry as HashEntry, HashMap};
 
 use super::entry::{Entry, GetEntry};
 pub use super::errors::FluentError;
-use super::resolve::{Env, ResolveValue};
+use super::resolve::{resolve_value_for_entry, Scope};
 use super::resource::FluentResource;
+use super::types::DisplayableNode;
 use super::types::FluentValue;
 use fluent_locale::{negotiate_languages, NegotiationStrategy};
 use fluent_syntax::ast;
 use intl_pluralrules::{IntlPluralRules, PluralRuleType};
 
 #[derive(Debug, PartialEq)]
-pub struct Message {
-    pub value: Option<String>,
-    pub attributes: HashMap<String, String>,
+pub struct Message<'m> {
+    pub value: Option<Cow<'m, str>>,
+    pub attributes: HashMap<&'m str, Cow<'m, str>>,
 }
 
 /// A collection of localization messages for a single locale, which are meant
@@ -165,8 +167,8 @@ impl<'bundle> FluentBundle<'bundle> {
     ///
     /// // Register a fn that maps from string to string length
     /// bundle.add_function("STRLEN", |positional, _named| match positional {
-    ///     [Some(FluentValue::String(str))] => Some(FluentValue::Number(str.len().to_string())),
-    ///     _ => None,
+    ///     [FluentValue::String(str)] => FluentValue::Number(str.len().to_string().into()),
+    ///     _ => FluentValue::None(),
     /// }).expect("Failed to add a function to the bundle.");
     ///
     /// let (value, _) = bundle.format("length", None)
@@ -178,7 +180,7 @@ impl<'bundle> FluentBundle<'bundle> {
     pub fn add_function<F>(&mut self, id: &str, func: F) -> Result<(), FluentError>
     where
         F: 'bundle
-            + Fn(&[Option<FluentValue>], &HashMap<&str, FluentValue>) -> Option<FluentValue>
+            + for<'a> Fn(&[FluentValue<'a>], &HashMap<&str, FluentValue<'a>>) -> FluentValue<'a>
             + Sync
             + Send,
     {
@@ -316,7 +318,7 @@ impl<'bundle> FluentBundle<'bundle> {
     ///
     /// In all other cases `format` returns a string even if it
     /// encountered errors. Generally, during partial errors `format` will
-    /// use `'___'` to replace parts of the formatted message that it could
+    /// use ids to replace parts of the formatted message that it could
     /// not successfuly build. For more fundamental errors `format` will return
     /// the path itself as the translation.
     ///
@@ -335,56 +337,55 @@ impl<'bundle> FluentBundle<'bundle> {
     /// bundle.add_resource(&resource)
     ///     .expect("Failed to add FTL resources to the bundle.");
     ///
-    /// // The result falls back to "___"
+    /// // The result falls back to "a foo b"
     /// let (value, _) = bundle.format("foo", None)
     ///     .expect("Failed to format a message.");
-    /// assert_eq!(&value, "___");
+    /// assert_eq!(&value, "a foo b");
     /// ```
     pub fn format(
-        &self,
+        &'bundle self,
         path: &str,
-        args: Option<&HashMap<&str, FluentValue>>,
-    ) -> Option<(String, Vec<FluentError>)> {
-        let env = Env::new(self, args);
+        args: Option<&'bundle HashMap<&str, FluentValue>>,
+    ) -> Option<(Cow<'bundle, str>, Vec<FluentError>)> {
+        let mut env = Scope::new(self, args);
 
         let mut errors = vec![];
 
-        if let Some(ptr_pos) = path.find('.') {
+        let string = if let Some(ptr_pos) = path.find('.') {
             let message_id = &path[..ptr_pos];
             let message = self.entries.get_message(message_id)?;
             let attr_name = &path[(ptr_pos + 1)..];
-            for attribute in message.attributes.iter() {
-                if attribute.id.name == attr_name {
-                    match attribute.to_value(&env) {
-                        Ok(val) => {
-                            return Some((val.format(self), errors));
-                        }
-                        Err(err) => {
-                            errors.push(FluentError::ResolverError(err));
-                            // XXX: In the future we'll want to get the partial
-                            // value out of resolver and return it here.
-                            // We also expect to get a Vec or errors out of resolver.
-                            return Some((path.to_string(), errors));
-                        }
-                    }
-                }
-            }
+            let attr = message
+                .attributes
+                .iter()
+                .find(|attr| attr.id.name == attr_name)?;
+            resolve_value_for_entry(
+                &attr.value,
+                DisplayableNode::new(message.id.name, Some(attr.id.name)),
+                &mut env,
+            )
+            .to_string()
         } else {
             let message_id = path;
             let message = self.entries.get_message(message_id)?;
-            match message.to_value(&env) {
-                Ok(val) => {
-                    let s = val.format(self);
-                    return Some((s, errors));
-                }
-                Err(err) => {
-                    errors.push(FluentError::ResolverError(err));
-                    return Some((message_id.to_string(), errors));
-                }
-            }
+            message
+                .value
+                .as_ref()
+                .map(|value| {
+                    resolve_value_for_entry(
+                        value,
+                        DisplayableNode::new(message.id.name, None),
+                        &mut env,
+                    )
+                })?
+                .to_string()
+        };
+
+        for err in env.errors {
+            errors.push(err.into());
         }
 
-        None
+        Some((string, errors))
     }
 
     /// Formats both the message value and attributes identified by `message_id`
@@ -412,8 +413,8 @@ impl<'bundle> FluentBundle<'bundle> {
     ///
     /// let (message, _) = bundle.compound("login-input", None)
     ///     .expect("Failed to format a message.");
-    /// assert_eq!(message.value, Some("Predefined value".to_string()));
-    /// assert_eq!(message.attributes.get("title"), Some(&"Type your login email".to_string()));
+    /// assert_eq!(message.value, Some("Predefined value".into()));
+    /// assert_eq!(message.attributes.get("title"), Some(&"Type your login email".into()));
     /// ```
     ///
     /// # Errors
@@ -422,46 +423,40 @@ impl<'bundle> FluentBundle<'bundle> {
     ///
     /// In all other cases `compound` returns a message even if it
     /// encountered errors. Generally, during partial errors `compound` will
-    /// use `'___'` to replace parts of the formatted message that it could
+    /// use ids to replace parts of the formatted message that it could
     /// not successfuly build. For more fundamental errors `compound` will return
     /// the path itself as the translation.
     ///
     /// The second term of the tuple will contain any extra error information
     /// gathered during formatting. A caller may safely ignore the extra errors
     /// if the fallback formatting policies are acceptable.
     pub fn compound(
-        &self,
+        &'bundle self,
         message_id: &str,
-        args: Option<&HashMap<&str, FluentValue>>,
-    ) -> Option<(Message, Vec<FluentError>)> {
+        args: Option<&'bundle HashMap<&str, FluentValue>>,
+    ) -> Option<(Message<'bundle>, Vec<FluentError>)> {
+        let mut env = Scope::new(self, args);
         let mut errors = vec![];
-
-        let env = Env::new(self, args);
         let message = self.entries.get_message(message_id)?;
 
-        let value = message
-            .value
-            .as_ref()
-            .and_then(|value| match value.to_value(&env) {
-                Ok(value) => Some(value.format(self)),
-                Err(err) => {
-                    errors.push(FluentError::ResolverError(err));
-                    None
-                }
-            });
+        let value = message.value.as_ref().map(|value| {
+            resolve_value_for_entry(value, DisplayableNode::new(message.id.name, None), &mut env)
+                .to_string()
+        });
 
         let mut attributes = HashMap::new();
 
         for attr in message.attributes.iter() {
-            match attr.to_value(&env) {
-                Ok(value) => {
-                    let val = value.format(self);
-                    attributes.insert(attr.id.name.to_owned(), val);
-                }
-                Err(err) => {
-                    errors.push(FluentError::ResolverError(err));
-                }
-            }
+            let val = resolve_value_for_entry(
+                &attr.value,
+                DisplayableNode::new(message.id.name, Some(attr.id.name)),
+                &mut env,
+            );
+            attributes.insert(attr.id.name, val.to_string());
+        }
+
+        for err in env.errors {
+            errors.push(err.into());
         }
 
         Some((Message { value, attributes }, errors))

diff --git a/fluent-bundle/src/entry.rs b/fluent-bundle/src/entry.rs
@@ -1,12 +1,12 @@
 //! `Entry` is used to store Messages, Terms and Functions in `FluentBundle` instances.
 use std::collections::hash_map::HashMap;
 
-use super::types::FluentValue;
+use super::types::*;
 use fluent_syntax::ast;
 
 type FluentFunction<'bundle> = Box<
     'bundle
-        + Fn(&[Option<FluentValue>], &HashMap<&str, FluentValue>) -> Option<FluentValue>
+        + for<'a> Fn(&[FluentValue<'a>], &HashMap<&str, FluentValue<'a>>) -> FluentValue<'a>
         + Send
         + Sync,
 >;