fix MRO for multimethod.__call__ using iterative algorithm.

Add tests MRO for inherited multimethods with multiple inheritance
Add tests for inherited and overridden multimethods

7 files changed, 137 insertions, 10 deletions

diff --git a/lib/spack/spack/multimethod.py b/lib/spack/spack/multimethod.py
index 7682ca063f..b95c8f2341 100644
--- a/lib/spack/spack/multimethod.py
+++ b/lib/spack/spack/multimethod.py
@@ -25,6 +25,7 @@ depending on the scenario, regular old conditionals might be clearer,
 so package authors should use their judgement.
 """
 import functools
+import inspect
 
 from llnl.util.lang import caller_locals
 
@@ -107,14 +108,26 @@ class SpecMultiMethod(object):
             return self.default(package_self, *args, **kwargs)
 
         else:
-            superclass = super(package_self.__class__, package_self)
-            superclass_fn = getattr(superclass, self.__name__, None)
-            if callable(superclass_fn):
-                return superclass_fn(*args, **kwargs)
-            else:
-                raise NoSuchMethodError(
-                    type(package_self), self.__name__, spec,
-                    [m[0] for m in self.method_list])
+            # Unwrap MRO by hand because super binds to the subclass
+            # and causes infinite recursion for inherited methods
+            for cls in inspect.getmro(package_self.__class__)[1:]:
+                superself = cls.__dict__.get(self.__name__, None)
+                if isinstance(superself, self.__class__):
+                    # Parent class method is a multimethod
+                    # check it locally for methods, conditional or default
+                    # Do not recurse, that will mess up MRO
+                    for spec, method in superself.method_list:
+                        if package_self.spec.satisfies(spec):
+                            return method(package_self, *args, **kwargs)
+                    if superself.default:
+                        return superself.default(package_self, *args, **kwargs)
+                elif superself:
+                    return superself(package_self, *args, **kwargs)
+
+            raise NoSuchMethodError(
+                type(package_self), self.__name__, package_self.spec,
+                [m[0] for m in self.method_list]
+            )
 
     def __str__(self):
         return "SpecMultiMethod {\n\tdefault: %s,\n\tspecs: %s\n}" % (
diff --git a/lib/spack/spack/test/multimethod.py b/lib/spack/spack/test/multimethod.py
index b475b3ae93..35ecce7aab 100644
--- a/lib/spack/spack/test/multimethod.py
+++ b/lib/spack/spack/test/multimethod.py
@@ -117,7 +117,44 @@ def test_virtual_dep_match(pkg_name):
 
 def test_multimethod_with_base_class(pkg_name):
     pkg = spack.repo.get(pkg_name + '@3')
-    assert pkg.base_method() == "subclass_method"
+    assert pkg.base_method() == pkg.spec.name
 
     pkg = spack.repo.get(pkg_name + '@1')
     assert pkg.base_method() == "base_method"
+
+
+def test_multimethod_inherited_and_overridden():
+    pkg = spack.repo.get('multimethod-inheritor@1.0')
+    assert pkg.inherited_and_overridden() == 'inheritor@1.0'
+
+    pkg = spack.repo.get('multimethod-inheritor@2.0')
+    assert pkg.inherited_and_overridden() == 'base@2.0'
+
+    pkg = spack.repo.get('multimethod@1.0')
+    assert pkg.inherited_and_overridden() == 'base@1.0'
+
+    pkg = spack.repo.get('multimethod@2.0')
+    assert pkg.inherited_and_overridden() == 'base@2.0'
+
+
+def test_multimethod_diamond_inheritance():
+    pkg = spack.repo.get('multimethod-diamond@1.0')
+    assert pkg.diamond_inheritance() == 'base_package'
+
+    pkg = spack.repo.get('multimethod-base@1.0')
+    assert pkg.diamond_inheritance() == 'base_package'
+
+    pkg = spack.repo.get('multimethod-diamond@2.0')
+    assert pkg.diamond_inheritance() == 'first_parent'
+
+    pkg = spack.repo.get('multimethod-inheritor@2.0')
+    assert pkg.diamond_inheritance() == 'first_parent'
+
+    pkg = spack.repo.get('multimethod-diamond@3.0')
+    assert pkg.diamond_inheritance() == 'second_parent'
+
+    pkg = spack.repo.get('multimethod-diamond-parent@3.0')
+    assert pkg.diamond_inheritance() == 'second_parent'
+
+    pkg = spack.repo.get('multimethod-diamond@4.0')
+    assert pkg.diamond_inheritance() == 'subclass'
diff --git a/var/spack/repos/builtin.mock/packages/multimethod-base/package.py b/var/spack/repos/builtin.mock/packages/multimethod-base/package.py
index 7b7d00b43b..def6b9ee82 100644
--- a/var/spack/repos/builtin.mock/packages/multimethod-base/package.py
+++ b/var/spack/repos/builtin.mock/packages/multimethod-base/package.py
@@ -19,3 +19,6 @@ class MultimethodBase(Package):
 
     def base_method(self):
         return "base_method"
+
+    def diamond_inheritance(self):
+        return "base_package"
diff --git a/var/spack/repos/builtin.mock/packages/multimethod-diamond-parent/package.py b/var/spack/repos/builtin.mock/packages/multimethod-diamond-parent/package.py
new file mode 100644
index 0000000000..d3413a36b5
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/multimethod-diamond-parent/package.py
@@ -0,0 +1,21 @@
+# Copyright 2013-2018 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+from spack.pkg.builtin.mock.multimethod_base import MultimethodBase
+
+
+class MultimethodDiamondParent(MultimethodBase):
+    """This package is designed for use with Spack's multimethod test.
+       It has a bunch of test cases for the @when decorator that the
+       test uses.
+    """
+
+    @when('@3.0')
+    def diamond_inheritance(self):
+        return "second_parent"
+
+    @when('@4.0, 2.0')
+    def diamond_inheritance(self):
+        return "should never be reached"
diff --git a/var/spack/repos/builtin.mock/packages/multimethod-diamond/package.py b/var/spack/repos/builtin.mock/packages/multimethod-diamond/package.py
new file mode 100644
index 0000000000..a1f2d71d22
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/multimethod-diamond/package.py
@@ -0,0 +1,18 @@
+# Copyright 2013-2018 Lawrence Livermore National Security, LLC and other
+# Spack Project Developers. See the top-level COPYRIGHT file for details.
+#
+# SPDX-License-Identifier: (Apache-2.0 OR MIT)
+
+import spack.pkg.builtin.mock.multimethod_inheritor as mi
+import spack.pkg.builtin.mock.multimethod_diamond_parent as mp
+
+
+class MultimethodDiamond(mi.MultimethodInheritor, mp.MultimethodDiamondParent):
+    """This package is designed for use with Spack's multimethod test.
+       It has a bunch of test cases for the @when decorator that the
+       test uses.
+    """
+
+    @when('@4.0')
+    def diamond_inheritance(self):
+        return 'subclass'
diff --git a/var/spack/repos/builtin.mock/packages/multimethod-inheritor/package.py b/var/spack/repos/builtin.mock/packages/multimethod-inheritor/package.py
index 066a23c6fd..d3b3cd7c44 100644
--- a/var/spack/repos/builtin.mock/packages/multimethod-inheritor/package.py
+++ b/var/spack/repos/builtin.mock/packages/multimethod-inheritor/package.py
@@ -11,3 +11,14 @@ class MultimethodInheritor(Multimethod):
        It has a bunch of test cases for the @when decorator that the
        test uses.
     """
+
+    @when('@1.0')
+    def inherited_and_overridden(self):
+        return "inheritor@1.0"
+
+    #
+    # Test multi-level inheritance
+    #
+    @when('@2:')
+    def base_method(self):
+        return 'multimethod-inheritor'
diff --git a/var/spack/repos/builtin.mock/packages/multimethod/package.py b/var/spack/repos/builtin.mock/packages/multimethod/package.py
index a8e3d4995a..738e41be41 100644
--- a/var/spack/repos/builtin.mock/packages/multimethod/package.py
+++ b/var/spack/repos/builtin.mock/packages/multimethod/package.py
@@ -124,4 +124,28 @@ class Multimethod(MultimethodBase):
     #
     @when("@2:")
     def base_method(self):
-        return "subclass_method"
+        return 'multimethod'
+
+    #
+    # Make sure methods with non-default implementations in a superclass
+    # will invoke those methods when none in the subclass match but one in
+    # the superclass does.
+    #
+    @when("@1.0")
+    def inherited_and_overridden(self):
+        return "base@1.0"
+
+    @when("@2.0")
+    def inherited_and_overridden(self):
+        return "base@2.0"
+
+    #
+    # Make sure that multimethods follow MRO properly with diamond inheritance
+    #
+    @when('@2.0')
+    def diamond_inheritance(self):
+        return 'first_parent'
+
+    @when('@4.0')
+    def diamond_inheritance(self):
+        return "should_not_be_reached"