Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

bpo-43950: Initial base implementation for PEP 657#26955

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.

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

Already on GitHub?Sign in to your account

Merged
pablogsal merged 1 commit intopython:mainfromcolnotab:bpo-43950
Jul 2, 2021

Conversation

@pablogsal
Copy link
Member

@pablogsalpablogsal commentedJun 29, 2021
edited by isidentical
Loading

⚠️ This is not the full feature and contains no optimizations for the time being, just the base implementation to avoid merge conflicts in the future⚠️

All the internal structures, and data representation are subjected to change in future PRs as we add optimizations

This PR is part of PEP 657 and augments the compiler to emit ending
line numbers as well as starting and ending columns from the AST
into compiled code objects. This allows bytecodes to be correlated
to the exact source code ranges that generated them.

This information is made available through the following public APIs:

  • Theco_positions method on code objects.
  • The C API functionPyCode_Addr2Location.

Co-authored-by: Pablo GalindoPablogsal@gmail.com
Co-authored-by: Batuhan Taskayaisidentical@gmail.com
Co-authored-by: Ammar Askerammar@ammaraskar.com

https://bugs.python.org/issue43950

@isidentical
Copy link
Member

A couple doctest failures FYI:https://travis-ci.com/github/python/cpython/jobs/520228793

@ammaraskarammaraskarforce-pushed thebpo-43950 branch 2 times, most recently fromb53b276 to0673116CompareJune 30, 2021 04:49
@pablogsalpablogsal added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelJun 30, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@pablogsal for commit 0673116a6fa30547c1138fd3f96059412d99a979 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelJun 30, 2021
@markshannon
Copy link
Member

Instead of adding three additional location fields to each instruction, and continually update the equivalent fields in the scope struct, why not changei_lineno toi_ast_node to hold the AST node with the matching location?

Doesn't PEP 657 require that the locations match those of the AST exactly?

@pablogsal
Copy link
MemberAuthor

pablogsal commentedJun 30, 2021
edited
Loading

Instead of adding three additional location fields to each instruction, and continually update the equivalent fields in the scope struct, why not changei_lineno toi_ast_node to hold the AST node with the matching location?

That would be basically the same because you are modifiying these fields in the compilation and assembler steps. For example:

...b->b_instr[i].i_lineno = prev_lineno;...

that means that if we use ai_ast_node pointer all instructions will share the same ast node, so changes to the lineno of one instruction will affect others, which is a big problem. So either we start adding a bunch of macros to get and set to handle the default case a NULL pointer fori_ast_node or we would end copiying the ast node location structure, which is almost exactly what we are doing currently.

Doesn't PEP 657 require that the locations match those of the AST exactly?

No, the PEP just states what is the source of the information, but it doesn't explain how is propagated. So we have some freedom to select from which nodes do we pick the information or to "fix it" if we ever need to.

@pablogsal
Copy link
MemberAuthor

@isidentical By the way, it seems that there are more places where we are settinglineno = -1 that we didn't update to also account for the new fields

@pablogsalpablogsalforce-pushed thebpo-43950 branch 2 times, most recently from9ca2578 to2498168CompareJuly 1, 2021 00:12
@ammaraskar
Copy link
Member

@terryjreedy Adding you as a reviewer as requested. You might not be too interested in this part, it's just exposing the lower level offset data but the tests might give you sort of an idea on how they can be used from Python. The follow up PRs will add thetraceback.py changes that you might be more interested in.

@isidenticalisidentical added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelJul 1, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@isidentical for commit ba64180d3f61ee75b2b624ae2055ec22a2fb27fc 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelJul 1, 2021
Copy link
Member

@terryjreedyterryjreedy left a comment

Choose a reason for hiding this comment

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

If I understand, the only officially visible change is the addition of code.co_positions, which returns an iterable of (start_line, end_line, start_column, end_column) tuples, where column numbers are 1-based. Suggested title after bpo tag: "Add code.co_positions (PEP 657)" or "AddPEP-657 code.co_positions".

I presume a doc change will come. I will review it for readability if pinged.

I read the tests and have two suggestions.

This PR is part of PEP 657 and augments the compiler to emit endingline numbers as well as starting and ending columns from the ASTinto compiled code objects. This allows bytecodes to be correlatedto the exact source code ranges that generated them.This information is made available through the following public APIs:* The `co_positions` method on code objects.* The C API function `PyCode_Addr2Location`.Co-authored-by: Batuhan Taskaya <isidentical@gmail.com>Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
@ammaraskar
Copy link
Member

ammaraskar commentedJul 2, 2021
edited
Loading

Amended the commit message as per Terry's recommendation and rebased#26958 on top of the latest changes, tests for the traceback changes are still passing.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@encukouencukouencukou left review comments

@ammaraskarammaraskarammaraskar left review comments

@terryjreedyterryjreedyterryjreedy left review comments

@Fidget-SpinnerFidget-SpinnerFidget-Spinner left review comments

@isidenticalisidenticalisidentical approved these changes

Assignees

No one assigned

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@pablogsal@isidentical@bedevere-bot@markshannon@ammaraskar@encukou@terryjreedy@Fidget-Spinner@the-knights-who-say-ni

[8]ページ先頭

©2009-2025 Movatter.jp