bpo-32225: Implementation of PEP 562 #4731

ilevkivskyi · 2017-12-05T20:24:36Z

This is a basic implementation of the PEP. Suggestions are welcome.

@gvanrossum @serhiy-storchaka Can one of you (or both?) please review?

https://bugs.python.org/issue32225

serhiy-storchaka

Add a What's New entry and document this feature in corresponding places.

serhiy-storchaka · 2017-12-05T21:43:27Z

Objects/moduleobject.c

+            }
+            return _PyObject_FastCall(getattr, stack, 1);
+        }
+        else if (PyErr_Occurred()) {


This never happen with _PyDict_GetItemId().

I prefer to use _PyDict_GetItemIdWithError() and don't silence errors, but this is a different large issue.

serhiy-storchaka · 2017-12-05T22:01:56Z

Objects/moduleobject.c

+                                "module __getattr__ must be callable");
+                return NULL;
+            }
+            return _PyObject_FastCall(getattr, stack, 1);


Could you please test the following case?

def __getattr__(name): if name != 'delgetattr': raise AttributeError del globals()['__getattr__'] # do something more

I suspect that it can crash (but not sure).

Thanks for review! I think I addressed all the comments, but there is a little problem. This test that I added as test_module_getattr_tricky fails with AttributeError in refleak hunting mode -R 3:3 (it passes in normal mode). Presumably this is because the module stays in memory between sequential runs and __getattr__ is already deleted after the first test. How do I fix this?

If the test didn't crash then perhaps we shouldn't add it.

I think it makes sense to keep something like this just in case. I simplified the test (__getattr__ always raises AttributeError, but will also delete itself). This way it works also in refleak hunting mode.

serhiy-storchaka · 2017-12-05T22:05:26Z

Objects/moduleobject.c

+                    result = NULL;
+                }
+                else {
+                    result = _PyObject_FastCall(dirfunc, NULL, 0);


_PyObject_CallNoArg()

serhiy-storchaka · 2017-12-05T22:10:26Z

Objects/moduleobject.c

+        if (PyDict_Check(dict)) {
+            PyObject *dirfunc = PyDict_GetItemString(dict, "__dir__");
+            if (dirfunc) {
+                if (!PyCallable_Check(dirfunc)) {


There is no a special check for the callability of a regular __dir__.

serhiy-storchaka · 2017-12-05T22:15:41Z

Lib/test/test_module.py

+        self.assertEqual(test, "There is test")
+        self.assertEqual(gga.x, 1)
+        self.assertEqual(gga.y, 2)
+        with self.assertRaises(AttributeError):


Test the error message.

merwok · 2017-12-07T16:49:27Z

Misc/NEWS.d/next/Core and Builtins/2017-12-05-21-33-47.bpo-32225.ucKjvw.rst

@@ -0,0 +1,2 @@
+PEP 562: Add support for module __getattr__ and __dir__. Implemented by Ivan
+Levkivskyi.


Escape the underscores to make this valid reST:

module ``__getattr__`` and ``__dir__``

Thanks, fixed!

ilevkivskyi · 2017-12-08T01:17:18Z

@serhiy-storchaka @merwok is something left to do here?

csabella · 2017-12-10T13:47:12Z

@ilevkivskyi Nick made a comment on the ticket about adding documentation. @serhiy-storchaka and @merwok would need to let you know if anything else was needed.

serhiy-storchaka · 2017-12-10T14:31:17Z

Objects/moduleobject.c

+        _Py_IDENTIFIER(__getattr__);
+        getattr = _PyDict_GetItemId(m->md_dict, &PyId___getattr__);
+        if (getattr) {
+            if (!PyCallable_Check(getattr)) {


There is no a special check for callability of a regular __getattr__ (as well as other special methods).

>>> class A: ... __getattr__ = "Surprise!" ... >>> A().x Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: 'str' object is not callable >>> A()[1] Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: 'str' object is not callable >>> A() + 1 Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: 'str' object is not callable

Of course special checks can be added in all these cases. This would help recognition marginal mistakes. But this will increase the size of the code and can slow down it (because optimizing this code can worsen the optimization of the normal path) and decrease readability. I would defer this change to a separate issue if it is worth to do it at all.

serhiy-storchaka · 2017-12-10T14:47:56Z

The documentation is required. @ncoghlan given a good suggestion.

ilevkivskyi · 2017-12-10T20:54:23Z

OK, I added docs as proposed by @ncoghlan (I also mentioned __class__).

serhiy-storchaka · 2017-12-10T21:29:59Z

Doc/reference/datamodel.rst

   returned. :func:`dir` converts the returned sequence to a list and sorts it.


+Customizing module attribute access


Add the index entries:

.. index:: single: __getattr__ (module attribute) single: __dir__ (module attribute) single: __class__ (module attribute)

serhiy-storchaka · 2017-12-10T21:39:27Z

Doc/reference/datamodel.rst

+computed value or raise an :exc:`AttributeError`. If an attribute is
+not found on a module object through the normal lookup, i.e.
+:meth:`object.__getattribute__`, then ``__getattr__`` is searched in
+the module ``__dict__`` before raising an :exc:`AttributeError`. If found,


Would be nice to make a reference to __dict__ as the module attribute (but not to the object attribute) if possible.

I am not sure if it is possible. The sub-section "Modules" itself refers to object.__dict__.

There is an index entry for __dict__ as the module attribute. But I don't know if it is possible to refer to the target of an index entry.

I believe you can you use :data: directive. Here's the definition in the Documentation Guide:

data Describes global data in a module, including both variables and values used as “defined constants.” Class and object attributes are not documented using this directive.

Where the __dict__ is defined:

.. index:: single: __dict__ (module attribute) .. data:: __dict__ Special read-only attribute: :attr:`~object.__dict__` is the module's namespace as a dictionary object.

and then where you want to link:

:meth:`object.__getattribute__`, then ``__getattr__`` is searched in the module :data:`__dict__` before raising an :exc:`AttributeError`.

As you mentioned, this may require fixing other links for module.__dict__ that now point to object.__dict__.

I'm not sure this is applicable here. In any case we can experiment with this in a separate issue. I prefer to keep the current markup for now.

In any case we can experiment with this in a separate issue. I prefer to keep the current markup for now.

I also think it can be deferred to a separate issue. IIUC the only thing remaining here is to get an OK from @ncoghlan

serhiy-storchaka · 2017-12-10T21:47:37Z

And please document PEP 562 in the What's New document.

serhiy-storchaka

LGTM. But perhaps @ncoghlan has other suggestions about the documentation.

Thanks, this is a nice improvement.

serhiy-storchaka · 2017-12-10T23:32:05Z

Doc/reference/datamodel.rst

+computed value or raise an :exc:`AttributeError`. If an attribute is
+not found on a module object through the normal lookup, i.e.
+:meth:`object.__getattribute__`, then ``__getattr__`` is searched in
+the module ``__dict__`` before raising an :exc:`AttributeError`. If found,


There is an index entry for __dict__ as the module attribute. But I don't know if it is possible to refer to the target of an index entry.

ncoghlan · 2017-12-14T05:58:46Z

+1 to merging from me (I haven't actually reviewed the docs updates yet, but I trust the reviews you've already received, and if I turn out to have comments after I get back from holidays we can always make further adjustments later)

ilevkivskyi · 2017-12-14T10:56:52Z

+1 to merging from me

OK, then I am going to merge this now.

gvanrossum · 2017-12-14T16:24:22Z

Whee!

ilevkivskyi added 6 commits September 9, 2017 22:37

Implement module __getattr__

119edbe

Add tests

c2ec3c1

Clean-up after tests

f657a7e

Fix test clean-up in refleak hunting mode

8aa052f

Add module __dir__ (with tests)

32d092f

Merge remote-tracking branch 'upstream/master' into module-getattr

68703f0

bedevere-bot added the awaiting review label Dec 5, 2017

the-knights-who-say-ni added the CLA signed label Dec 5, 2017

ilevkivskyi changed the title ~~[skip-issue] [skip-news] Implementation of PEP 562~~ [skip-news] bpo-32225: Implementation of PEP 562 Dec 5, 2017

Add news entry

4fa4b1b

ilevkivskyi changed the title ~~[skip-news] bpo-32225: Implementation of PEP 562~~ bpo-32225: Implementation of PEP 562 Dec 5, 2017

serhiy-storchaka reviewed Dec 5, 2017

View reviewed changes

serhiy-storchaka added the type-feature A feature request or enhancement label Dec 5, 2017

ilevkivskyi added 3 commits December 7, 2017 01:06

Address CR

d1ada11

Fix whitespace

a9f7d4a

Simplify the tricky test

f93b168

merwok reviewed Dec 7, 2017

View reviewed changes

Fix rst formatting

c720cfd

serhiy-storchaka reviewed Dec 10, 2017

View reviewed changes

Add docs; minor comment by Serhiy

fd2f02f

ilevkivskyi mentioned this pull request Dec 10, 2017

bpo-32226: Implementation of PEP 560 (core components) #4732

Merged

serhiy-storchaka reviewed Dec 10, 2017

View reviewed changes

Add what's new entry, and index items

e4b53d9

serhiy-storchaka approved these changes Dec 10, 2017

View reviewed changes

bedevere-bot removed the awaiting review label Dec 10, 2017

bedevere-bot added the awaiting merge label Dec 10, 2017

serhiy-storchaka requested a review from ncoghlan December 10, 2017 23:35

ilevkivskyi merged commit 5364b5c into python:master Dec 14, 2017

bedevere-bot removed the awaiting merge label Dec 14, 2017

ilevkivskyi deleted the module-getattr branch December 14, 2017 10:59

skirpichev mentioned this pull request Aug 22, 2023

gh-106016: Support customizing of module attributes access with __setattr__/__delattr__ (PEP 726) #108261

Closed

		@@ -0,0 +1,2 @@
		PEP 562: Add support for module __getattr__ and __dir__. Implemented by Ivan
		Levkivskyi.

		returned. :func:`dir` converts the returned sequence to a list and sorts it.


		Customizing module attribute access

Uh oh!

bpo-32225: Implementation of PEP 562 #4731

bpo-32225: Implementation of PEP 562 #4731

Uh oh!

Conversation

ilevkivskyi commented Dec 5, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Dec 8, 2017

Uh oh!

csabella commented Dec 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Dec 10, 2017

Uh oh!

ilevkivskyi commented Dec 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Dec 10, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncoghlan commented Dec 14, 2017 via email

Uh oh!

ilevkivskyi commented Dec 14, 2017

Uh oh!

gvanrossum commented Dec 14, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ilevkivskyi commented Dec 5, 2017 •

edited by bedevere-bot

Loading