bpo-33211: Change line number of decorated nodes back to def #6460

emmatyping · 2018-04-12T22:33:03Z

This change moves the patching of decorated defs from the publicly used AST to the private bytecode.
This has the advantage that decorated defs in the ast keep their line numbers correctly, but inspect.getsource should still work.

https://bugs.python.org/issue33211

ilevkivskyi

Few comments. Also watch out, this PR will get merge conflicts pretty often.

ilevkivskyi · 2018-04-22T10:09:46Z

Python/ast.c

-        thing->col_offset = n->n_col_offset;
+        return ast_for_async_funcdef(c, CHILD(n, 1), decorator_seq);
+    } else {
+        return NULL; // should never ever happen


Maybe then assert 0?

Py_UNREACHABLE()

This is still not fixed.

ilevkivskyi · 2018-04-22T10:11:47Z

Python/compile.c

+    // fixup decorators to be the first line number internally.
+    if (asdl_seq_LEN(decos) > 0) {
+        expr_ty first_decorator = asdl_seq_GET(decos, 0);
+        c->u->u_firstlineno = first_decorator->lineno;


Why this is needed? It would be a bit awkward if we externally show everywhere line of def as a first line while internally always use the decorator line.

To clarify, I think it may be OK if you give a good comment with an explanation.

We need to maintain the first line number of the code object to be the line number of the first decorator so that inspect.getsource will work correctly. The other alternative would be to change this, so that code objects point to both the line number of the def and the first source line number, but I doubt that would be useful, or worth any additional overhead that would cause. I will add a more detailed comment.

serhiy-storchaka

What about classes? Does they need correcting the first line number?

Please add test for AST line numbers.

serhiy-storchaka · 2018-04-22T21:13:10Z

Python/ast.c

-        thing->col_offset = n->n_col_offset;
+        return ast_for_async_funcdef(c, CHILD(n, 1), decorator_seq);
+    } else {
+        return NULL; // should never ever happen


Py_UNREACHABLE()

serhiy-storchaka · 2018-04-22T21:29:39Z

Lib/test/test_dis.py

@@ -65,7 +65,7 @@ def cm(cls, x):
              8 STORE_ATTR               0 (x)
             10 LOAD_CONST               0 (None)
             12 RETURN_VALUE
-""" % (_C.cm.__code__.co_firstlineno + 2,)
+""" % (_C.cm.__code__.co_firstlineno + 1,)


Why this is changed? I thought this PR shouldn't change __code__.co_firstlineno

Thank you for pointing this out. I realized that my changes weren't actually updating the first line number in the compiler! I've fixed that.

emmatyping · 2018-04-22T21:46:58Z

@serhiy-storchaka

What about classes? Does they need correcting the first line number?

Yes, I just added the same change that happens in compiler_function to compiler_class, which should be all that is needed.

Please add test for AST line numbers.

I am planning on doing so. I want things to pass existing tests in CI first however.

ilevkivskyi · 2018-04-23T23:39:36Z

Python/ast.c

-        thing->col_offset = n->n_col_offset;
+        return ast_for_async_funcdef(c, CHILD(n, 1), decorator_seq);
+    } else {
+        return NULL; // should never ever happen


This is still not fixed.

emmatyping · 2018-04-23T23:56:24Z

Okay, looks like test_dis and test_inspect passed, so I will get to writing new tests for decorated nodes in test_ast.

Done! Ready for review.

ilevkivskyi

This is almost ready.

ilevkivskyi · 2018-04-24T08:00:00Z

Python/compile.c

@@ -1949,6 +1949,26 @@ compiler_default_arguments(struct compiler *c, arguments_ty args)
    return funcflags;
 }

+static int corrected_firstlineno(struct compiler *c, stmt_ty s,
+                                                                 asdl_seq * decos)


Why weird indentation on a separate line?

I think patchcheck messed up some of this up, sorry for the noise.

On the Py_UNREACHABLE() line, I originally had double tab indentation (I forgot to change Visual Studio's settings), and its seemed to change that to triple groups of 4 spaces, so I'm guessing when I ran patchcheck it caused other problems. I'll manually verify in future.

ilevkivskyi · 2018-04-24T08:03:24Z

Python/compile.c

@@ -1988,6 +2008,10 @@ compiler_function(struct compiler *c, stmt_ty s, int is_async)
    if (!compiler_decorators(c, decos))
        return 0;

+
+        int first_lineno = corrected_firstlineno(c, s, decos);


Indentation is misleading, shift left.

ilevkivskyi · 2018-04-24T08:03:48Z

Python/compile.c

@@ -2050,6 +2074,8 @@ compiler_class(struct compiler *c, stmt_ty s)
    if (!compiler_decorators(c, decos))
        return 0;

+        int first_lineno = corrected_firstlineno(c, s, decos);


Wrong indentation.

ilevkivskyi · 2018-04-24T08:04:23Z

Python/compile.c

+
+        int first_lineno = corrected_firstlineno(c, s, decos);
+
+


Too many empty lines

ilevkivskyi · 2018-04-24T08:04:40Z

Python/compile.c

@@ -1988,6 +2008,10 @@ compiler_function(struct compiler *c, stmt_ty s, int is_async)
    if (!compiler_decorators(c, decos))
        return 0;

+


Too many empty lines.

ilevkivskyi · 2018-04-24T08:05:01Z

Python/compile.c

+        else {
+                return s->lineno;
+        }
+


This empty line is unnecessary.

serhiy-storchaka

It may be worth to add a note about this change in the "Whats New" document.

serhiy-storchaka · 2018-04-24T08:14:43Z

Lib/test/test_ast.py

@@ -43,12 +44,36 @@ def to_tuple(t):
    "def f(**kwargs): pass",
    # FunctionDef with all kind of args and docstring
    "def f(a, b=1, c=None, d=[], e={}, *args, f=42, **kwargs): 'doc for f()'",
+    # Decorated FunctionDef
+    textwrap.dedent("""
+    @deco


Since you use textwrap.dedent() it would clearer if add an indentation of the inner code.

Please increase the indentation of these fragments.

serhiy-storchaka · 2018-04-24T08:15:24Z

Lib/test/test_ast.py

+    # Decorated FunctionDef
+    textwrap.dedent("""
+    @deco
+    def f():...


Nitpick: add a space after :. And maybe use pass?

serhiy-storchaka · 2018-04-24T08:23:30Z

Lib/test/test_ast.py

@@ -207,7 +232,9 @@ def _assertTrueorder(self, ast_node, parent_pos):
            parent_pos = (ast_node.lineno, ast_node.col_offset)
        for name in ast_node._fields:
            value = getattr(ast_node, name)
-            if isinstance(value, list):
+            # Since decorators are stored as an attribute of a FunctionDef
+            # they don't follow the true order of the syntax.


Maybe add a test that all decorators are before a FunctionDef?

serhiy-storchaka · 2018-04-24T08:27:46Z

Misc/NEWS.d/next/Core and Builtins/2018-04-12-14-48-55.bpo-33211.i-395-.rst

@@ -0,0 +1,3 @@
+Change the line number of decorated defs back to the def or class token in the
+AST, not the first decorator. Have the change of line number happen in the
+compiler.


Please add "Patch by *yourname"." And add your name in Misc/ACKS.

serhiy-storchaka · 2018-04-24T08:28:48Z

Python/ast.c

@@ -1686,19 +1685,14 @@ ast_for_decorated(struct compiling *c, const node *n)
           TYPE(CHILD(n, 1)) == classdef);

    if (TYPE(CHILD(n, 1)) == funcdef) {
-      thing = ast_for_funcdef(c, CHILD(n, 1), decorator_seq);
+        return ast_for_funcdef(c, CHILD(n, 1), decorator_seq);
    } else if (TYPE(CHILD(n, 1)) == classdef) {


Actually now you can remove all elses.

You'd think so, sadly MSVC warns that not all code paths return if there isn't an else.

Even with Py_UNREACHABLE()?

No, with the Py_UNREACHABLE() it is happy. Do you want something like this?

if (TYPE(CHILD(n, 1)) == funcdef) { return ast_for_funcdef(c, CHILD(n, 1), decorator_seq); } else if (TYPE(CHILD(n, 1)) == classdef) { return ast_for_classdef(c, CHILD(n, 1), decorator_seq); } else if (TYPE(CHILD(n, 1)) == async_funcdef) { return ast_for_async_funcdef(c, CHILD(n, 1), decorator_seq); } Py_UNREACHABLE();

I suggest to remove other elses too.

serhiy-storchaka · 2018-04-24T08:29:45Z

Python/compile.c

+static int corrected_firstlineno(struct compiler *c, stmt_ty s,
+                                 asdl_seq * decos)
+{
+        /* To keep the ability to get the relevant source of a decorated item


Us 4 spaces for indentation.

serhiy-storchaka · 2018-04-24T08:30:42Z

Python/compile.c

+                                 asdl_seq * decos)
+{
+        /* To keep the ability to get the relevant source of a decorated item
+        using inspect.getsource, we need to keep the first line number


Align wrapped lines to "To keep", not to "/*".

serhiy-storchaka · 2018-04-24T08:32:07Z

Python/compile.c

+        This used to be done via the AST, but it is better to hide this
+        internally.
+        */
+        if (asdl_seq_LEN(decos) > 0) {


> 0 is not needed. Using just asdl_seq_LEN(decos) in the boolean context is common.

serhiy-storchaka · 2018-04-24T08:36:00Z

Python/compile.c

+        */
+        if (asdl_seq_LEN(decos) > 0) {
+                expr_ty first_decorator = asdl_seq_GET(decos, 0);
+                c->u->u_firstlineno = first_decorator->lineno;


Seems c->u still refers to the outer scope.

Add a regression test for these cases:

def f(): @deco def g(): pass

class A: @deco def m(self): pass

Seems c->u still refers to the outer scope.

I'm probably missing something, but the generated AST is:

('Module', [('ClassDef', (1, 0), 'A', [], [], [('FunctionDef', (3, 4), 'm', ('arguments', [('arg', (3, 10), 'self', None)], None, [], [], None, []), [('Pass', (3, 17))], [('Name', (2, 5), 'deco', ('Load',))], None, None)], [], None)], None)

which seems correct. Perhaps I am misunderstanding?

serhiy-storchaka

The generated AST is correct, the generated code is wrong. Look at f.__code__.co_firstlineno in

def f():
    @deco
    def g(): pass

and

def f():
    @deco
    class A: pass

serhiy-storchaka · 2018-04-24T09:45:05Z

Python/ast.c

@@ -1686,19 +1685,14 @@ ast_for_decorated(struct compiling *c, const node *n)
           TYPE(CHILD(n, 1)) == classdef);

    if (TYPE(CHILD(n, 1)) == funcdef) {
-      thing = ast_for_funcdef(c, CHILD(n, 1), decorator_seq);
+        return ast_for_funcdef(c, CHILD(n, 1), decorator_seq);
    } else if (TYPE(CHILD(n, 1)) == classdef) {


Even with Py_UNREACHABLE()?

emmatyping · 2018-04-24T21:46:37Z

@serhiy-storchaka

The generated AST is correct, the generated code is wrong.

Thank you for catching this. I've added tests to both test_ast and test_inspect to make sure the AST and inspect.getsource don't regress.

Also, I believe I have responded to all of your feedback. Thank you!

serhiy-storchaka

Why there are changes in Python/importlib.h and Python/importlib_external.h? I thought the purpose of this PR is changing line numbers in AST, but keeping the bytecode unchanged.

serhiy-storchaka · 2018-04-25T11:16:14Z

Lib/test/test_ast.py

@@ -43,12 +44,36 @@ def to_tuple(t):
    "def f(**kwargs): pass",
    # FunctionDef with all kind of args and docstring
    "def f(a, b=1, c=None, d=[], e={}, *args, f=42, **kwargs): 'doc for f()'",
+    # Decorated FunctionDef
+    textwrap.dedent("""
+    @deco


Please increase the indentation of these fragments.

serhiy-storchaka · 2018-04-25T11:23:06Z

Python/compile.c

+    // modify first line number for decorated nodes
+    if (asdl_seq_LEN(decos)) {
+        expr_ty first_decorator = asdl_seq_GET(decos, 0);
+        c->u->u_firstlineno = first_decorator->lineno;


Is not it set in compiler_enter_scope()?

serhiy-storchaka · 2018-04-25T11:24:11Z

Python/compile.c

+        // modify first line number for decorated nodes
+        if (asdl_seq_LEN(decos)) {
+            expr_ty first_decorator = asdl_seq_GET(decos, 0);
+            c->u->u_firstlineno = first_decorator->lineno;


Is not it set in compiler_enter_scope()?

emmatyping · 2018-04-25T11:56:56Z

Why there are changes in Python/importlib.h and Python/importlib_external.h? I thought the purpose of this PR is changing line numbers in AST, but keeping the bytecode unchanged.

Yes, that is concerning. I think I will take a closer look at what is going on. W.R.T your comments on compiler_enter_scope, it seems that the modified line number actually doesn't do anything! The change needed is just c->u->u_firstlineno after the enter scope.

So based on these I think I need to figure out what is actually going on before I proceed, I still have much to learn about the compiler's intricacies.

gvanrossum · 2018-04-30T04:08:11Z

Um, it seems you still need to act on the comment from 5 days ago?

emmatyping · 2018-04-30T06:30:24Z

Um, it seems you still need to act on the comment from 5 days ago?

Ah, yes I haven't pushed anything because I haven't found out why the generated bytecode is different yet. I will once I figure things out more.

emmatyping · 2018-05-02T07:59:46Z

Sorry this is temorarily stalled until I can get my computer working again...

gvanrossum · 2018-05-07T16:25:33Z

@serhiy-storchaka What do you think? Is it ready to be merged? I don't suppose this can make it into 3.7, since IIUC the breakage is pretty noticeable.

serhiy-storchaka · 2018-05-07T16:41:56Z

If I understand it correctly, the bytecode shouldn't be affected by this PR. But the frozen bytecode is changed, and I don't understand the cause. Either there is a bug in the code generation introduced by this PR, or my understanding is wrong.

I like this PR in general and hope Ethan and me will found the cause.

emmatyping · 2018-05-07T19:43:05Z

If I understand it correctly, the bytecode shouldn't be affected by this PR. But the frozen bytecode is changed, and I don't understand the cause.

Yes, this is correct. I am going to try to spend more time today trying to understand why it changes the bytecode.

ilevkivskyi · 2018-11-22T22:31:20Z

@ethanhs
I just tested, normal functions, coroutines, and classes all work correctly with decorators on master. Is this PR still relevant, or it can be closed?

emmatyping · 2018-11-22T22:38:07Z

@ilevkivskyi Serhiy asked me to see if there was anything useful in this PR that could be pulled out in the original issue https://bugs.python.org/issue33211#msg330181 (He accidentally redid this work on his own). I don't see anything that would be beneficial that is in this PR and not in #9731, so I will close this.

emmatyping added 2 commits April 12, 2018 14:58

Change line number of decorated nodes back to def

b60b6a6

Whitespace fixes

ae578b0

the-knights-who-say-ni added the CLA signed label Apr 12, 2018

bedevere-bot added the awaiting review label Apr 12, 2018

emmatyping added 5 commits April 12, 2018 15:47

Fix test_dis

191bd38

Change fistlineno of decorated nodes in the compiler

da42832

Remove unused var

649506c

Fix indentation bug

29b0d60

Run patcheck

68b2228

ilevkivskyi reviewed Apr 22, 2018

View reviewed changes

serhiy-storchaka self-requested a review April 22, 2018 21:11

serhiy-storchaka reviewed Apr 22, 2018

View reviewed changes

Merge remote tracking branch into deflineno

09d8ab2

emmatyping added 2 commits April 23, 2018 16:31

Change tests back to original state

df9fbe2

Fix line number in compiler

9840f84

ilevkivskyi approved these changes Apr 23, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Apr 23, 2018

emmatyping added 2 commits April 23, 2018 16:41

Use unreachable macro

0842c20

Fix silly indentation mistake

7f4fcdf

emmatyping added 2 commits April 23, 2018 17:03

Test ast line numbers for decorated nodes

e4e7f1a

Fix true order tests for decorators

a8e747d

ilevkivskyi reviewed Apr 24, 2018

View reviewed changes

Fix bad indentation

3df5862

ilevkivskyi approved these changes Apr 24, 2018

View reviewed changes

serhiy-storchaka reviewed Apr 24, 2018

View reviewed changes

Respond to review

44a3cd9

serhiy-storchaka reviewed Apr 24, 2018

View reviewed changes

Fix regression with inner decorated nodes

afd3b7e

serhiy-storchaka reviewed Apr 25, 2018

View reviewed changes

emmatyping mentioned this pull request Apr 29, 2018

[DO NOT MERGE] Fix def line numbers for decorated nodes python/typed_ast#61

Closed

emmatyping added 2 commits May 6, 2018 18:48

Remove redundant line number setting

bdc9942

Fix style in compile.c

914d0a0

emmatyping force-pushed the deflineno branch from ad00dc4 to 914d0a0 Compare May 7, 2018 06:32

emmatyping closed this Nov 22, 2018

emmatyping deleted the deflineno branch November 22, 2018 22:38

		@@ -1988,6 +2008,10 @@ compiler_function(struct compiler *c, stmt_ty s, int is_async)
		if (!compiler_decorators(c, decos))
		return 0;

Uh oh!

bpo-33211: Change line number of decorated nodes back to def #6460

bpo-33211: Change line number of decorated nodes back to def #6460

Uh oh!

Conversation

emmatyping commented Apr 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmatyping commented Apr 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmatyping commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmatyping Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmatyping commented Apr 12, 2018 •

edited

Loading

emmatyping commented Apr 22, 2018 •

edited

Loading

emmatyping commented Apr 23, 2018 •

edited

Loading

emmatyping Apr 24, 2018 •

edited

Loading

ilevkivskyi Apr 24, 2018 •

edited

Loading