◐ Shell
reader mode source ↗
Skip to content

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

Closed
emmatyping wants to merge 19 commits into
python:masterfrom
emmatyping:deflineno
Closed

bpo-33211: Change line number of decorated nodes back to def#6460
emmatyping wants to merge 19 commits into
python:masterfrom
emmatyping:deflineno

Conversation

@emmatyping

@emmatyping emmatyping commented Apr 12, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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
ilevkivskyi reviewed Apr 22, 2018
View reviewed changes

@ilevkivskyi ilevkivskyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

@serhiy-storchaka serhiy-storchaka self-requested a review April 22, 2018 21:11
serhiy-storchaka
serhiy-storchaka reviewed Apr 22, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

Please add test for AST line numbers.

@emmatyping

emmatyping commented Apr 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

@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.

@emmatyping

emmatyping commented Apr 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

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
ilevkivskyi reviewed Apr 24, 2018
View reviewed changes

@ilevkivskyi ilevkivskyi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

This is almost ready.

serhiy-storchaka
serhiy-storchaka reviewed Apr 24, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

serhiy-storchaka
serhiy-storchaka reviewed Apr 24, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

@emmatyping

emmatyping commented Apr 24, 2018

Copy link
Copy Markdown
Member Author

@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
serhiy-storchaka reviewed Apr 25, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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.

@emmatyping

emmatyping commented Apr 25, 2018

Copy link
Copy Markdown
Member Author

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.

@emmatyping emmatyping mentioned this pull request Apr 29, 2018
Closed
@gvanrossum

gvanrossum commented Apr 30, 2018

Copy link
Copy Markdown
Member

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

@emmatyping

emmatyping commented Apr 30, 2018

Copy link
Copy Markdown
Member Author

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

emmatyping commented May 2, 2018

Copy link
Copy Markdown
Member Author

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

@gvanrossum

gvanrossum commented May 7, 2018

Copy link
Copy Markdown
Member

@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

serhiy-storchaka commented May 7, 2018

Copy link
Copy Markdown
Member

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

emmatyping commented May 7, 2018

Copy link
Copy Markdown
Member Author

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

ilevkivskyi commented Nov 22, 2018

Copy link
Copy Markdown
Member

@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

emmatyping commented Nov 22, 2018

Copy link
Copy Markdown
Member Author

@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 emmatyping closed this Nov 22, 2018
@emmatyping emmatyping deleted the deflineno branch November 22, 2018 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@ilevkivskyi ilevkivskyi ilevkivskyi approved these changes

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@emmatyping @gvanrossum @serhiy-storchaka @ilevkivskyi @the-knights-who-say-ni @bedevere-bot