Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-95238: lazily compute line number for tracebacks #95237

Closed
wants to merge 5 commits into from

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Jul 25, 2022

@kumaraditya303 kumaraditya303 changed the title lazy line number GH-95238: lazily compute line number for tracebacks Jul 25, 2022
@kumaraditya303 kumaraditya303 marked this pull request as ready for review July 25, 2022 11:19
@markshannon
Copy link
Member

This PR handles line numbers, but the issue is about lazily creating the whole traceback.
Does the title of one of the other need changing?

@kumaraditya303
Copy link
Contributor Author

This PR handles line numbers, but the issue is about lazily creating the whole traceback. Does the title of one of the other need changing?

I updated the issue title to restrict it to this specific change, I'll add a blurb entry.

@gvanrossum
Copy link
Member

Is the tb_lineno field documented in the C API? It would appear to be public. Putting negative values there might break C code that's interpreting tracebacks somehow. You'd be surprised how many packages exist that do things like this (we found out when changing much more obscure parts of the interpreter state).

@kumaraditya303
Copy link
Contributor Author

Is the tb_lineno field documented in the C API?

As far as I can see on https://docs.python.org/3/search.html?q=tb_lineno&check_keywords=yes&area=default tb_lineno is not documented as part of the C API.

@gvanrossum
Copy link
Member

As far as I can see on https://docs.python.org/3/search.html?q=tb_lineno&check_keywords=yes&area=default tb_lineno is not documented as part of the C API.

That's a relief -- OTOH it's still part of the "regular" API (traceback.h is included by Python.h unless in limited ABI mode) and I am not 100% comfortable changing its meaning.

Maybe you can search GitHub for references to this symbol?

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Jul 25, 2022

That's a relief -- OTOH it's still part of the "regular" API (traceback.h is included by Python.h unless in limited ABI mode) and I am not 100% comfortable changing its meaning.

We can provide a new function to get tb_lineno of traceback in 3.12 and provide backwards shim in https://github.com/python/pythoncapi-compat and document it in porting to 3.12 to use the function if it becomes a problem

@gvanrossum
Copy link
Member

We can provide a new function to get tb_lineno of traceback in 3.12 and provide backwards shim in https://github.com/python/pythoncapi-compat and document it in porting to 3.12 to use the function if it becomes a problem

That doesn't answer my question -- I'm trying to answer the question of how many people will find that they have to make changes to their code to adopt 3.12 due to this change. Because we are breaking backwards compatibility for them.

@markshannon
Copy link
Member

I agree with both of you.
tb_lineno is not part of the API, but we shouldn't change its meaning.

The problem is that the name of the C field is same as the Python attribute, so writers of C extensions assume that they can read the tb_lineno field directly and that tb->tb_lineno will be the same as PyObject_GetAttrString(tb, "tb_lineno") even though that has never been the intention.
We had the same problem with fields in the frame object.

C extensions authors should not make these assumptions, and they are wrong in doing so, but they do it anyway.
The danger is not that we break those C extensions, but that we so silently.
Changing the name of the field will break those extension noisily.

If we are going to change the semantics of tb_lineno we should also change its name, provide a C-API function to access the traceback's line number and document how to use it in the "porting to 3.12" section of the release notes.

@kumaraditya303
Copy link
Contributor Author

kumaraditya303 commented Aug 20, 2022

@markshannon Should I create a separate issue to add a C API function or just document to read the field with PyObject_GetAttrString and change the C level name?

@markshannon
Copy link
Member

Probably best to add a C API function, as you'll need to implement the function anyway. No need for a separate PR.

@kumaraditya303
Copy link
Contributor Author

Probably best to add a C API function, as you'll need to implement the function anyway. No need for a separate PR.

Sure, I'll add a C API function.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM

@kumaraditya303 kumaraditya303 added performance Performance or resource usage 3.12 bugs and security fixes labels Aug 30, 2022
@kumaraditya303 kumaraditya303 deleted the lazy-tracebacks branch September 26, 2022 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes awaiting review performance Performance or resource usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants