bpo-29822: make inspect.isabstract() work during __init_subclass__ #678

Soares · 2017-03-15T18:50:04Z

At the time when an abstract base class' __init_subclass__ runs, ABCMeta.__new__ has not yet finished running, so in the presence of __init_subclass__, inspect.isabstract() can no longer depend only on TPFLAGS_IS_ABSTRACT.

At the time when an abstract base class' __init_subclass__ runs, ABCMeta.__new__ has not yet finished running, so in the presence of __init_subclass__, inspect.isabstract() can no longer depend only on TPFLAGS_IS_ABSTRACT.

mention-bot · 2017-03-15T18:50:08Z

@Soares, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zestyping, @1st1 and @larryhastings to be potential reviewers.

serhiy-storchaka · 2017-03-18T05:45:10Z

Lib/inspect.py

 def isabstract(object):
    """Return true if the object is an abstract base class (ABC)."""
-    return bool(isinstance(object, type) and object.__flags__ & TPFLAGS_IS_ABSTRACT)
+    if isinstance(object, type):


if isinstance(object, type): return False

This will decrease an indentation level.

serhiy-storchaka · 2017-03-18T05:47:10Z

Lib/inspect.py

 def isabstract(object):
    """Return true if the object is an abstract base class (ABC)."""
-    return bool(isinstance(object, type) and object.__flags__ & TPFLAGS_IS_ABSTRACT)
+    if isinstance(object, type):


if not isinstance(object, type): return False

This will decrease an indentation level.

serhiy-storchaka · 2017-03-18T05:54:41Z

Lib/inspect.py

+    if isinstance(object, type):
+        if object.__flags__ & TPFLAGS_IS_ABSTRACT:
+            return True
+        elif (issubclass(type(object), abc.ABCMeta)


Just if rather than elif. And you can return False if the condition is false for decreasing an indentation level.

serhiy-storchaka · 2017-03-18T06:00:11Z

Lib/inspect.py

+            # We're likely in the __init_subclass__ of an ABC. ABCMeta.__new__
+            # hasn't finished running yet. We have to search for abstractmethods
+            # by hand. Code copied from ABCMeta.__new__.
+            abstracts = {name


It seems to me that collecting names of abstract method is not needed.
False can be returned just after finding any abstract method.

- return earlier to decrease indentation levels - return immediately upon finding an abstractmethod, instead of collecting the whole set

serhiy-storchaka · 2017-03-18T07:43:15Z

Lib/inspect.py

+        if getattr(value, "__isabstractmethod__", False):
+            return True
+    for base in object.__bases__:
+        for name in getattr(base, "__abstractmethods__", frozenset()):


You can use an empty tuple () rather than frozenset(). This is shorter and faster.

serhiy-storchaka · 2017-03-18T07:51:12Z

Lib/test/test_inspect.py

+            def foo(self):
+                pass
+
+        class ClassExample(AbstractClassExample):


The test doesn't cover the case when the abstract method is not in the __dict__ of final class, but is inherited from base class.

serhiy-storchaka · 2017-03-18T08:03:57Z

Misc/NEWS

 - Issue #29581: ABCMeta.__new__ now accepts **kwargs, allowing abstract base
  classes to use keyword parameters in __init_subclass__. Patch by Nate Soares.

+- Issue #29822: inspect.isabstract() now works during __init_subclass__. Patch


Move this at the start of the Library section. Use two spaces between sentences.

serhiy-storchaka · 2017-03-18T08:08:35Z

Lib/test/test_inspect.py


+    def test_isabstract_during_init_subclass(self):
+        from abc import ABCMeta, abstractmethod
+


Use blank lines in test method only to indicate logical sections.

- Added a new test. - Cleaned up some code style here and there

serhiy-storchaka · 2017-03-18T16:56:42Z

Lib/test/test_inspect.py

+        isabstract_checks.clear()
+        class AbstractChild(AbstractClassExample):
+            pass
+        class AbstractGrandchild(AbstractClassExample):


What is the difference between AbstractChild and AbstractGrandchild?

Soares · 2017-03-18T17:42:35Z

Oh, my bad, I meant to have AbstractGrandchild inherit AbstractChild. AbstractGrandchild is not strictly necessary, I suppose; I have it in there to make sure that we're walking the whole __mro__ instead of just the __bases__.

agoose77 · 2017-04-03T16:20:16Z

Lib/inspect.py

+        # TPFLAGS_IS_ABSTRACT should have been accurate.
+        return False
+    # It looks like ABCMeta.__new__ has not finished running yet; we're
+    # probably in __init_subcalss_. We'll look for abstractmethods manually.


Pedantic but typo in comment. Should be:

# probably in __init_subclass__. We'll look for abstractmethods manually.

Soares · 2017-04-04T17:42:47Z

Thanks for the review. Fixed the typo, and updated the NEWS file to resolve conflicts.

Soares · 2017-04-23T17:07:39Z

Bump. @agoose77, I have fixed the aforementioned typo, are there any outstanding issues as far as you're concerned?

serhiy-storchaka · 2017-04-23T18:18:35Z

Misc/NEWS


-What's New in Python 3.6.0 beta 1?
-==================================
+What's New in Python 3.6.0 beta ==============================


Eaten "1?" and a newline.

Fixed "1?\n====" that I accidentally killed during the resolution of a merge conflict.

Soares · 2017-04-23T18:23:11Z

My bad; thanks for catching that; should be fixed now.

serhiy-storchaka · 2017-04-23T19:49:12Z

Lib/inspect.py

+        # TPFLAGS_IS_ABSTRACT should have been accurate.
+        return False
+    # It looks like ABCMeta.__new__ has not finished running yet; we're
+    # probably in __init_subcalss__. We'll look for abstractmethods manually.


Typo: "__init_subcalss__".

Soares · 2017-04-23T20:03:37Z

Fixed thanks.

ncoghlan

Nice find, and the proposed fix and test case look good to me.

agoose77 · 2017-04-24T16:45:56Z

Thanks @Soares LGTM👍

serhiy-storchaka · 2017-04-24T17:07:37Z

Thank you for your contribution @Soares. Do you mind to backport the fix to 3.6?

Soares · 2017-04-24T21:02:15Z

I'd be happy to backport. Note that #527 will also need backporting (in Python 3.6, ABCMeta doesn't work with __init_subclass__ at all). IIRC, the developer guide says that that PR will need a "needs backport to 3.6" label that only core devs can apply; if that's correct, can you apply that label? Then I'll backport both PRs. Thanks!

…s__. (pythonGH-678) At the time when an abstract base class' __init_subclass__ runs, ABCMeta.__new__ has not yet finished running, so in the presence of __init_subclass__, inspect.isabstract() can no longer depend only on TPFLAGS_IS_ABSTRACT.. (cherry picked from commit fcfe80e)

Nate Soares added 2 commits March 15, 2017 11:48

fix inspect.isabstract() during __init_subclass__

a353492

At the time when an abstract base class' __init_subclass__ runs, ABCMeta.__new__ has not yet finished running, so in the presence of __init_subclass__, inspect.isabstract() can no longer depend only on TPFLAGS_IS_ABSTRACT.

Added NEWS item

ebbbf58

the-knights-who-say-ni added the CLA signed label Mar 15, 2017

serhiy-storchaka reviewed Mar 18, 2017

View reviewed changes

serhiy-storchaka requested a review from 1st1 March 18, 2017 06:09

integrate review comments

4ba6c4d

- return earlier to decrease indentation levels - return immediately upon finding an abstractmethod, instead of collecting the whole set

serhiy-storchaka reviewed Mar 18, 2017

View reviewed changes

Integrated more review comments.

b92b496

- Added a new test. - Cleaned up some code style here and there

serhiy-storchaka reviewed Mar 18, 2017

View reviewed changes

fixed typo in the tests

abf6861

serhiy-storchaka approved these changes Mar 18, 2017

View reviewed changes

agoose77 reviewed Apr 3, 2017

View reviewed changes

Nate Soares added 2 commits April 3, 2017 17:58

Typo fix

3582895

Merge branch 'master' into fix-issue-29822

6fe5f99

Merge branch 'master' into fix-issue-29822

70a0e62

serhiy-storchaka reviewed Apr 23, 2017

View reviewed changes

Typo fix

cd15f76

Fixed "1?\n====" that I accidentally killed during the resolution of a merge conflict.

serhiy-storchaka reviewed Apr 23, 2017

View reviewed changes

Typo fix in comment (calss -> class)

8a2b5d2

serhiy-storchaka requested a review from ncoghlan April 24, 2017 05:07

ncoghlan approved these changes Apr 24, 2017

View reviewed changes

serhiy-storchaka merged commit fcfe80e into python:master Apr 24, 2017

serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label Apr 24, 2017

serhiy-storchaka added the needs backport to 3.6 label Apr 24, 2017

This was referenced Apr 24, 2017

Backport bd583ef 3.6 #1277

Closed

[3.6] bpo-29581: Make __init_subclass__ work on ABCMeta type classes #1282

Merged

Soares mentioned this pull request Jun 7, 2017

[3.6] bpo-29822: make inspect.isabstract() work during __init_subclass__ #1979

Merged

Mariatta removed the needs backport to 3.6 label Jun 19, 2017


		def test_isabstract_during_init_subclass(self):
		from abc import ABCMeta, abstractmethod

Uh oh!

bpo-29822: make inspect.isabstract() work during __init_subclass__ #678

bpo-29822: make inspect.isabstract() work during __init_subclass__ #678

Uh oh!

Conversation

Soares commented Mar 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Mar 15, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Soares commented Mar 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Soares commented Apr 4, 2017

Uh oh!

Soares commented Apr 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Soares commented Apr 23, 2017

Uh oh!

serhiy-storchaka Apr 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Soares commented Apr 23, 2017

Uh oh!

ncoghlan left a comment

Choose a reason for hiding this comment

Uh oh!

agoose77 commented Apr 24, 2017

Uh oh!

serhiy-storchaka commented Apr 24, 2017

Uh oh!

Soares commented Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Soares commented Mar 15, 2017 •

edited

Loading

serhiy-storchaka Apr 23, 2017 •

edited

Loading

Soares commented Apr 24, 2017 •

edited

Loading