X Tutup
Skip to content

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

Merged
pablogsal merged 28 commits intopython:mainfrom
tusharsadhwani:argcnt-errmsg
Sep 10, 2024
Merged

gh-122239: Add actual count in unbalanced unpacking error message when possible#122244
pablogsal merged 28 commits intopython:mainfrom
tusharsadhwani:argcnt-errmsg

Conversation

@tusharsadhwani
Copy link
Contributor

@tusharsadhwani tusharsadhwani commented Jul 24, 2024 •
edited by bedevere-app bot
Loading

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.

@bedevere-app bedevere-app bot mentioned this pull request Jul 24, 2024
Closed
Eclips4
Eclips4 reviewed Jul 24, 2024
View reviewed changes
@tusharsadhwani @Eclips4
PEP7
272cc6a 
Co-authored-by: Kirill Podoprigora <kirill.bast9@mail.ru>
blhsing
blhsing suggested changes Jul 26, 2024
View reviewed changes
blhsing
blhsing suggested changes Jul 26, 2024
View reviewed changes
tusharsadhwani and others added 2 commits July 26, 2024 14:56
@tusharsadhwani @blhsing
Update Python/ceval.c
0c741e2 
Co-authored-by: blhsing <blhsing@gmail.com>
@tusharsadhwani
remove stale comment
0a2f9ba
encukou
encukou reviewed Jul 26, 2024
View reviewed changes
Python/ceval.c Outdated
argcnt);

ll = PyObject_Size(v);
if (ll < 0) {
Copy link
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
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
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?

@tusharsadhwani tusharsadhwani requested a review from encukou July 29, 2024 10:48
@encukou
Copy link
Member

encukou commented Aug 2, 2024

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
Copy link
Contributor Author

tusharsadhwani commented Aug 2, 2024 •
edited
Loading

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
Copy link
Contributor Author

tusharsadhwani commented Aug 28, 2024

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.

@tusharsadhwani tusharsadhwani requested a review from encukou August 28, 2024 10:51
@encukou
Copy link
Member

encukou commented Aug 29, 2024

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
Copy link
Contributor

blhsing commented Aug 30, 2024

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
encukou reviewed Aug 30, 2024
View reviewed changes
@encukou
Copy link
Member

encukou commented Aug 30, 2024

Yes, PyObject_Size would work too.

blhsing
blhsing reviewed Aug 30, 2024
View reviewed changes
@encukou
Copy link
Member

encukou commented Sep 4, 2024

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
Copy link
Contributor Author

tusharsadhwani commented Sep 4, 2024

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

picnixz
picnixz reviewed Sep 4, 2024
View reviewed changes
tusharsadhwani and others added 2 commits September 4, 2024 17:29
@tusharsadhwani @picnixz
Apply suggestions from code review
f183de8 
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
@tusharsadhwani
indentation fix
1b4c21b
encukou
encukou approved these changes Sep 9, 2024
View reviewed changes
Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

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

@tusharsadhwani
Copy link
Contributor Author

tusharsadhwani commented Sep 9, 2024

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

@encukou
Copy link
Member

encukou commented Sep 10, 2024

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 pablogsal merged commit 3597642 into python:main Sep 10, 2024
@pablogsal
Copy link
Member

pablogsal commented Sep 10, 2024

Congratulations on your contribution @tusharsadhwani 🚀 Great work ✨

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

@picnixz picnixz mentioned this pull request Jan 10, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz left review comments

@Eclips4 Eclips4 Eclips4 left review comments

@encukou encukou encukou approved these changes

@pablogsal pablogsal pablogsal approved these changes

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

@iritkatriel iritkatriel Awaiting requested review from iritkatriel

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently

+1 more reviewer

@blhsing blhsing blhsing requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tusharsadhwani @encukou @pablogsal @picnixz @blhsing @Eclips4
X Tutup