Skip to content
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

bpo-43857: Improve the AttributeError message when deleting a missing attribute #25424

Merged
merged 13 commits into from May 5, 2022

Conversation

geryogam
Copy link
Contributor

@geryogam geryogam commented Apr 15, 2021

@github-actions
Copy link

github-actions bot commented Jun 3, 2021

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Jun 3, 2021
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

I agree with this idea. Could you add a NEWS entry and a unit test that exercises this message?

(I promise I'm not stalking you, I just opened page 24 of the open PRs and found this one :) .)

@geryogam geryogam changed the title bpo-43857: Fix the AttributeError message for deletion of a missing attribute bpo-43857: Improve the AttributeError message when deleting a missing attribute May 4, 2022
@JelleZijlstra JelleZijlstra self-assigned this May 4, 2022
@geryogam
Copy link
Contributor Author

geryogam commented May 4, 2022

Thanks @JelleZijlstra, but please don’t merge right now as some tests are failing. I am going to work on it in the next few hours. I’ll let you know when it works.

@geryogam
Copy link
Contributor Author

geryogam commented May 4, 2022

Not only objects but also type objects have inconsistent AttributeError messages between missing attribute retrieval and deletion:

$ python3
Python 3.9.12 (main, Mar 26 2022, 15:52:10) 
[Clang 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> class A: pass
... 
>>> A.x
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'A' has no attribute 'x'
>>> del A.x
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: x
>>> A().x
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'A' object has no attribute 'x'
>>> del A().x
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: x

Compare that with PyPy for which the messages are consistent (well almost, 'type' object has no attribute 'x' should be type object 'A' has no attribute 'x' for del A.x):

$ pypy3
Python 3.7.13 (7e0ae751533460d5f89f3ac48ce366d8642d1db5, Apr 26 2022, 09:31:20)
[PyPy 7.3.9 with GCC Apple LLVM 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>>> class A: pass
>>>> 
>>>> A.x
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'A' has no attribute 'x'
>>>> del A.x
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'type' object has no attribute 'x'
>>>> A().x
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'A' object has no attribute 'x'
>>>> del A().x
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'A' object has no attribute 'x'

Following @markshannon’s PR #28802 for Python 3.11, the message fix of this line in Objects/object.c provided at the early stage of this PR no longer works for objects but only for type objects:

      if (res < 0 && PyErr_ExceptionMatches(PyExc_KeyError))
-         PyErr_SetObject(PyExc_AttributeError, name);
+         PyErr_Format(PyExc_AttributeError,
+                      "'%.100s' object has no attribute '%U'",
+                      tp->tp_name, name);

To fix the message for objects, we should also update this line in Objects/dictobject.c:

-             PyErr_SetObject(PyExc_AttributeError, name);
+             PyErr_Format(PyExc_AttributeError,
+                          "'%.100s' object has no attribute '%U'",
+                          Py_TYPE(obj)->tp_name, name);

That way we get the same messages as in PyPy.

Finally to solve the last message inconsistency ('type' object has no attribute 'x' should be type object 'A' has no attribute 'x' for del A.x), I replaced the last fix in Objects/object.c with this fix:

-     if (res < 0 && PyErr_ExceptionMatches(PyExc_KeyError))
-         PyErr_SetObject(PyExc_AttributeError, name);
+     if (res < 0 && PyErr_ExceptionMatches(PyExc_KeyError)) {
+         if (PyType_IsSubtype(tp, &PyType_Type)) {
+             PyErr_Format(PyExc_AttributeError,
+                          "type object '%.50s' has no attribute '%U'",
+                          ((PyTypeObject*)obj)->tp_name, name);
+         }
+         else {
+             PyErr_Format(PyExc_AttributeError,
+                          "'%.100s' object has no attribute '%U'",
+                          tp->tp_name, name);
+         }
+     }

@geryogam
Copy link
Contributor Author

geryogam commented May 4, 2022

@JelleZijlstra Done, all tests are passing. The PR is ready.

@JelleZijlstra JelleZijlstra merged commit a95138b into python:main May 5, 2022
@geryogam geryogam deleted the patch-24 branch May 5, 2022 14:04
@geryogam
Copy link
Contributor Author

geryogam commented May 5, 2022

Thanks again @JelleZijlstra!

(I promise I'm not stalking you, I just opened page 24 of the open PRs and found this one :) .)

No worries. And even if you were stalking my PRs I would not complain, quite the opposite =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale PR or inactive for long period of time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants