More on Builtin.contribute and Builtin.get_functions. (#1298)

Some missing details that I discovered regarding these methods, and a
pytest to test the right behaviour:

* Fix the regular expression used in get_function to support multiline
docstrings.
* Fix how the full name of builtins are built in `contribute` when the
class does not have a `context` attribute.
* Add tests for Builtin.contribute

More tests can be added later.

mathics/core/builtin.py

@@ -239,12 +239,16 @@ def __init__(self, *args, **kwargs):
     def contribute(self, definitions: Definitions, is_pymodule=False):
         from mathics.core.parser import parse_builtin_rule
 
-        # Set the default context
-        if not self.context:
-            self.context = "Pymathics`" if is_pymodule else "System`"
         name = self.get_name()
         attributes = self.attributes
         options = {}
+        # Set the default context
+        if not self.context:
+            self.context = "Pymathics`" if is_pymodule else "System`"
+            # get_name takes the context from the class, not from the
+            # instance, so even if we set the context here,
+            # self.get_name() does not includes the context.
+            name = self.context + name
 
         # - 'Strict': warn and fail with unsupported options
         # - 'Warn': warn about unsupported options, but continue
@@ -468,7 +472,15 @@ def get_functions(self, prefix="eval", is_pymodule=False):
                 if pattern is None:  # Fixes PyPy bug
                     continue
                 else:
-                    m = re.match(r"[(]([\w,]+),[)]\:\s*(.*)", pattern)
+                    # TODO: consider to use a more sophisticated
+                    # regular expression, which handles breaklines
+                    # more properly, that supports format names
+                    # with contexts (context`name) and be less
+                    # fragile against leaving spaces between the
+                    # elements.
+                    m = re.match(
+                        r"[(]([\w,]+),[ ]*[)]\:\s*(.*)", pattern.replace("\n", " ")
+                    )
                 if m is not None:
                     attrs = m.group(1).split(",")
                     pattern = m.group(2)
@@ -491,7 +503,7 @@ def get_functions(self, prefix="eval", is_pymodule=False):
                     yield (pattern, function)
 
     @staticmethod
-    def get_option(options, name, evaluation, pop=False):
+    def get_option(options, name, evaluation, pop=False) -> Optional[BaseElement]:
         return get_option(options, name, evaluation, pop)
 
     def _get_unavailable_function(self) -> Optional[Callable]:
@@ -503,7 +515,12 @@ def _get_unavailable_function(self) -> Optional[Callable]:
         requires = getattr(self, "requires", [])
         return None if check_requires_list(requires) else UnavailableFunction(self)
 
-    def get_option_string(self, *params):
+    def get_option_string(self, *params) -> Tuple[Optional[str], Optional[BaseElement]]:
+        """
+        Return a tuple of a `str` representing the option name,
+        and the proper Mathics value of the option.
+        If the value does not have a name, the name is None.
+        """
         s = self.get_option(*params)
         if isinstance(s, String):
             return s.get_string_value(), s
@@ -789,7 +806,9 @@ def check_requires_list(requires: list) -> bool:
     return True
 
 
-def get_option(options: dict, name, evaluation, pop=False, evaluate=True):
+def get_option(
+    options: dict, name, evaluation, pop=False, evaluate=True
+) -> Optional[BaseElement]:
     # we do not care whether an option X is given as System`X,
     # Global`X, or with any prefix from $ContextPath for that
     # matter. Also, the quoted string form "X" is ok. all these

test/core/test_builtin.py

@@ -0,0 +1,131 @@
+"""
+Test how builtins are loaded
+"""
+
+from mathics.builtin.makeboxes import MakeBoxes
+from mathics.core.builtin import Builtin
+from mathics.core.definitions import Definitions
+from mathics.core.element import BaseElement
+from mathics.core.evaluation import Evaluation
+from mathics.core.load_builtin import import_and_load_builtins
+from mathics.core.symbols import SymbolTrue
+
+
+class TrialBuiltin(Builtin):
+    """
+    <dl>
+      <dt>'TrialBuiltin'[$x$]
+      <dd>nothing
+    </dl>
+    """
+
+    options = {
+        "FakeOption": "True",
+    }
+    messages = {
+        "nomsg": "Test message `1`.",
+    }
+
+    # A Downvalue
+    def eval_downvalue(self, expr, evaluation):
+        """expr: TrialBuiltin[_:Symbol]"""
+        return
+
+    # An Upvalue
+    def eval_upvalue(self, expr, x, evaluation):
+        """expr: G[TrialBuiltin[x_:Symbol]]"""
+        return
+
+    # A format rule for a custom format
+    def format_parm1(self, expr, x, evaluation):
+        """(CustomForm,): expr: TrialBuiltin[x_:Symbol]"""
+        return
+
+    # A MakeBoxes rule, using a name pattern,
+    # with a line break.
+    def format_parm2(self, expr, x, y, evaluation):
+        """(MakeBoxes, ):expr: TrialBuiltin[x_:Symbol,
+        y_]
+        """
+        return
+
+    # A format rule for OutputForm
+    def format_parmb(self, expr, x, y, evaluation):
+        """(OutputForm,): expr: TrialBuiltin[x_:Symbol,
+        y:P|Q]
+        """
+        return
+
+    # A general format rule.
+    def format_parm_general(self, expr, x, y, evaluation):
+        """expr: TrialBuiltin[x_:Symbol,
+        y:P|Q]
+        """
+        return
+
+
+# This happens before any call to import_and_load_builtins
+DEFINITIONS = Definitions()
+EVALUATION = Evaluation(DEFINITIONS)
+MakeBoxes(expression=False).contribute(DEFINITIONS)
+TrialBuiltin(expression=False).contribute(DEFINITIONS)
+
+
+def test_other_attributes_builtin():
+    import_and_load_builtins()
+    definitions = Definitions(add_builtin=True)
+    definition = definitions.builtin["System`Plus"]
+
+    builtin = definition.builtin
+    assert builtin.context == "System`"
+    assert builtin.get_name() == "System`Plus"
+    assert builtin.get_name(short=True) == "Plus"
+
+
+def test_builtin_get_functions():
+    definitions = DEFINITIONS
+    MakeBoxes.context = "System`"
+    MakeBoxes(expression=False).contribute(definitions)
+    builtin = definitions.builtin["System`TrialBuiltin"].builtin
+    evalrules = list(builtin.get_functions("eval"))
+    for r in evalrules:
+        assert isinstance(r, tuple) and len(r) == 2
+        assert isinstance(r[0], BaseElement)
+
+    evalrules = list(builtin.get_functions("format_"))
+    for r in evalrules:
+        assert isinstance(r, tuple) and len(r) == 2
+        # For formatvalues, the pattern can be both a BaseElement
+        # or a tuple of a string with a format name and a BaseElement.
+        if isinstance(r[0], tuple):
+            assert len(r[0]) == 2
+            assert isinstance(r[0][0], list)
+            assert isinstance(r[0][1], BaseElement)
+        else:
+            assert isinstance(r[0], BaseElement)
+
+
+def test_contribute_builtin():
+    """Test for Builtin.contribute."""
+
+    definitions = Definitions()
+    evaluation = Evaluation(definitions)
+    MakeBoxes(expression=False).contribute(definitions)
+
+    TrialBuiltin(expression=False).contribute(definitions)
+    assert "System`TrialBuiltin" in definitions.builtin.keys()
+    definition = definitions.get_definition("System`TrialBuiltin")
+    # Check that the formats are loaded into the right places.
+    assert "System`MakeBoxes" in definition.formatvalues
+    assert "System`CustomForm" in definition.formatvalues
+    assert "System`OutputForm" in definition.formatvalues
+    # Test if the rules are loaded into the right place.
+    assert definition.upvalues
+    assert definition.downvalues
+    assert definition.messages
+    assert definition.options
+    builtin = definition.builtin
+    assert builtin.get_option_string(definition.options, "FakeOption", evaluation) == (
+        "True",
+        SymbolTrue,
+    )