resolver - store attributes in _messages_and_terms dict

This has the result of performance optimization in a number of places,
because we don't need to traverse attributes each time (with probably a very
small startup cost), and simplifying quite a bit of logic.

Also, tests for the corner cases found by looking at code coverage for the
changes, and cleanups of places that were handling message IDs

Author: spookylukey

fluent.runtime/fluent/runtime/__init__.py

@@ -9,6 +9,7 @@
 
 from .builtins import BUILTINS
 from .resolver import resolve
+from .utils import ATTRIBUTE_SEPARATOR, TERM_SIGIL, add_message_and_attrs_to_store, ast_to_id
 
 
 class FluentBundle(object):
@@ -41,35 +42,25 @@ def add_messages(self, source):
         # TODO - warn/error about duplicates
         for item in resource.body:
             if isinstance(item, (Message, Term)):
-                prefix = "-" if isinstance(item, Term) else ""
-                full_id = prefix + item.id.name
+                full_id = ast_to_id(item)
                 if full_id not in self._messages_and_terms:
-                    self._messages_and_terms[full_id] = item
+                    # We add attributes to the store to enable faster looker
+                    # later, and more direct code in some instances.
+                    add_message_and_attrs_to_store(self._messages_and_terms, full_id, item)
 
     def has_message(self, message_id):
-        if message_id.startswith('-'):
+        if message_id.startswith(TERM_SIGIL) or ATTRIBUTE_SEPARATOR in message_id:
             return False
         return message_id in self._messages_and_terms
 
     def format(self, message_id, args=None):
-        message = self._get_message(message_id)
+        if message_id.startswith(TERM_SIGIL):
+            raise LookupError(message_id)
+        message = self._messages_and_terms[message_id]
         if args is None:
             args = {}
         return resolve(self, message, args)
 
-    def _get_message(self, message_id):
-        if message_id.startswith('-'):
-            raise LookupError(message_id)
-        if '.' in message_id:
-            name, attr_name = message_id.split('.', 1)
-            msg = self._messages_and_terms[name]
-            for attribute in msg.attributes:
-                if attribute.id.name == attr_name:
-                    return attribute.value
-            raise LookupError(message_id)
-        else:
-            return self._messages_and_terms[message_id]
-
     def _get_babel_locale(self):
         for l in self.locales:
             try:

fluent.runtime/fluent/runtime/resolver.py

@@ -7,7 +7,7 @@
 import attr
 import six
 
-from fluent.syntax.ast import (AttributeExpression, CallExpression, Identifier, Message, MessageReference,
+from fluent.syntax.ast import (Attribute, AttributeExpression, CallExpression, Identifier, Message, MessageReference,
                                NumberLiteral, Pattern, Placeable, SelectExpression, StringLiteral, Term, TermReference,
                                TextElement, VariableReference, VariantExpression, VariantList)
 
@@ -190,24 +190,31 @@ def handle_term_reference(term_reference, env):
 
 
 def lookup_reference(ref, env):
+    """
+    Given a MessageReference, TermReference or AttributeExpression, returns the
+    AST node, or FluentNone if not found, including fallback logic
+    """
     ref_id = reference_to_id(ref)
-    if "." in ref_id:
-        parent_id, attr_name = ref_id.split('.')
-        if parent_id not in env.context._messages_and_terms:
-            env.errors.append(unknown_reference_error_obj(ref_id))
-            return FluentNone(ref_id)
-        else:
-            parent = env.context._messages_and_terms[parent_id]
-            for attribute in parent.attributes:
-                if attribute.id.name == attr_name:
-                    return attribute.value
-            env.errors.append(unknown_reference_error_obj(ref_id))
-            return parent
-    else:
-        if ref_id not in env.context._messages_and_terms:
-            env.errors.append(unknown_reference_error_obj(ref_id))
-            return FluentNone(ref_id)
-        return env.context._messages_and_terms[ref_id]
+
+    message = None
+    try:
+        message = env.context._messages_and_terms[ref_id]
+    except LookupError:
+        env.errors.append(unknown_reference_error_obj(ref_id))
+
+        if isinstance(ref, AttributeExpression):
+            parent_id = reference_to_id(ref.ref)
+            try:
+                message = env.context._messages_and_terms[parent_id]
+            except LookupError:
+                # Don't add error here, because we already added error for the
+                # actual thing we were looking for.
+                pass
+
+    if message is None:
+        message = FluentNone(ref_id)
+
+    return message
 
 
 @handle.register(FluentNone)
@@ -247,6 +254,11 @@ def handle_attribute_expression(attribute_ref, env):
     return handle(lookup_reference(attribute_ref, env), env)
 
 
+@handle.register(Attribute)
+def handle_attribute(attribute, env):
+    return handle(attribute.value, env)
+
+
 @handle.register(VariantList)
 def handle_variant_list(variant_list, env):
     return select_from_variant_list(variant_list, env, None)

fluent.runtime/fluent/runtime/utils.py

@@ -1,7 +1,27 @@
-from fluent.syntax.ast import AttributeExpression, TermReference
+from fluent.syntax.ast import AttributeExpression, Term, TermReference
 
 from .errors import FluentReferenceError
 
+TERM_SIGIL = '-'
+ATTRIBUTE_SEPARATOR = '.'
+
+
+def ast_to_id(ast):
+    """
+    Returns a string reference for a Term or Message
+    """
+    return (TERM_SIGIL if isinstance(ast, Term) else '') + ast.id.name
+
+
+def add_message_and_attrs_to_store(store, ref_id, item, is_parent=True):
+    store[ref_id] = item
+    if is_parent:
+        for attr in item.attributes:
+            add_message_and_attrs_to_store(store,
+                                           _make_attr_id(ref_id, attr.id.name),
+                                           attr,
+                                           is_parent=False)
+
 
 def numeric_to_native(val):
     """
@@ -28,15 +48,22 @@ def reference_to_id(ref):
        -term.attr
     """
     if isinstance(ref, AttributeExpression):
-        return "{0}.{1}".format(reference_to_id(ref.ref),
-                                ref.name.name)
-    return ('-' if isinstance(ref, TermReference) else '') + ref.id.name
+        return _make_attr_id(reference_to_id(ref.ref),
+                             ref.name.name)
+    return (TERM_SIGIL if isinstance(ref, TermReference) else '') + ref.id.name
 
 
 def unknown_reference_error_obj(ref_id):
-    if '.' in ref_id:
+    if ATTRIBUTE_SEPARATOR in ref_id:
         return FluentReferenceError("Unknown attribute: {0}".format(ref_id))
-    elif ref_id.startswith('-'):
+    elif ref_id.startswith(TERM_SIGIL):
         return FluentReferenceError("Unknown term: {0}".format(ref_id))
     else:
         return FluentReferenceError("Unknown message: {0}".format(ref_id))
+
+
+def _make_attr_id(parent_ref_id, attr_name):
+    """
+    Given a parent id and the attribute name, return the attribute id
+    """
+    return ''.join([parent_ref_id, ATTRIBUTE_SEPARATOR, attr_name])

fluent.runtime/tests/format/test_attributes.py

@@ -106,6 +106,7 @@ def setUp(self):
             ref-qux = { qux.missing }
             attr-only =
                      .attr  = Attr Only Attribute
+            ref-double-missing = { missing.attr }
         """))
 
     def test_falls_back_for_msg_with_string_value_and_no_attributes(self):
@@ -147,3 +148,8 @@ def test_attr_only_attribute(self):
         val, errs = self.ctx.format('attr-only.attr', {})
         self.assertEqual(val, 'Attr Only Attribute')
         self.assertEqual(len(errs), 0)
+
+    def test_missing_message_and_attribute(self):
+        val, errs = self.ctx.format('ref-double-missing', {})
+        self.assertEqual(val, 'missing.attr')
+        self.assertEqual(errs, [FluentReferenceError('Unknown attribute: missing.attr')])