From 2621af41d16201020b6c0965b0f6b1762b81346b Mon Sep 17 00:00:00 2001 From: Gregory Becker Date: Wed, 5 Dec 2018 12:27:01 -0800 Subject: fix MRO for multimethod.__call__ using iterative algorithm. Add tests MRO for inherited multimethods with multiple inheritance Add tests for inherited and overridden multimethods --- lib/spack/spack/multimethod.py | 29 +++++++++++----- lib/spack/spack/test/multimethod.py | 39 +++++++++++++++++++++- .../packages/multimethod-base/package.py | 3 ++ .../packages/multimethod-diamond-parent/package.py | 21 ++++++++++++ .../packages/multimethod-diamond/package.py | 18 ++++++++++ .../packages/multimethod-inheritor/package.py | 11 ++++++ .../builtin.mock/packages/multimethod/package.py | 26 ++++++++++++++- 7 files changed, 137 insertions(+), 10 deletions(-) create mode 100644 var/spack/repos/builtin.mock/packages/multimethod-diamond-parent/package.py create mode 100644 var/spack/repos/builtin.mock/packages/multimethod-diamond/package.py 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" -- cgit v1.2.3-70-g09d2