Decorator order implemented backwards?
I haven't seen anyone remark upon this yet, but the order of decorator application appears to be implemented backwards. According to the PEP:
Proposed Syntax
The current syntax for function decorators as implemented in Python 2.4a2 is: @dec2 @dec1 def func(arg1, arg2, ...): pass
This is equivalent to: def func(arg1, arg2, ...): pass func = dec2(dec1(func))
However, it seems the '@' form is really doing dec1(dec2(func)), not dec2(dec1(func)). The test_decorators.py reveals that it is, indeed, testing *for* the wrong behavior. test_decorators.py:
def test_order(self): class C(object): @funcattrs(abc=1) @staticmethod def foo(): return 42 # This wouldn't work if staticmethod was called first self.assertEqual(C.foo(), 42) self.assertEqual(C().foo(), 42)
So, um...I assume this is a bug in the implementation, not the PEP, because the PEP's way makes sense and the way it's implemented doesn't. James
James Y Knight wrote:
I haven't seen anyone remark upon this yet, but the order of decorator application appears to be implemented backwards.
def test_order(self): class C(object): @funcattrs(abc=1) @staticmethod def foo(): return 42 # This wouldn't work if staticmethod was called first self.assertEqual(C.foo(), 42) self.assertEqual(C().foo(), 42)
How odd. I'm positive at one point I tested this and it was the right way round.
def dec1(x): ... print "dec1" ... return x ... def dec2(x): ... print "dec2" ... return x ...
@dec1 ... @dec2 ... def foo(): pass ... dec1 dec2
This is definitely wrong. Wrong wrong wrong. Anthony
Anthony Baxter wrote:
James Y Knight wrote:
I haven't seen anyone remark upon this yet, but the order of decorator application appears to be implemented backwards.
def test_order(self): class C(object): @funcattrs(abc=1) @staticmethod def foo(): return 42 # This wouldn't work if staticmethod was called first self.assertEqual(C.foo(), 42) self.assertEqual(C().foo(), 42)
How odd. I'm positive at one point I tested this and it was the right way round.
Mark, is it possible that this changed between the first and final versions of the decorator patch? The SF report doesn't list any of the older versions... Thanks, Anthony
On Tue, 2004-08-10 at 06:08, Anthony Baxter wrote:
Mark, is it possible that this changed between the first and final versions of the decorator patch? The SF report doesn't list any of the older versions...
No, it has always been this way round. In fact test_order was inherited from Guido's original version of test_decorators.py (from patch #926860) where it read: def test_order(self): class C(object): [funcattrs(abc=1), staticmethod] def foo(): return 42 # This wouldn't work if staticmethod was called first self.assertEqual(C.foo(), 42) self.assertEqual(C().foo(), 42) (i.e. identical to the current version except for the change in syntax). In fact I relied on the fact that this test passed to convince me I had the order right! But I should have spotted the inconsistency between that and the documentation that I wrote for the reference manual. I'll do a patch to fix the order and the corresponding tests. While I'm at it, do we want to drop support for multiple decorators on a single line? Nobody has spoken up for it, and in fact forcing one-per-line would simplify the grammar (as well as making it easier for toy parsers to find decorators). Mark
On Aug 10, 2004, at 5:07 PM, Mark Russell wrote:
While I'm at it, do we want to drop support for multiple decorators on a single line? Nobody has spoken up for it, and in fact forcing one-per-line would simplify the grammar (as well as making it easier for toy parsers to find decorators).
+1 -bob
On Tue, 2004-08-10 at 17:24, Bob Ippolito wrote:
On Aug 10, 2004, at 5:07 PM, Mark Russell wrote:
While I'm at it, do we want to drop support for multiple decorators on a single line? Nobody has spoken up for it, and in fact forcing one-per-line would simplify the grammar (as well as making it easier for toy parsers to find decorators).
+1
+1 -Barry
Barry Warsaw wrote:
On Tue, 2004-08-10 at 17:24, Bob Ippolito wrote:
On Aug 10, 2004, at 5:07 PM, Mark Russell wrote:
While I'm at it, do we want to drop support for multiple decorators on a single line? Nobody has spoken up for it, and in fact forcing one-per-line would simplify the grammar (as well as making it easier for toy parsers to find decorators).
+1
+1
+1 = +3 from me! =) trying-to-get-through-over-1,000-emails-for-the-next-summary-yr's, Brett
No, it has always been this way round. In fact test_order was inherited from Guido's original version of test_decorators.py (from patch #926860) where it read:
def test_order(self): class C(object): [funcattrs(abc=1), staticmethod] def foo(): return 42 # This wouldn't work if staticmethod was called first self.assertEqual(C.foo(), 42) self.assertEqual(C().foo(), 42)
(i.e. identical to the current version except for the change in syntax). In fact I relied on the fact that this test passed to convince me I had the order right! But I should have spotted the inconsistency between that and the documentation that I wrote for the reference manual.
Oops. When using a list, it makes sense to apply the decorators left-to-right. But when using @deco, I think it makes more sense that the decorators closer to the def are applied first. IOW the syntax is conceptually right-recursive: definition ::= '@' decorator definition | 'def' ...
I'll do a patch to fix the order and the corresponding tests.
Great.
While I'm at it, do we want to drop support for multiple decorators on a single line? Nobody has spoken up for it, and in fact forcing one-per-line would simplify the grammar (as well as making it easier for toy parsers to find decorators).
+1 --Guido van Rossum (home page: http://www.python.org/~guido/)
Mark Russell wrote: [SNIP - stuff about the order]
I'll do a patch to fix the order and the corresponding tests.
I hope I am not stepping on Mark's toes but I went ahead and fixed it to be bottom-up like the PEP. Applied as rev. 2.318 for Python/compile.c and rev. 1.4 for Lib/test/test_decorators.py (rewrote the order test to not rely on causing an error to signify an error to but actually lead to a failure). I did not make any changes to to force decorators on to a single line, though. Mark can still have that one. =) -Brett
On Sun, 2004-08-15 at 08:22, Brett C. wrote:
Mark Russell wrote:
[SNIP - stuff about the order]
I'll do a patch to fix the order and the corresponding tests.
I hope I am not stepping on Mark's toes but I went ahead and fixed it to be bottom-up like the PEP.
Now that's what I call timing :-) I finished the patch to fix this yesterday and decided to leave it until today to upload (I've been swamped this week, hence the delay getting time to do this). I've uploaded it now (as patch #1009560) because I think it is a more correct fix (as well as also fixing the Lib/compiler package). It turns out that the order of evaluation of issue is more subtle than it first appears. Here's the comment from the start of the new test_eval_order() in test_decorators.py: # Evaluating a decorated function involves four steps for each # decorator-maker (the function that returns a decorator): # # 1: Evaluate the decorator-maker name # 2: Evaluate the decorator-maker arguments (if any) # 3: Call the decorator-maker to make a decorator # 4: Call the decorator # # When there are multiple decorators, these steps should be # performed in the above order for each decorator, but we should # iterate through the decorators in the reverse of the order # they appear in the source. Your patch results in the evaluation order: evalname1 evalargs1 makedec1 evalname2 evalargs2 makedec2 evalname3 evalargs3 makedec3 calldec3 calldec2 calldec1 Mine (#1009560) gives: evalname3 evalargs3 makedec3 calldec3 evalname2 evalargs2 makedec2 calldec2 evalname1 evalargs1 makedec1 calldec1 I would defend this as the correct order because it is the same as the order you get with the current method (func = staticmethod(func)).
I did not make any changes to to force decorators on to a single line, though. Mark can still have that one. =)
Thanks :-) My patch implements that change as well (plus doc and test updates to match). Mark
In article <1092583647.1670.18.camel@localhost>, Mark Russell <marktrussell@btopenworld.com> wrote: [Brett C's]
patch results in the evaluation order:
evalname1 evalargs1 makedec1 evalname2 evalargs2 makedec2 evalname3 evalargs3 makedec3 calldec3 calldec2 calldec1
Mine (#1009560) gives:
evalname3 evalargs3 makedec3 calldec3 evalname2 evalargs2 makedec2 calldec2 evalname1 evalargs1 makedec1 calldec1
It would probably be bad style to have any order dependencies in the evalname evalargs makedec parts, but Brett's order makes more sense to me. That is, I like the actual application of decorators to be in nested order, but it makes more sense to me for the lines constructing the decorators to be evaluated in the order they appear in the code. -- David Eppstein Computer Science Dept., Univ. of California, Irvine http://www.ics.uci.edu/~eppstein/
On Sun, 2004-08-15 at 18:11, David Eppstein wrote:
It would probably be bad style to have any order dependencies in the evalname evalargs makedec parts, but Brett's order makes more sense to me. That is, I like the actual application of decorators to be in nested order, but it makes more sense to me for the lines constructing the decorators to be evaluated in the order they appear in the code.
On second thoughts I think you are right. I was concerned about the difficulty of explaining the two different evaluation orders, but that is outweighed by the non-intuitiveness of having decorator expressions evaluated in the opposite of source code order. I've updated the patch. Here's my current attempt at describing the semantics in the reference manual: A function definition may be wrapped by one or more decorator expressions. Decorator expressions are evaluated when the function is defined, in the scope that contains the function definition. The result must be a callable, which is invoked with the function object as the only argument. The returned value is bound to the function name instead of the function object. Multiple decorators are applied in nested fashion. For example, the following code: @f1(arg) @f2 def func(): pass is equivalent to: def func(): pass func = f1(arg)(f2(func)) Mark
On Sun, Aug 15, 2004 at 08:46:03PM +0100, Mark Russell wrote:
Here's my current attempt at describing the semantics in the reference manual:
A function definition may be wrapped by one or more decorator expressions. Decorator expressions are evaluated when the function is defined, in the scope that contains the function definition. The result must be a callable, which is invoked with the function object as the only argument. The returned value is bound to the function name instead of the function object. Multiple decorators are applied in nested fashion. For example, the following code:
@f1(arg) @f2 def func(): pass
is equivalent to:
def func(): pass func = f1(arg)(f2(func))
Is func actually defined when f1 and f2 are called? Looking at the code the VAR_STORE doesn't happen until after the decorators are added. [but I'm no expert on bytecode]. This won't matter for sane decorators but we're talking about the strange cases already. Reading the @ code top to bottom you would not expect func to be defined, but if you want it to be strictly like the 'func = decorate(func)' translation is should be. My patch to add support for class decorators defines func before the decorators as a side effect of moving Mark's decorator code out of com_funcdef (and yes, that was a plug). -Jack
On Aug 15, 2004, at 11:45 PM, Jack Diederich wrote:
My patch to add support for class decorators defines func before the decorators as a side effect of moving Mark's decorator code out of com_funcdef (and yes, that was a plug).
That's definitely worse. It causes a possibly incorrect temporary value to be bound, even if only for a short amount of time. The current behavior of only doing the store after executing the decorators is cleaner. I'd rewrite the translation as: """ The following code: @f1(arg) @f2 def func(): return True is nearly* equivalent to: func = f1(arg)(f2(lambda : True)) * However, you don't have the syntactical restrictions of a lambda, and the function name gets set to "func" instead of "<lambda>". """ James
On Mon, Aug 16, 2004 at 12:38:06AM -0400, James Y Knight wrote:
On Aug 15, 2004, at 11:45 PM, Jack Diederich wrote:
My patch to add support for class decorators defines func before the decorators as a side effect of moving Mark's decorator code out of com_funcdef (and yes, that was a plug).
That's definitely worse. It causes a possibly incorrect temporary value to be bound, even if only for a short amount of time. The current behavior of only doing the store after executing the decorators is cleaner.
I agree it would be unexpected if a function defined two lines down happens before the @mutate above it.
I'd rewrite the translation as: """ The following code: @f1(arg) @f2 def func(): return True
is nearly* equivalent to: func = f1(arg)(f2(lambda : True))
* However, you don't have the syntactical restrictions of a lambda, and the function name gets set to "func" instead of "<lambda>". """
Yes, the docs just need some kind of asterisk about the evaluation order. I was just pointing out a minor documentation bug. Mainly I just wanted to say "class decorators please" and to say I've submitted and would be happy to submit more patches to make it happen (for '@' or anything else). I currently write: class FooOverTimeReport(object): __metaclass__ = RunDaily # works OK unless you need more than one ... when I mean @run_daily class FooOverTimeReport(object): ... -Jack
At 12:38 AM 8/16/04 -0400, James Y Knight wrote:
On Aug 15, 2004, at 11:45 PM, Jack Diederich wrote:
My patch to add support for class decorators defines func before the decorators as a side effect of moving Mark's decorator code out of com_funcdef (and yes, that was a plug).
That's definitely worse. It causes a possibly incorrect temporary value to be bound, even if only for a short amount of time. The current behavior of only doing the store after executing the decorators is cleaner.
Not only that, it's the behavior *required* by the PEP. Note that it states "*without* the intermediate assignment to the variable func". (Emphasis added.) This choice of semantics is intentional, to allow for things like Ed Loper's property_getter/property_setter and generic functions to be possible. That is, it's intended to allow for decorators that construct an object from multiple function definitions. Such decorators need to be able to get the previous object bound to that name, and that's not possible if the new function has been saved under that name. There should probably be some kind of test added to the test suite to verify this behavior, and adding an explanation to the PEP is probably in order as well.
Mark Russell wrote:
On Sun, 2004-08-15 at 08:22, Brett C. wrote:
Mark Russell wrote:
[SNIP - stuff about the order]
I'll do a patch to fix the order and the corresponding tests.
I hope I am not stepping on Mark's toes but I went ahead and fixed it to be bottom-up like the PEP.
Now that's what I call timing :-) I finished the patch to fix this yesterday and decided to leave it until today to upload (I've been swamped this week, hence the delay getting time to do this). I've uploaded it now (as patch #1009560) because I think it is a more correct fix (as well as also fixing the Lib/compiler package).
=) I kept doing cvs update, just waiting for someone to fix this right in the middle of me doing all of this.
It turns out that the order of evaluation of issue is more subtle than it first appears. Here's the comment from the start of the new test_eval_order() in test_decorators.py:
# Evaluating a decorated function involves four steps for each # decorator-maker (the function that returns a decorator): # # 1: Evaluate the decorator-maker name # 2: Evaluate the decorator-maker arguments (if any) # 3: Call the decorator-maker to make a decorator # 4: Call the decorator # # When there are multiple decorators, these steps should be # performed in the above order for each decorator, but we should # iterate through the decorators in the reverse of the order # they appear in the source.
Your patch results in the evaluation order:
evalname1 evalargs1 makedec1 evalname2 evalargs2 makedec2 evalname3 evalargs3 makedec3 calldec3 calldec2 calldec1
Mine (#1009560) gives:
evalname3 evalargs3 makedec3 calldec3 evalname2 evalargs2 makedec2 calldec2 evalname1 evalargs1 makedec1 calldec1
I would defend this as the correct order because it is the same as the order you get with the current method (func = staticmethod(func)).
I am leaving this up to Guido (seems to have become the default PEP 318 champion) to make the decision on the order. Personally I like my order more since I would expect evaluation of arguments to be in the order listed, just the application order to be reversed. Either way I won't be able to deal with this patch if it is accepted; with the python-dev Summary coverage period ending today I still have 728 emails to summarize on top of any sent in today so I will be swamped with that for the rest of the week. I am sure someone else can step up and applying it if Guido okays the application order. -Brett
728 emails to summarize on top of any sent in today so I will be swamped with that for the rest of the week. I am sure someone else can step up and applying it if Guido okays the application order.
Since Mark ended up agreeing with your order, the OK is given by default. :-) --Guido van Rossum (home page: http://www.python.org/~guido/)
participants (10)
-
Anthony Baxter -
Barry Warsaw -
Bob Ippolito -
Brett C. -
David Eppstein -
Guido van Rossum -
Jack Diederich -
James Y Knight -
Mark Russell -
Phillip J. Eby