Skip to content

gh-112331: Fix reference manual description of attribute lookup mechanics #112375

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

Merged
merged 7 commits into from
Nov 25, 2023

Conversation

rhettinger
Copy link
Contributor

@rhettinger rhettinger commented Nov 24, 2023

Make the lookup mechanics description match the logic found in slot_tp_getattr_hook in `Objects/typeobject.c'.

Notably, mention of __getattribute__ was missing.


📚 Documentation preview 📚: https://cpython-previews--112375.org.readthedocs.build/

rhettinger and others added 3 commits November 25, 2023 00:25
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Looks like a clear improvement to me. Left one minor suggestion, and a question:

reference may yield different objects.

This production can be customized by overriding the
:meth:`~object.__getattribute__` method or the :meth:`~object.__getattr__`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:meth:`~object.__getattribute__` method or the :meth:`~object.__getattr__`
:meth:`~object.__getattribute__` method and/or the :meth:`~object.__getattr__`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think "or" is fine. The "and/or" is correct but looks weird and doesn't read well.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree that it looks weird, but won't insist

Comment on lines +832 to +834
method. The :meth:`!__getattribute__` method is called first and either
returns a value or raises :exc:`AttributeError` if the attribute is not
available.
Copy link
Member

Choose a reason for hiding this comment

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

Of course, it is possible to create badly behaved objects that raise other exceptions from __getattribute__:

>>> class Foo:
...     def __getattribute__(self, attr):
...         if attr == 'foo':
...             raise TypeError('no')
...         return object.__getattribute__(self, attr)
...
>>> f = Foo()
>>> f.a = 1
>>> f.a
1
>>> f.b
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
    f.b
  File "<stdin>", line 5, in __getattribute__
    return object.__getattribute__(self, attr)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AttributeError: 'Foo' object has no attribute 'b'
>>> f.foo
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
    f.foo
  File "<stdin>", line 4, in __getattribute__
    raise TypeError('no')
TypeError: no

Does that need to be mentioned here? Or is it too obscure?

Copy link
Contributor Author

@rhettinger rhettinger Nov 25, 2023

Choose a reason for hiding this comment

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

No need. Any code can raise a weird error at any time but we don't need to mention that in the docs in the thousands of places where this could occur:

This wording corresponds to what the code actually does:

def getattr_hook(obj, name):
    "Emulate slot_tp_getattr_hook() in Objects/typeobject.c"
    try:
        return obj.__getattribute__(name)
    except AttributeError:
        if not hasattr(type(obj), '__getattr__'):
            raise
    return type(obj).__getattr__(obj, name)

reference may yield different objects.

This production can be customized by overriding the
:meth:`~object.__getattribute__` method or the :meth:`~object.__getattr__`
Copy link
Member

Choose a reason for hiding this comment

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

I disagree that it looks weird, but won't insist

@rhettinger rhettinger merged commit 97f8f28 into python:main Nov 25, 2023
@miss-islington-app
Copy link

Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 25, 2023
… mechanics (pythongh-112375)

(cherry picked from commit 97f8f28)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Nov 25, 2023

GH-112412 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Nov 25, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 25, 2023
… mechanics (pythongh-112375)

(cherry picked from commit 97f8f28)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
@bedevere-app
Copy link

bedevere-app bot commented Nov 25, 2023

GH-112413 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Nov 25, 2023
@rhettinger rhettinger deleted the ref_attribute_reference branch November 25, 2023 22:22
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants