8000 bpo-33211: Change line number of decorated nodes back to def by emmatyping · Pull Request #6460 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 19 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b60b6a6
Change line number of decorated nodes back to def
emmatyping Apr 12, 2018
ae578b0
Whitespace fixes
emmatyping Apr 12, 2018
191bd38
Fix test_dis
emmatyping Apr 12, 2018
da42832
Change fistlineno of decorated nodes in the compiler
emmatyping Apr 12, 2018
649506c
Remove unused var
emmatyping Apr 12, 2018
29b0d60
Fix indentation bug
emmatyping Apr 12, 2018
68b2228
Run patcheck
emmatyping Apr 13, 2018
09d8ab2
Merge remote tracking branch into deflineno
emmatyping Apr 22, 2018
df9fbe2
Change tests back to original state
emmatyping Apr 23, 2018
9840f84
Fix line number in compiler
emmatyping Apr 23, 2018
0842c20
Use unreachable macro
emmatyping Apr 23, 2018
7f4fcdf
Fix silly indentation mistake
emmatyping Apr 23, 2018
e4e7f1a
Test ast line numbers for decorated nodes
emmatyping Apr 24, 2018
a8e747d
Fix true order tests for decorators
emmatyping Apr 24, 2018
3df5862
Fix bad indentation
emmatyping Apr 24, 2018
44a3cd9
Respond to review
emmatyping Apr 24, 2018
afd3b7e
Fix regression with inner decorated nodes
8000 emmatyping Apr 24, 2018
bdc9942
Remove redundant line number setting
emmatyping May 7, 2018
914d0a0
Fix style in compile.c
emmatyping May 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Lib/test/test_dis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


dis_c_static_method = """\
%3d 0 LOAD_FAST 0 (x)
Expand All @@ -74,7 +74,7 @@ def cm(cls, x):
6 STORE_FAST 0 (x)
8 LOAD_CONST 0 (None)
10 RETURN_VALUE
""" % (_C.sm.__code__.co_firstlineno + 2,)
""" % (_C.sm.__code__.co_firstlineno + 1,)

# Class disassembling info has an extra newline at end.
dis_c = """\
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2018-04-12-14-48-55.bpo-33211.i-395-.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Copy link
Member

Choose a reason for hiding this 8000 comment

The reason will be displayed to describe this comment to others. Learn more.

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

16 changes: 5 additions & 11 deletions Python/ast.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1672,7 +1672,6 @@ static stmt_ty
ast_for_decorated(struct compiling *c, const node *n)
{
/* decorated: decorators (classdef | funcdef | async_funcdef) */
stmt_ty thing = NULL;
asdl_seq *decorator_seq = NULL;

REQ(n, decorated);
Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually now you can remove all elses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with Py_UNREACHABLE()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to remove other elses too.

thing = ast_for_classdef(c, CHILD(n, 1), decorator_seq);
return ast_for_classdef(c, CHILD(n, 1), decorator_seq);
} else if (TYPE(CHILD(n, 1)) == async_funcdef) {
thing = ast_for_async_funcdef(c, CHILD(n, 1), decorator_seq);
}
/* we count the decorators in when talking about the class' or
* function's line number */
if (thing) {
thing->lineno = LINENO(n);
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe then assert 0?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Py_UNREACHABLE()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still not fixed.

}
return thing;
}

static expr_ty
Expand Down
6 changes: 6 additions & 0 deletions Python/compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1982,6 +1982,12 @@ compiler_function(struct compiler *c, stmt_ty s, int is_async)
if (!compiler_decorators(c, decos))
return 0;

// 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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many empty lines

funcflags = compiler_default_arguments(c, args);
if (funcflags == -1) {
return 0;
Expand Down
Loading
0