Skip to content

gh-122239: Add actual count in unbalanced unpacking error message when possible #122244

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.

Sign up for GitHub

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
pablogsal merged 28 commits into from
Sep 10, 2024
Merged

gh-122239: Add actual count in unbalanced unpacking error message when possible #122244

Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
71d53eb
add actual iterable count in error message
tusharsadhwani Jul 24, 2024
e55ce9c
Merge remote-tracking branch 'upstream/main' into argcnt-errmsg
tusharsadhwani Jul 24, 2024
04595e5
add test for non-sequence type
tusharsadhwani Jul 24, 2024
b8dc5eb
make test pass for non-sequences
tusharsadhwani Jul 24, 2024
e444582
Add news entry
tusharsadhwani Jul 24, 2024
525447c
rephase news
tusharsadhwani Jul 24, 2024
272cc6a
PEP7
tusharsadhwani Jul 24, 2024
43beedf
Fallback to PyMapping_Size
tusharsadhwani Jul 26, 2024
f40e782
remove whitespace
tusharsadhwani Jul 26, 2024
32dbf36
Merge branch 'main' into argcnt-errmsg
tusharsadhwani Jul 26, 2024
0c741e2
Update Python/ceval.c
tusharsadhwani Jul 26, 2024
0a2f9ba
remove stale comment
tusharsadhwani Jul 26, 2024
ea5a84a
Handle weird cases
tusharsadhwani Jul 27, 2024
69a5f9f
lint
tusharsadhwani Jul 27, 2024
cd0d534
use ChainExceptions1 to simplify code
tusharsadhwani Jul 27, 2024
088f90f
handle BaseException propagation
tusharsadhwani Aug 2, 2024
d8df9bc
remove whitespace
tusharsadhwani Aug 2, 2024
95a41b5
Merge branch 'main' into argcnt-errmsg
tusharsadhwani Aug 2, 2024
6e8c53f
flip the logic to simplify it
tusharsadhwani Aug 2, 2024
a5d926f
rollback to the simpler approach
tusharsadhwani Aug 28, 2024
fe9be07
line length
tusharsadhwani Aug 28, 2024
4bcb19f
Use PyDict_Size for dicts
tusharsadhwani Aug 29, 2024
d68ba12
simplify code
tusharsadhwani Sep 4, 2024
f183de8
Apply suggestions from code review
tusharsadhwani Sep 4, 2024
1b4c21b
indentation fix
tusharsadhwani Sep 4, 2024
bb1f132
Update Misc/NEWS.d/next/Core_and_Builtins/2024-07-25-01-45-21.gh-issu…
tusharsadhwani Sep 9, 2024
9b94761
Merge branch 'main' into argcnt-errmsg
tusharsadhwani Sep 9, 2024
b05bd53
Add whatsnew entry
tusharsadhwani Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update Python/ceval.c 
Co-authored-by: blhsing <blhsing@gmail.com>
  • Loading branch information
@tusharsadhwani @blhsing
tusharsadhwani and blhsing authored Jul 26, 2024
commit 0c741e27b1f0d5efda9690717cc5f6b1b94bead6
5 changes: 1 addition & 4 deletions Python/ceval.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2165,10 +2165,7 @@ _PyEval_UnpackIterableStackRef(PyThreadState *tstate, _PyStackRef v_stackref,
Py_DECREF(w);

/* If v is a sequence or mapping, we can show its length in the error */
ll = PySequence_Size(v);
if (ll < 0) {
ll = PyMapping_Size(v);
}
ll = PyObject_Size(v);
if (ll < 0) {
Copy link
Copy Markdown
Member

@encukou encukou Jul 26, 2024

Choose a reason for hiding this comment

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

Suggested change
if (ll < 0) {
if (ll <= argcnt) {

Please consider, and test, evil classes that lie about their size. For example:

class EvilLiar:
  def __len__(self):
      return 2

  def __iter__(self):
      yield from (1, 2, 3)

a, b = EvilLiar()

At this level, I don't think we want to print (expected 2, got 2), even if user is at fault here.

In other evil classes, __len__ can fail with an exception. Please test this.
IMO, if it's not a TypeError/AttributeError, and especially if it's a BaseException but not Exception, it needs to be chained (e.g. using PyException_SetContext).

Other evil classes can have side effects, or take a long time. IMO, that's OK -- an extra __len__ call shouldn't hurt.

Copy link
Copy Markdown
Contributor Author

@tusharsadhwani tusharsadhwani Jul 26, 2024

Choose a reason for hiding this comment

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

Thanks for catching this, I'll test and take care of these cases.

Copy link
Copy Markdown
Contributor Author

@tusharsadhwani tusharsadhwani Jul 27, 2024

Choose a reason for hiding this comment

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

I think I've handled the weird cases now, but I couldn't figure out what would raise an AttributeError in the normal case, so I'm only ignoring TypeError's context right now.

There's also a bunch of code duplication for _PyErr_Format, should it be deduped in some way?

_PyErr_Format(tstate, PyExc_ValueError,
"too many values to unpack (expected %d)",
Expand Down