◐ Shell
reader mode source ↗
Skip to content

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

Merged
serhiy-storchaka merged 8 commits into
python:mainfrom
geryogam:patch-27
Jan 18, 2022
Merged

Conversation

@geryogam

@geryogam geryogam commented May 3, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

@geryogam geryogam changed the title bpo-44024: Use common TypeError message for built-in functions getattr and hasattr May 4, 2021
@geryogam @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>
@geryogam geryogam changed the title bpo-44024: Use the common TypeError message for non-string second arguments passed to the built-in functions getattr and hasattr May 4, 2021
@geryogam

geryogam commented May 4, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for the review @JelleZijlstra!

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

github-actions Bot commented Jun 25, 2021

Copy link
Copy Markdown

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

merwok commented Jan 5, 2022

Copy link
Copy Markdown
Member

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

serhiy-storchaka
serhiy-storchaka reviewed Jan 5, 2022
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

@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 reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM.

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

Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@geryogam @merwok @JelleZijlstra @serhiy-storchaka @erlend-aasland @hugovk @the-knights-who-say-ni @bedevere-bot