Merge lp:~spiv/bzr/pretty-decorator-vs-string-identity-718569 into lp:bzr

[Andrew Bennetts]

Preserve identity of default values in the pretty decorators. (#718569)

This fixes bug 718569. Instead of relying on the repr() of default parameter values preserving the semantics of those values (which isn't the case of at least symbol_versioning.DEPRECATED_PARAMETER), it runs the exec statement with a scope that provides the original objects, and uses those instead.

All the existing tests for prettiness of the "pretty" decorators pass, so presumably this doesn't uglify them too much :) Correctness trumps prettiness anyway. I added new tests to catch this problem.

[Vincent Ladeuil (vila)]

Sounds reasonable and tested. A bit scary in retrospect, nice catch !

review: Approve

[Andrew Bennetts (spiv)]

sent to pqm by email

=== modified file 'bzrlib/decorators.py'
--- bzrlib/decorators.py	2010-02-17 17:11:16 +0000
+++ bzrlib/decorators.py	2011-02-14 12:09:20 +0000
@@ -30,14 +30,16 @@
 def _get_parameters(func):
     """Recreate the parameters for a function using introspection.
 
-    :return: (function_params, calling_params)
+    :return: (function_params, calling_params, default_values)
         function_params: is a string representing the parameters of the
             function. (such as "a, b, c=None, d=1")
             This is used in the function declaration.
         calling_params: is another string representing how you would call the
             function with the correct parameters. (such as "a, b, c=c, d=d")
-            Assuming you sued function_params in the function declaration, this
+            Assuming you used function_params in the function declaration, this
             is the parameters to put in the function call.
+        default_values_block: a dict with the default values to be passed as
+            the scope for the 'exec' statement.
 
         For example:
 
@@ -49,9 +51,15 @@
     # it globally.
     import inspect
     args, varargs, varkw, defaults = inspect.getargspec(func)
+    defaults_dict = {}
+    def formatvalue(value):
+        default_name = '__default_%d' % len(defaults_dict)
+        defaults_dict[default_name] = value
+        return '=' + default_name
     formatted = inspect.formatargspec(args, varargs=varargs,
                                       varkw=varkw,
-                                      defaults=defaults)
+                                      defaults=defaults,
+                                      formatvalue=formatvalue)
     if defaults is None:
         args_passed = args
     else:
@@ -65,7 +73,7 @@
         args_passed.append('**' + varkw)
     args_passed = ', '.join(args_passed)
 
-    return formatted[1:-1], args_passed
+    return formatted[1:-1], args_passed, defaults_dict
 
 
 def _pretty_needs_read_lock(unbound):
@@ -107,14 +115,17 @@
         return result
 read_locked = %(name)s_read_locked
 """
-    params, passed_params = _get_parameters(unbound)
+    params, passed_params, defaults_dict = _get_parameters(unbound)
     variables = {'name':unbound.__name__,
                  'params':params,
                  'passed_params':passed_params,
                 }
     func_def = template % variables
 
-    exec func_def in locals()
+    scope = dict(defaults_dict)
+    scope['unbound'] = unbound
+    exec func_def in scope
+    read_locked = scope['read_locked']
 
     read_locked.__doc__ = unbound.__doc__
     read_locked.__name__ = unbound.__name__
@@ -172,14 +183,17 @@
         return result
 write_locked = %(name)s_write_locked
 """
-    params, passed_params = _get_parameters(unbound)
+    params, passed_params, defaults_dict = _get_parameters(unbound)
     variables = {'name':unbound.__name__,
                  'params':params,
                  'passed_params':passed_params,
                 }
     func_def = template % variables
 
-    exec func_def in locals()
+    scope = dict(defaults_dict)
+    scope['unbound'] = unbound
+    exec func_def in scope
+    write_locked = scope['write_locked']
 
     write_locked.__doc__ = unbound.__doc__
     write_locked.__name__ = unbound.__name__

=== modified file 'bzrlib/tests/test_decorators.py'
--- bzrlib/tests/test_decorators.py	2010-04-23 08:51:52 +0000
+++ bzrlib/tests/test_decorators.py	2011-02-14 12:09:20 +0000
@@ -27,11 +27,13 @@
     pass
 
 
-def create_decorator_sample(style, unlock_error=None):
+def create_decorator_sample(style, unlock_error=None, meth=None):
     """Create a DecoratorSample object, using specific lock operators.
 
     :param style: The type of lock decorators to use (fast/pretty/None)
     :param unlock_error: If specified, an error to raise from unlock.
+    :param meth: a function to be decorated and added as a 'meth_read' and
+        'meth_write' to the object.
     :return: An instantiated DecoratorSample object.
     """
 
@@ -92,6 +94,10 @@
             self.actions.append('fail_during_write')
             raise TypeError('during write')
 
+        if meth is not None:
+            meth_read = needs_read_lock(meth)
+            meth_write = needs_write_lock(meth)
+
     return DecoratorSample()
 
 
@@ -149,6 +155,20 @@
         self.assertEqual(['lock_write', ('bank', 'bar', 'bing'),
                           'unlock_fail'], sam.actions)
 
+    def test_read_lock_preserves_default_str_kwarg_identity(self):
+        a_constant = 'A str used as a constant'
+        def meth(self, param=a_constant):
+            return param
+        sam = create_decorator_sample(self._decorator_style, meth=meth)
+        self.assertIs(a_constant, sam.meth_read())
+
+    def test_write_lock_preserves_default_str_kwarg_identity(self):
+        a_constant = 'A str used as a constant'
+        def meth(self, param=a_constant):
+            return param
+        sam = create_decorator_sample(self._decorator_style, meth=meth)
+        self.assertIs(a_constant, sam.meth_write())
+
 
 class TestFastDecoratorActions(TestDecoratorActions):
 

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-02-11 17:12:35 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-02-14 12:09:20 +0000
@@ -90,6 +90,10 @@
   ``bzr plugins`` and in crash reports.
   (#704195, Martin Pool)
 
+* The "pretty" version of ``needs_read_lock`` and ``needs_write_lock`` now
+  preserves the identity of default parameter values.
+  (Andrew Bennetts, #718569)
+
 * ``bzr dump-btree --raw`` no longer tracebacks on a B-Tree file
   containing no rows. (Eric Siegerman, #715508)