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

Jump to bottom

bpo-44024: Improve the TypeError message for non-string second arguments passed to the built-in functions getattr and hasattr #25863

Merged
merged 8 commits into from Jan 18, 2022
Merged

bpo-44024: Improve the TypeError message for non-string second arguments passed to the built-in functions getattr and hasattr #25863

merged 8 commits into from Jan 18, 2022

Conversation

maggyero
Copy link
Contributor

@maggyero maggyero commented May 3, 2021 •
edited

Problem

Actual behaviour:

>>> getattr('foobar', 123)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: getattr(): attribute name must be string
>>> hasattr('foobar', 123)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: hasattr(): attribute name must be string

Expected behaviour:

>>> getattr('foobar', 123)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: attribute name must be string, not 'int'
>>> hasattr('foobar', 123)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: attribute name must be string, not 'int'

Motivation:

>>> setattr('foobar', 123, 'baz')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: attribute name must be string, not 'int'
>>> delattr('foobar', 123)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: attribute name must be string, not 'int'

Solution

In the function builtin_getattr defined in Python/bltinmodule.c, we remove the following lines:

    if (!PyUnicode_Check(name)) {
        PyErr_SetString(PyExc_TypeError,
                        "getattr(): attribute name must be string");
        return NULL;
    }

because the expected TypeError message is already implemented in the subsequent call to the functions _PyObject_LookupAttr and PyObject_GetAttr defined in Objects/object.c:

PyObject *
PyObject_GetAttr(PyObject *v, PyObject *name)
{
    PyTypeObject *tp = Py_TYPE(v);
    if (!PyUnicode_Check(name)) {
        PyErr_Format(PyExc_TypeError,
                     "attribute name must be string, not '%.200s'",
                     Py_TYPE(name)->tp_name);
        return NULL;
    }

[…]

int
_PyObject_LookupAttr(PyObject *v, PyObject *name, PyObject **result)
{
    PyTypeObject *tp = Py_TYPE(v);

    if (!PyUnicode_Check(name)) {
        PyErr_Format(PyExc_TypeError,
                     "attribute name must be string, not '%.200s'",
                     Py_TYPE(name)->tp_name);
        *result = NULL;
        return -1;
    }

[…]

Likewise, in the function builtin_hasattr_impl defined in Python/bltinmodule.c, we remove the following lines:

    if (!PyUnicode_Check(name)) {
        PyErr_SetString(PyExc_TypeError,
                        "hasattr(): attribute name must be string");
        return NULL;
    }

because the expected TypeError message is already implemented in the subsequent call to the function _PyObject_LookupAttr defined in Objects/object.c.

https://bugs.python.org/issue44024

JelleZijlstra
JelleZijlstra reviewed May 4, 2021
View changes
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Could you add a test for the error message and a news entry for the change?

@maggyero maggyero changed the title bpo-44024: Use common TypeError message for built-in functions getattr and hasattr bpo-44024: Use the common TypeError message for non-string second arguments passed to the built-in functions getattr and hasattr May 4, 2021
JelleZijlstra
JelleZijlstra reviewed May 4, 2021
View changes
Lib/test/test_call.py Outdated Show resolved Hide resolved
@maggyero @JelleZijlstra
Update Misc/NEWS.d/next/Core and Builtins/2021-05-04-21-55-49.bpo-440…
46c3a0f 
…24.M9m8Qd.rst

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@maggyero maggyero changed the title bpo-44024: Use the common TypeError message for non-string second arguments passed to the built-in functions getattr and hasattr bpo-44024: Improve the error message for non-string second arguments passed to the built-in functions getattr and hasattr May 4, 2021
@maggyero
Copy link
Contributor Author

maggyero commented May 4, 2021

Thanks for the review @JelleZijlstra!

@maggyero maggyero changed the title bpo-44024: Improve the error message for non-string second arguments passed to the built-in functions getattr and hasattr bpo-44024: Improve the TypeError message for non-string second arguments passed to the built-in functions getattr and hasattr May 4, 2021
@github-actions
Copy link

github-actions bot commented Jun 25, 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 25, 2021
@merwok
Copy link
Member

merwok commented Jan 5, 2022

Looks good, thanks! Could you pull and merge upstream in your branch?

serhiy-storchaka
serhiy-storchaka reviewed Jan 5, 2022
View changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

The change LGTM, but tests are added in wrong place.

Lib/test/test_call.py Outdated Show resolved Hide resolved
@hugovk hugovk removed the stale Stale PR or inactive for long period of time. label Jan 5, 2022
serhiy-storchaka
serhiy-storchaka approved these changes Jan 10, 2022
View changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

LGTM.

@serhiy-storchaka serhiy-storchaka merged commit 16bf9bd into python:main Jan 18, 2022
12 checks passed
@maggyero maggyero deleted the patch-27 branch Jan 18, 2022
maggyero added a commit to maggyero/cpython that referenced this pull request May 4, 2022
@maggyero
Remove duplicate test (see python#25863)
42da011
@maggyero maggyero mentioned this pull request May 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erlend-aasland erlend-aasland

@JelleZijlstra JelleZijlstra

@serhiy-storchaka serhiy-storchaka

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@maggyero @merwok @JelleZijlstra @serhiy-storchaka @erlend-aasland @hugovk @the-knights-who-say-ni @bedevere-bot