diff --git a/fluent-bundle/src/resolver/errors.rs b/fluent-bundle/src/resolver/errors.rs index 7606faba..1a456ed8 100644 --- a/fluent-bundle/src/resolver/errors.rs +++ b/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"), } diff --git a/fluent-bundle/src/resolver/expression.rs b/fluent-bundle/src/resolver/expression.rs index ce030e4c..377cef19 100644 --- a/fluent-bundle/src/resolver/expression.rs +++ b/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, + 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) + } + } + } +} diff --git a/fluent-bundle/src/resolver/inline_expression.rs b/fluent-bundle/src/resolver/inline_expression.rs index 3f8c8d4f..83b7c4cb 100644 --- a/fluent-bundle/src/resolver/inline_expression.rs +++ b/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), } } } diff --git a/fluent-bundle/src/resolver/pattern.rs b/fluent-bundle/src/resolver/pattern.rs index e20bfcde..dc331739 100644 --- a/fluent-bundle/src/resolver/pattern.rs +++ b/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(); diff --git a/fluent-bundle/src/resolver/scope.rs b/fluent-bundle/src/resolver/scope.rs index 1ddff1a4..441ea277 100644 --- a/fluent-bundle/src/resolver/scope.rs +++ b/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( &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, + 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, + 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( &mut self, w: &mut W, diff --git a/fluent-bundle/tests/fixtures/macros.yaml b/fluent-bundle/tests/fixtures/macros.yaml index eb0c2d40..c0217b8d 100644 --- a/fluent-bundle/tests/fixtures/macros.yaml +++ b/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 \ No newline at end of file + value: It + - + name: For term with multiple attributes + asserts: + - + id: more-than-one-attr + value: is floating around happily diff --git a/fluent-bundle/tests/resolver_fixtures.rs b/fluent-bundle/tests/resolver_fixtures.rs index e242a390..8114cf5c 100644 --- a/fluent-bundle/tests/resolver_fixtures.rs +++ b/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"); diff --git a/fluent-syntax/benches/parser.rs b/fluent-syntax/benches/parser.rs index 2397044d..71fe96e5 100644 --- a/fluent-syntax/benches/parser.rs +++ b/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> { diff --git a/fluent-syntax/src/ast/mod.rs b/fluent-syntax/src/ast/mod.rs index 273aceaf..1027e6e3 100644 --- a/fluent-syntax/src/ast/mod.rs +++ b/fluent-syntax/src/ast/mod.rs @@ -1444,6 +1444,7 @@ pub enum Expression { /// [key1] Value 1 /// *[other] Value 2 /// } + /// Invariant: exactly 1 variant must have default: true. /// ``` Select { selector: InlineExpression, diff --git a/fluent-syntax/tests/parser_fixtures.rs b/fluent-syntax/tests/parser_fixtures.rs index eb8b9d1f..a067d38e 100644 --- a/fluent-syntax/tests/parser_fixtures.rs +++ b/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()) {