Skip to content

fix(stdlib/types): FrameType.f_lineno can be None #6769

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

Merged
merged 1 commit into from
Jan 2, 2022

Conversation

jd
Copy link
Contributor

@jd jd commented Dec 31, 2021

In very rare (error?) cases, f_lineno can actually be None.

In very rare (error?) cases, f_lineno can actually be None.
@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/_code/code.py:140: error: Unsupported operand types for - ("None" and "int")  [operator]
+ src/_pytest/_code/code.py:140: note: Left operand is of type "Optional[int]"
+ testing/deprecated_test.py:125: error: Unsupported operand types for > ("int" and "None")  [operator]
+ testing/deprecated_test.py:125: note: Left operand is of type "Optional[int]"
+ testing/deprecated_test.py:125: error: Unsupported operand types for < ("int" and "None")  [operator]
+ testing/deprecated_test.py:125: note: Right operand is of type "Optional[int]"

rich (https://github.com/willmcgugan/rich)
+ rich/console.py:1823: error: Incompatible return value type (got "Tuple[str, Optional[int], Dict[str, Any]]", expected "Tuple[str, int, Dict[str, Any]]")

@srittau
Copy link
Collaborator

srittau commented Jan 2, 2022

Do you have a test case? The frame object is implemented in C, and has type int, so if it is int, something must have been gone severly wrong:

https://github.com/python/cpython/blob/60929576e40038ec71d896230f69e4411c82be4b/Include/cpython/frameobject.h#L12

There's also some code in CPython that assumes it not to be None.

@Akuli
Copy link
Collaborator

Akuli commented Jan 2, 2022

This getter returns None if that int is negative: https://github.com/python/cpython/blob/60929576e40038ec71d896230f69e4411c82be4b/Objects/frameobject.c#L55-L65

Edit: It also returns None when the int in struct _frame is zero and PyCode_Addr2Line fails.

@srittau
Copy link
Collaborator

srittau commented Jan 2, 2022

Good catch, and a bit unfortunate that the Python docs and implementation disagree.

@srittau srittau merged commit 1bfcf30 into python:master Jan 2, 2022
@hauntsaninja
Copy link
Collaborator

I'm still curious if we have a test case. When would int be negative / when would PyCode_Addr2Line fail? This has non negligible primer impact, so would be useful to be able to explain the issue in the not unlikely case that users complain.

@hauntsaninja
Copy link
Collaborator

That's a couple thumbs ups, but no replies :-)

@jd Any luck with a test case? It sucks to tell users "this can be None, but we don't know when or why". There's also a chance you've run into a CPython bug.

@jd
Copy link
Contributor Author

jd commented Jan 4, 2022

Sorry, I don't have a good test case. I've seen this happening randomly while using the profiler project I work on, but it was reported by a user, not something I saw firsthand.

If None bothers people, it might be good to "fix" CPython to return, e.g., 0 instead.

@Akuli
Copy link
Collaborator

Akuli commented Jan 4, 2022

Here's a test case:

def f():
    1/0

f.__code__ = f.__code__.replace(co_linetable=b'\x04\x80\xff\x80')

try:
    f()
except Exception as ex:
    t = ex.__traceback__
    print(t.tb_next.tb_frame.f_lineno)

I created this by adding a printf statement to CPython where it returns None, running ./python -m test, and reducing the part of the test suite that made the printf run.

That said, the test case seems contrived and not something that would come up in practice, so I'm not sure if it's very helpful.

@AlexWaygood
Copy link
Member

Here's a case where lineno is None (but note that it's been reported as a 3.11 regression): https://bugs.python.org/issue46389

@Akuli
Copy link
Collaborator

Akuli commented Jan 15, 2022

I think we should just use int | Any. This way you can assume that lineno is an int, or you can check if it's None, and mypy won't complain either way. We already do this for regex groups, which return None much more commonly.

AlexWaygood added a commit to AlexWaygood/typeshed that referenced this pull request Jan 17, 2022
@AlexWaygood
Copy link
Member

I think we should just use int | Any.

Here's a PR doing that: #6935

jd added a commit to jd/dd-trace-py that referenced this pull request Jan 17, 2022
…er None

In some cases, f_lineno can be `None` which would trigger an error in the pprof
output. This is not even caught by mypy, and I've proposed a fix here:

  python/typeshed#6769

This patch makes sure we always use 0 if the line number is `None`.

It also makes sure a funcname is always passed — in practice it's now the case,
but make sure typing reflects that.

Fixes DataDog#3046
@jd jd deleted the fix-lineno branch January 18, 2022 15:25
Kyle-Verhoog pushed a commit to DataDog/dd-trace-py that referenced this pull request Jan 18, 2022
…er None (#3099)

In some cases, f_lineno can be `None` which would trigger an error in the pprof
output. This is not even caught by mypy, and I've proposed a fix here:

  python/typeshed#6769

This patch makes sure we always use 0 if the line number is `None`.

It also makes sure a funcname is always passed — in practice it's now the case,
but make sure typing reflects that.

Fixes #3046
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants