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

[tusharsadhwani]

This should allow showing the actual count of received items during an unbalanced unpacking, if the value has a pre-determined length. Such as:

>>> foo = lambda: [1, 2, 3, 4]
>>> x, y, z = foo()
Traceback (most recent call last):
  ...
ValueError: too many values to unpack (expected 3, got 4)

This is my first time trying to modify C code in Python, so let me know if I'm doing something obviously wrong.

• Issue: Improve error message for unbalanced unpacking #122239

[encukou]

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.

[tusharsadhwani]

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

[tusharsadhwani]

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?

[encukou]

Sorry if it seems like I'm adding more work or making bad suggestions! I don't know the right answer here.

Looking at the resulting traceback, it doesn't seem correct:

Traceback (most recent call last):
  File "/tmp/rep.py", line 3, in __len__
    raise RuntimeError
RuntimeError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/tmp/rep.py", line 6, in <module>
    x, y, z = CustomSeq()
    ^^^^^^^
ValueError: too many values to unpack (expected 3)

Perhaps they should be chained the other way around?

Traceback (most recent call last):
  File "/tmp/rep.py", line 6, in <module>
    x, y, z = CustomSeq()
    ^^^^^^^
ValueError: too many values to unpack (expected 3)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/tmp/rep.py", line 3, in __len__
    raise RuntimeError
RuntimeError

But then the error type is different...
... but for a KeyboardInterrupt, that's what you'd want!

So, my suggestion for a the next thing to try would be:

• Ignore Exception entirely, and chain other BaseExceptions so that the BaseException is raised

Or, if this rabbit hole is getting too deep:

• Limit this only to lists and tuples, whose __len__ can't fail

[tusharsadhwani]

Ignore Exception entirely, and chain other BaseExceptions so that the BaseException is raised

I'm leaning towards raising ValueError for Exception's (including original context), but for BaseException we ignore the original and just raise the BaseException.

Sorry if it seems like I'm adding more work

Absolutely not! I'm here for learning the process.

[tusharsadhwani]

I'm not sure if Py_SIZE is supposed to work with PyDict objects or not, but the test seems to work so I've kept that. If Dict is not supposed to be used with Py_SIZE, we could instead use PySequence_Fast_GET_SIZE as well.

[encukou]

Py_SIZE and PySequence_Fast_GET_SIZE should not be used on dicts. (But they unfortunately do no error checking and seem to work on common platforms.)

Please use PyDict_Size for dicts.

[blhsing]

Py_SIZE and PySequence_Fast_GET_SIZE should not be used on dicts. (But they unfortunately do no error checking and seem to work on common platforms.)

Please use PyDict_Size for dicts.

Please correct me if I'm wrong, but doesn't PyObject_Size, which I suggested back in this code review comment (and which was implemented with this commit), support both sequences and mappings (including dicts), and does error checking already?

[encukou]

Yes, PyObject_Size would work too.

[encukou]

I think this will be good to merge after the simplification I suggested.
@tusharsadhwani, do you want to push it over the finish line?

[tusharsadhwani]

@encukou Thanks for the reminder, I think that should be it.

[encukou]

Looks good to me! The scope was reduced a lot, but now it should not be controversial :)

[tusharsadhwani]

I don't think it needs one, but should I bring back the What's new in Python entry?

[encukou]

If you want it, go ahead. We can always remove it if it looks out of place -- for example if it ends up as the only entry in Improved error messages).

[pablogsal]

Congratulations on your contribution @tusharsadhwani 🚀 Great work ✨

Thanks a lot for your reviews @encukou @picnixz @blhsing