Resolve more pattern types into &str references

fluent-bundle/src/resolver/errors.rs

@@ -53,7 +53,6 @@ where
 pub enum ResolverError {
     Reference(ReferenceKind),
     NoValue(String),
-    MissingDefault,
     Cyclic,
     TooManyPlaceables,
 }
@@ -82,7 +81,6 @@ impl std::fmt::Display for ResolverError {
                 ReferenceKind::Variable { id } => write!(f, "Unknown variable: ${}", id),
             },
             Self::NoValue(id) => write!(f, "No value: {}", id),
-            Self::MissingDefault => f.write_str("No default"),
             Self::Cyclic => f.write_str("Cyclical dependency detected"),
             Self::TooManyPlaceables => f.write_str("Too many placeables"),
         }

fluent-bundle/src/resolver/expression.rs

@@ -7,7 +7,7 @@ use std::fmt;
 use fluent_syntax::ast;
 
 use crate::memoizer::MemoizerKind;
-use crate::resolver::{ResolveValue, ResolverError};
+use crate::resolver::ResolveValue;
 use crate::resource::FluentResource;
 use crate::types::FluentValue;
 
@@ -43,13 +43,12 @@ impl<'bundle> WriteValue<'bundle> for ast::Expression<&'bundle str> {
                     _ => {}
                 }
 
-                for variant in variants {
-                    if variant.default {
-                        return variant.value.write(w, scope);
-                    }
-                }
-                scope.add_error(ResolverError::MissingDefault);
-                Ok(())
+                variants
+                    .iter()
+                    .find(|variant| variant.default)
+                    .expect("select expressions have a default variant")
+                    .value
+                    .write(w, scope)
             }
         }
     }
@@ -64,3 +63,44 @@ impl<'bundle> WriteValue<'bundle> for ast::Expression<&'bundle str> {
         }
     }
 }
+
+impl<'bundle> ResolveValue<'bundle> for ast::Expression<&'bundle str> {
+    fn resolve<'ast, 'args, 'errors, R, M>(
+        &'ast self,
+        scope: &mut Scope<'bundle, 'ast, 'args, 'errors, R, M>,
+    ) -> FluentValue<'bundle>
+    where
+        R: Borrow<FluentResource>,
+        M: MemoizerKind,
+    {
+        match self {
+            Self::Inline(exp) => exp.resolve(scope),
+            Self::Select { selector, variants } => {
+                let selector = selector.resolve(scope);
+                match selector {
+                    FluentValue::String(_) | FluentValue::Number(_) => {
+                        for variant in variants {
+                            let key = match variant.key {
+                                ast::VariantKey::Identifier { name } => name.into(),
+                                ast::VariantKey::NumberLiteral { value } => {
+                                    FluentValue::try_number(value)
+                                }
+                            };
+                            if key.matches(&selector, scope) {
+                                return variant.value.resolve(scope);
+                            }
+                        }
+                    }
+                    _ => {}
+                }
+
+                variants
+                    .iter()
+                    .find(|variant| variant.default)
+                    .expect("select expressions have a default variant")
+                    .value
+                    .resolve(scope)
+            }
+        }
+    }
+}

fluent-bundle/src/resolver/inline_expression.rs

@@ -183,14 +183,72 @@ impl<'bundle> ResolveValue<'bundle> for ast::InlineExpression<&'bundle str> {
                     let result = func(resolved_positional_args.as_slice(), &resolved_named_args);
                     result
                 } else {
+                    scope.add_error(self.into());
+                    FluentValue::Error
+                }
+            }
+            ast::InlineExpression::MessageReference { id, attribute } => {
+                if let Some(msg) = scope.bundle.get_entry_message(id.name) {
+                    if let Some(attr) = attribute {
+                        msg.attributes
+                            .iter()
+                            .find_map(|a| {
+                                if a.id.name == attr.name {
+                                    Some(scope.track_resolve(&a.value))
+                                } else {
+                                    None
+                                }
+                            })
+                            .unwrap_or_else(|| {
+                                scope.add_error(self.into());
+                                FluentValue::Error
+                            })
+                    } else {
+                        msg.value
+                            .as_ref()
+                            .map(|value| scope.track_resolve(value))
+                            .unwrap_or_else(|| {
+                                scope.add_error(ResolverError::NoValue(id.name.to_string()));
+                                FluentValue::Error
+                            })
+                    }
+                } else {
+                    scope.add_error(self.into());
                     FluentValue::Error
                 }
             }
-            _ => {
-                let mut result = String::new();
-                self.write(&mut result, scope).expect("Failed to write");
-                result.into()
+            ast::InlineExpression::TermReference {
+                id,
+                attribute,
+                arguments,
+            } => {
+                let (_, resolved_named_args) = scope.get_arguments(arguments.as_ref());
+
+                scope.local_args = Some(resolved_named_args);
+                let result = scope
+                    .bundle
+                    .get_entry_term(id.name)
+                    .and_then(|term| {
+                        if let Some(attr) = attribute {
+                            term.attributes.iter().find_map(|a| {
+                                if a.id.name == attr.name {
+                                    Some(scope.track_resolve(&a.value))
+                                } else {
+                                    None
+                                }
+                            })
+                        } else {
+                            Some(scope.track_resolve(&term.value))
+                        }
+                    })
+                    .unwrap_or_else(|| {
+                        scope.add_error(self.into());
+                        FluentValue::Error
+                    });
+                scope.local_args = None;
+                result
             }
+            ast::InlineExpression::Placeable { expression } => expression.resolve(scope),
         }
     }
 }

fluent-bundle/src/resolver/pattern.rs

@@ -91,13 +91,33 @@ impl<'bundle> ResolveValue<'bundle> for ast::Pattern<&'bundle str> {
     {
         let len = self.elements.len();
 
+        // If there is only 1 element, then it is more efficient to attempt to resolve as a message
+        // reference that can be passed back as a Cow::Borrowed rather than writing a new String.
         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());
-            }
+            match &self.elements[0] {
+                &ast::PatternElement::TextElement { value } => {
+                    return scope
+                        .bundle
+                        .transform
+                        .map_or_else(|| value.into(), |transform| transform(value).into());
+                }
+                ast::PatternElement::Placeable { expression } => {
+                    let before = scope.placeables;
+                    scope.placeables += 1;
+
+                    let res = scope.maybe_track_resolve(self, expression);
+                    if !matches!(res, FluentValue::Error) {
+                        return res;
+                    }
+
+                    // when hitting an error, reset scope state and format using writer below to write error information
+                    scope.placeables = before;
+                    scope.dirty = false;
+                    if let Some(err) = &mut scope.errors {
+                        err.pop();
+                    }
+                }
+            };
         }
 
         let mut result = String::new();

fluent-bundle/src/resolver/scope.rs

@@ -78,6 +78,10 @@ impl<'bundle, 'ast, 'args, 'errors, R, M> Scope<'bundle, 'ast, 'args, 'errors, R
         }
     }
 
+    /// Cyclic pattern reference detection used in expression resolvers.
+    ///
+    /// Writes an error as soon as an identical pattern is encountered more than once,
+    /// which would lead to an infinite loop.
     pub fn track<W>(
         &mut self,
         w: &mut W,
@@ -102,6 +106,49 @@ impl<'bundle, 'ast, 'args, 'errors, R, M> Scope<'bundle, 'ast, 'args, 'errors, R
         }
     }
 
+    /// Similar to [`Scope::maybe_track`], but resolves to a value
+    /// instead of writing to a Writer instance.
+    pub fn maybe_track_resolve(
+        &mut self,
+        pattern: &'ast ast::Pattern<&'bundle str>,
+        exp: &'ast ast::Expression<&'bundle str>,
+    ) -> FluentValue<'bundle>
+    where
+        R: Borrow<FluentResource>,
+        M: MemoizerKind,
+    {
+        if self.travelled.is_empty() {
+            self.travelled.push(pattern);
+        }
+        let res = exp.resolve(self);
+        if self.dirty {
+            FluentValue::Error
+        } else {
+            res
+        }
+    }
+
+    /// Similar to [`Scope::track`], but resolves to a value
+    /// instead of writing to a Writer instance.
+    pub fn track_resolve(
+        &mut self,
+        pattern: &'ast ast::Pattern<&'bundle str>,
+    ) -> FluentValue<'bundle>
+    where
+        R: Borrow<FluentResource>,
+        M: MemoizerKind,
+    {
+        if self.travelled.contains(&pattern) {
+            self.add_error(ResolverError::Cyclic);
+            FluentValue::Error
+        } else {
+            self.travelled.push(pattern);
+            let result = pattern.resolve(self);
+            self.travelled.pop();
+            result
+        }
+    }
+
     pub fn write_ref_error<W>(
         &mut self,
         w: &mut W,

fluent-bundle/tests/fixtures/macros.yaml

@@ -313,6 +313,7 @@ suites:
                     *[traditional] neuter
                       [chicago] feminine
                   }
+                  .floats = true
               ref-attr = {-ship.gender ->
                 *[masculine] He
                   [feminine] She
@@ -333,6 +334,10 @@ suites:
                   [feminine] She
                   [neuter] It
               }
+              more-than-one-attr = {-ship.floats ->
+                 *[false] is sinking
+                  [true] is floating around happily
+              }
         tests:
           -
             name: Not parameterized, no externals
@@ -389,4 +394,10 @@ suites:
                 id: call-attr-with-other-arg
                 args:
                   style: chicago
-                value: It
+                value: It
+          -
+            name: For term with multiple attributes
+            asserts:
+              -
+                id: more-than-one-attr
+                value: is floating around happily

fluent-bundle/tests/resolver_fixtures.rs

@@ -334,7 +334,6 @@ fn test_errors(errors: &[FluentError], reference: Option<&[TestError]>) {
                 ResolverError::TooManyPlaceables => {
                     assert_eq!(reference.error_type, "TooManyPlaceables");
                 }
-                _ => unimplemented!(),
             },
             FluentError::ParserError(_) => {
                 assert_eq!(reference.error_type, "Parser");

fluent-syntax/benches/parser.rs

@@ -18,7 +18,7 @@ fn get_resources(tests: &[&'static str]) -> HashMap<&'static str, String> {
         let path = format!("./benches/{}", test);
         ftl_strings.insert(*test, read_file(&path).expect("Couldn't load file"));
     }
-    return ftl_strings;
+    ftl_strings
 }
 
 fn get_ctxs(tests: &[&'static str]) -> HashMap<&'static str, Vec<String>> {

fluent-syntax/src/ast/mod.rs

@@ -1444,6 +1444,7 @@ pub enum Expression<S> {
     ///     [key1] Value 1
     ///    *[other] Value 2
     /// }
+    /// Invariant: exactly 1 variant must have default: true.
     /// ```
     Select {
         selector: InlineExpression<S>,

fluent-syntax/tests/parser_fixtures.rs

@@ -27,7 +27,7 @@ fn parse_fixtures_compare() {
 
         let reference_path = path.replace(".ftl", ".json");
         let reference_file = read_file(&reference_path, true).unwrap();
-        let ftl_file = read_file(&path, false).unwrap();
+        let ftl_file = read_file(path, false).unwrap();
 
         println!("Parsing: {:#?}", path);
         let target_ast = match parse(ftl_file) {
@@ -72,7 +72,7 @@ fn parse_bench_fixtures() {
             file_name.replace(".ftl", ".json")
         );
         let reference_file = read_file(&reference_path, true).unwrap();
-        let ftl_file = read_file(&path, false).unwrap();
+        let ftl_file = read_file(path, false).unwrap();
 
         println!("Parsing: {:#?}", path);
         let target_ast = match parse(ftl_file) {
@@ -106,7 +106,7 @@ fn parse_bench_fixtures() {
                 file_name.replace(".ftl", ".json")
             );
             let reference_file = read_file(&reference_path, true).unwrap();
-            let ftl_file = read_file(&path, false).unwrap();
+            let ftl_file = read_file(path, false).unwrap();
 
             println!("Parsing: {:#?}", path);
             let target_ast = match parse(ftl_file.clone()) {