Fix Code.getargs() treating co_flags bitmask values as counts - #14493
Conversation
Sorry, something went wrong.
CO_VARARGS (4) and CO_VARKEYWORDS (8) are bitmask flags, but they were used directly as increment values for the argument count via `&`. This caused co_varnames[:argcount] to slice beyond the actual arguments when the function body contained enough local variables. Fix by wrapping the bitmask results in bool(), so they contribute at most 1 to the argcount. Closes pytest-dev#14492
for more information, see https://pre-commit.ci
|
You were right about kwonly args. The layout of co_varnames in Python 3.8+ puts kwonly args between *args and **kwargs, so without accounting for co_kwonlyargcount the slice grabs the kwonly name before hitting z. Pushed a fix that adds raw.co_kwonlyargcount to argcount when var=True, and added a test case with a kwonly arg (f6) to cover it. ptal. |
Sorry, something went wrong.
|
man, this one is a messy piece with a history - i wonder if signature could be used in more modern code to just use the names from the signature that being said - the remenants of py.code is one of the painful legacies that evolved back in 2005 when it was modern code growing out of the testing utilities of the pypy project - now its 20 years later and python changed quite a bit |
Sorry, something went wrong.
yeah this needs more thought. I’ll come back to it tomorrow. |
Sorry, something went wrong.
co_varnames layout is: positional args, kwonly args, *args, **kwargs, locals. The f6 test expected the wrong order. Also suppress ruff F841 false positives on locals deliberately placed to test co_varnames slicing.
Codecov reports coverage for testing/ files since the project config includes them. The function bodies of f5/f6 (local var assignments) were never executed because the test only used Code.from_function(), never called the functions. Replace raise NotImplementedError() with actual calls to cover all diff lines.
|
kwonly args are now accounted for and CI is all green. The inspect.signature idea makes sense for a future cleanup — the current co_varnames slicing works but it's fragile with every Python release adding new co_* fields. |
Sorry, something went wrong.
| @@ -0,0 +1 @@ | |||
| Fixed ``Code.getargs()`` incorrectly including local variable names in the returned argument tuple for functions with ``*args`` and/or ``**kwargs``. The method was using ``co_flags`` bitmask values (``4`` and ``8``) directly as counts instead of converting them to ``1`` via ``bool()``. | |||
There was a problem hiding this comment.
Could the changelog entry also mention the kwonly fix?
account for co_kwonlyargcount in getargs when var=True
Right now it only talks about the bitmask, but someone affected by the kwonly case wouldn't find their bug in the release notes.
Sorry, something went wrong.
There was a problem hiding this comment.
Good point, updated the changelog to mention the kwonly fix.
Sorry, something went wrong.
RonnyPfannschmidt
left a comment
There was a problem hiding this comment.
looks good now
do you want to squash or should we
@nicoddemus im on the fence on whether this should be marked for backlog
Sorry, something went wrong.
|
feel free to squash on merge — whatever's easier on your side. |
Sorry, something went wrong.
|
Thanks @EternalRights!
Seems OK to backport, I will do it. 👍 |
Sorry, something went wrong.
Backport to 9.0.x: 💚 backport PR created✅ Backport PR branch: patchback/backports/9.0.x/f976bb67b293cf358e2fbd0352638e2f5a9da8e0/pr-14493 Backported as #14535 🤖 @patchback |
Sorry, something went wrong.
#14535) CO_VARARGS (4) and CO_VARKEYWORDS (8) are bitmask flags, but they were used directly as increment values for the argument count via `&`. This caused co_varnames[:argcount] to slice beyond the actual arguments when the function body contained enough local variables. Fix by wrapping the bitmask results in bool(), so they contribute at most 1 to the argcount. Closes #14492 (cherry picked from commit f976bb6) Co-authored-by: EternalRights <162705204+EternalRights@users.noreply.github.com>
As a follow-up to pytest-dev#14493, replace direct co_* accesses in Code.path and Code.getargs with inspect.getfile and inspect.getfullargspec respectively, delegating argument-decoding logic to well-tested stdlib APIs rather than re-implementing it with raw bitmask arithmetic. Code.firstlineno and Code.name still access co_firstlineno and co_name directly: the inspect module has no equivalent for these that does not require reading the source file from disk, which would regress correctness for compile()'d code and performance for every traceback formatting path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ess (#14536) As a follow-up to #14493, replace direct co_* accesses in Code.path and Code.getargs with inspect.getfile and inspect.getfullargspec respectively, delegating argument-decoding logic to well-tested stdlib APIs rather than re-implementing it with raw bitmask arithmetic. Code.firstlineno and Code.name still access co_firstlineno and co_name directly: the inspect module has no equivalent for these that does not require reading the source file from disk, which would regress correctness for compile()'d code and performance for every traceback formatting path. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
CO_VARARGS (4) and CO_VARKEYWORDS (8) are bitmask flags, but they were used directly as increment values for the argument count via &. This caused co_varnames[:argcount] to slice beyond the actual arguments when the function body contained enough local variables.
Fix by wrapping the bitmask results in bool(), so they contribute at most 1 to the argcount.
Closes #14492