Fix/update global AddMethod

Fixes #3028

Signed-off-by: Mats Wichmann <mats@linux.com>

5 files changed, 119 insertions, 129 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index ae754ef..3a1e25c 100755
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -63,6 +63,8 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
     - Make sure cProfile is used if profiling - SCons was expecting
       the Util module to monkeypatch in cProfile as profile if available,
       but this is no longer being done.
+    - Cleanup in Util.AddMethod; detect environment instances and add
+      them using MethodWrapper to fix #3028. MethodWrapper moves to Util.
 
   From Simon Tegelid
     - Fix using TEMPFILE in multiple actions in an action list. Previously a builder, or command
diff --git a/SCons/Environment.py b/SCons/Environment.py
index 3040427..cc62679 100644
--- a/SCons/Environment.py
+++ b/SCons/Environment.py
@@ -54,6 +54,7 @@ import SCons.SConsign
 import SCons.Subst
 import SCons.Tool
 import SCons.Util
+from SCons.Util import MethodWrapper
 import SCons.Warnings
 
 class _Null:
@@ -180,48 +181,17 @@ def _delete_duplicates(l, keep_last):
 # Shannon at the following page (there called the "transplant" class):
 #
 # ASPN : Python Cookbook : Dynamically added methods to a class
-# http://aspn.activestate.com/ASPN/Cookbook/Python/Recipe/81732
+# https://code.activestate.com/recipes/81732/
 #
 # We had independently been using the idiom as BuilderWrapper, but
 # factoring out the common parts into this base class, and making
 # BuilderWrapper a subclass that overrides __call__() to enforce specific
 # Builder calling conventions, simplified some of our higher-layer code.
+#
+# Note: MethodWrapper moved to SCons.Util as it was needed there
+# and otherwise we had a circular import problem.
 
-class MethodWrapper:
-    """
-    A generic Wrapper class that associates a method (which can
-    actually be any callable) with an object.  As part of creating this
-    MethodWrapper object an attribute with the specified (by default,
-    the name of the supplied method) is added to the underlying object.
-    When that new "method" is called, our __call__() method adds the
-    object as the first argument, simulating the Python behavior of
-    supplying "self" on method calls.
-
-    We hang on to the name by which the method was added to the underlying
-    base class so that we can provide a method to "clone" ourselves onto
-    a new underlying object being copied (without which we wouldn't need
-    to save that info).
-    """
-    def __init__(self, object, method, name=None):
-        if name is None:
-            name = method.__name__
-        self.object = object
-        self.method = method
-        self.name = name
-        setattr(self.object, name, self)
-
-    def __call__(self, *args, **kwargs):
-        nargs = (self.object,) + args
-        return self.method(*nargs, **kwargs)
-
-    def clone(self, new_object):
-        """
-        Returns an object that re-binds the underlying "method" to
-        the specified new object.
-        """
-        return self.__class__(new_object, self.method, self.name)
-
-class BuilderWrapper(MethodWrapper):
+class BuilderWrapper(SCons.Util.MethodWrapper):
     """
     A MethodWrapper subclass that that associates an environment with
     a Builder.
@@ -248,7 +218,7 @@ class BuilderWrapper(MethodWrapper):
             target = [target]
         if source is not None and not SCons.Util.is_List(source):
             source = [source]
-        return MethodWrapper.__call__(self, target, source, *args, **kw)
+        return super().__call__(target, source, *args, **kw)
 
     def __repr__(self):
         return '<BuilderWrapper %s>' % repr(self.name)
@@ -2377,7 +2347,7 @@ class OverrideEnvironment(Base):
         # Environment they are being constructed with and so will not
         # have access to overrided values. So we rebuild them with the
         # OverrideEnvironment so they have access to overrided values.
-        if isinstance(attr, (MethodWrapper, BuilderWrapper)):
+        if isinstance(attr, MethodWrapper):
             return attr.clone(self)
         else:
             return attr
diff --git a/SCons/Environment.xml b/SCons/Environment.xml
index bb9b90e..a1fd8ec 100644
--- a/SCons/Environment.xml
+++ b/SCons/Environment.xml
@@ -286,31 +286,22 @@ until the Action object is actually used.
 </arguments>
 <summary>
 <para>
-When called with the
-<function>AddMethod</function>()
-form,
-adds the specified
-<parameter>function</parameter>
-to the specified
-<parameter>object</parameter>
-as the specified method
-<parameter>name</parameter>.
-When called using the
-&f-env-AddMethod; form,
-adds the specified
-<parameter>function</parameter>
-to the construction environment
-<replaceable>env</replaceable>
-as the specified method
-<parameter>name</parameter>.
-In both cases, if
-<parameter>name</parameter>
-is omitted or
-<constant>None</constant>,
-the name of the
-specified
-<parameter>function</parameter>
-itself is used for the method name.
+Adds <parameter>function</parameter> to an object as a method.
+<parameter>function</parameter> will be called with an instance
+object as the first argument as for other methods.
+If <parameter>name</parameter> is given, it is used as
+the name of the new method, else the name of
+<parameter>function</parameter> is used.
+</para>
+<para>
+When the global function &f-AddMethod; is called,
+the object to add the method to must be passed as the first argument;
+typically this will be &Environment;,
+in order to create a method which applies to all &consenvs;
+subsequently constructed.
+When called using the &f-env-AddMethod; form,
+the method is added to the specified &consenv; only.
+Added methods propagate through &f-env-Clone; calls.
 </para>
 
 <para>
@@ -318,22 +309,17 @@ Examples:
 </para>
 
 <example_commands>
-# Note that the first argument to the function to
-# be attached as a method must be the object through
-# which the method will be called; the Python
-# convention is to call it 'self'.
+# Function to add must accept an instance argument.
+# The Python convention is to call this 'self'.
 def my_method(self, arg):
     print("my_method() got", arg)
 
-# Use the global AddMethod() function to add a method
-# to the Environment class.  This
+# Use the global function to add a method to the Environment class:
 AddMethod(Environment, my_method)
 env = Environment()
 env.my_method('arg')
 
-# Add the function as a method, using the function
-# name for the method call.
-env = Environment()
+# Use the optional name argument to set the name of the method:
 env.AddMethod(my_method, 'other_method_name')
 env.other_method_name('another arg')
 </example_commands>
diff --git a/SCons/EnvironmentTests.py b/SCons/EnvironmentTests.py
index 8d2704d..e308865 100644
--- a/SCons/EnvironmentTests.py
+++ b/SCons/EnvironmentTests.py
@@ -721,7 +721,7 @@ sys.exit(0)
         r = env4.func2()
         assert r == 'func2-4', r
 
-        # Test that clones don't re-bind an attribute that the user
+        # Test that clones don't re-bind an attribute that the user set.
         env1 = Environment(FOO = '1')
         env1.AddMethod(func2)
         def replace_func2():
@@ -731,6 +731,18 @@ sys.exit(0)
         r = env2.func2()
         assert r == 'replace_func2', r
 
+        # Test clone rebinding if using global AddMethod.
+        env1 = Environment(FOO='1')
+        SCons.Util.AddMethod(env1, func2)
+        r = env1.func2()
+        assert r == 'func2-1', r
+        r = env1.func2('-xxx')
+        assert r == 'func2-1-xxx', r
+        env2 = env1.Clone(FOO='2')
+        r = env2.func2()
+        assert r == 'func2-2', r
+
+
     def test_Override(self):
         """Test overriding construction variables"""
         env = SubstitutionEnvironment(ONE=1, TWO=2, THREE=3, FOUR=4)
diff --git a/SCons/Util.py b/SCons/Util.py
index 347395f..88aeaae 100644
--- a/SCons/Util.py
+++ b/SCons/Util.py
@@ -27,23 +27,19 @@ import os
 import sys
 import copy
 import re
-import types
 import pprint
 import hashlib
 from collections import UserDict, UserList, UserString, OrderedDict
 from collections.abc import MappingView
+from types import MethodType, FunctionType
 
 PYPY = hasattr(sys, 'pypy_translation_info')
 
-# Below not used?
-# InstanceType    = types.InstanceType
-
-MethodType      = types.MethodType
-FunctionType    = types.FunctionType
-
-def dictify(keys, values, result={}):
-    for k, v in zip(keys, values):
-        result[k] = v
+# unused?
+def dictify(keys, values, result=None):
+    if result is None:
+        result = {}
+    result.update(dict(zip(keys, values)))
     return result
 
 _altsep = os.altsep
@@ -633,6 +629,41 @@ class Delegate:
         else:
             return self
 
+
+class MethodWrapper:
+    """A generic Wrapper class that associates a method with an object.
+
+    As part of creating this MethodWrapper object an attribute with the
+    specified name (by default, the name of the supplied method) is added
+    to the underlying object.  When that new "method" is called, our
+    __call__() method adds the object as the first argument, simulating
+    the Python behavior of supplying "self" on method calls.
+
+    We hang on to the name by which the method was added to the underlying
+    base class so that we can provide a method to "clone" ourselves onto
+    a new underlying object being copied (without which we wouldn't need
+    to save that info).
+    """
+    def __init__(self, object, method, name=None):
+        if name is None:
+            name = method.__name__
+        self.object = object
+        self.method = method
+        self.name = name
+        setattr(self.object, name, self)
+
+    def __call__(self, *args, **kwargs):
+        nargs = (self.object,) + args
+        return self.method(*nargs, **kwargs)
+
+    def clone(self, new_object):
+        """
+        Returns an object that re-binds the underlying "method" to
+        the specified new object.
+        """
+        return self.__class__(new_object, self.method, self.name)
+
+
 # attempt to load the windows registry module:
 can_read_reg = 0
 try:
@@ -1096,7 +1127,7 @@ def adjustixes(fname, pre, suf, ensure_suffix=False):
 
 
 # From Tim Peters,
-# http://aspn.activestate.com/ASPN/Cookbook/Python/Recipe/52560
+# https://code.activestate.com/recipes/52560
 # ASPN: Python Cookbook: Remove duplicates from a sequence
 # (Also in the printed Python Cookbook.)
 
@@ -1170,9 +1201,8 @@ def unique(s):
     return u
 
 
-
 # From Alex Martelli,
-# http://aspn.activestate.com/ASPN/Cookbook/Python/Recipe/52560
+# https://code.activestate.com/recipes/52560
 # ASPN: Python Cookbook: Remove duplicates from a sequence
 # First comment, dated 2001/10/13.
 # (Also in the printed Python Cookbook.)
@@ -1375,42 +1405,41 @@ def make_path_relative(path):
     return path
 
 
-
-# The original idea for AddMethod() and RenameFunction() come from the
+# The original idea for AddMethod() came from the
 # following post to the ActiveState Python Cookbook:
 #
-#   ASPN: Python Cookbook : Install bound methods in an instance
-#   http://aspn.activestate.com/ASPN/Cookbook/Python/Recipe/223613
-#
-# That code was a little fragile, though, so the following changes
-# have been wrung on it:
+# ASPN: Python Cookbook : Install bound methods in an instance
+# https://code.activestate.com/recipes/223613
 #
+# Changed as follows:
 # * Switched the installmethod() "object" and "function" arguments,
 #   so the order reflects that the left-hand side is the thing being
 #   "assigned to" and the right-hand side is the value being assigned.
-#
-# * Changed explicit type-checking to the "try: klass = object.__class__"
-#   block in installmethod() below so that it still works with the
-#   old-style classes that SCons uses.
-#
-# * Replaced the by-hand creation of methods and functions with use of
-#   the "new" module, as alluded to in Alex Martelli's response to the
-#   following Cookbook post:
-#
-#   ASPN: Python Cookbook : Dynamically added methods to a class
-#   http://aspn.activestate.com/ASPN/Cookbook/Python/Recipe/81732
+# * The instance/class detection is changed a bit, as it's all
+#   new-style classes now with Py3.
+# * The by-hand construction of the function object from renamefunction()
+#   is not needed, the remaining bit is now used inline in AddMethod.
 
 def AddMethod(obj, function, name=None):
-    """
-    Adds either a bound method to an instance or the function itself (or an unbound method in Python 2) to a class.
-    If name is ommited the name of the specified function
-    is used by default.
+    """Adds a method to an object.
+
+    Adds `function` to `obj` if `obj` is a class object.
+    Adds `function` as a bound method if `obj` is an instance object.
+    If `obj` looks like an environment instance, use `MethodWrapper`
+    to add it.  If `name` is supplied it is used as the name of `function`.
+
+    Although this works for any class object, the intent as a public
+    API is to be used on Environment, to be able to add a method to all
+    construction environments; it is preferred to use env.AddMethod
+    to add to an individual environment.
 
     Example::
 
+        class A:
+            ...
         a = A()
         def f(self, x, y):
-        self.z = x + y
+            self.z = x + y
         AddMethod(f, A, "add")
         a.add(2, 4)
         print(a.z)
@@ -1420,32 +1449,24 @@ def AddMethod(obj, function, name=None):
     if name is None:
         name = function.__name__
     else:
-        function = RenameFunction(function, name)
+        # "rename"
+        function = FunctionType(
+            function.__code__, function.__globals__, name, function.__defaults__
+        )
 
-    # Note the Python version checks - WLB
-    # Python 3.3 dropped the 3rd parameter from types.MethodType
     if hasattr(obj, '__class__') and obj.__class__ is not type:
-        # "obj" is an instance, so it gets a bound method.
-        if sys.version_info[:2] > (3, 2):
-            method = MethodType(function, obj)
+        # obj is an instance, so it gets a bound method.
+        if hasattr(obj, "added_methods"):
+            method = MethodWrapper(obj, function, name)
+            obj.added_methods.append(method)
         else:
-            method = MethodType(function, obj, obj.__class__)
+            method = MethodType(function, obj)
     else:
-        # Handle classes
+        # obj is a class
         method = function
 
     setattr(obj, name, method)
 
-def RenameFunction(function, name):
-    """
-    Returns a function identical to the specified function, but with
-    the specified name.
-    """
-    return FunctionType(function.__code__,
-                        function.__globals__,
-                        name,
-                        function.__defaults__)
-
 
 if hasattr(hashlib, 'md5'):
     md5 = True
@@ -1523,8 +1544,7 @@ def silent_intern(x):
 
 # From Dinu C. Gherman,
 # Python Cookbook, second edition, recipe 6.17, p. 277.
-# Also:
-# http://aspn.activestate.com/ASPN/Cookbook/Python/Recipe/68205
+# Also: https://code.activestate.com/recipes/68205
 # ASPN: Python Cookbook: Null Object Design Pattern
 
 class Null: