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

Fix 7190:__init_subclass__ is not type-checked #7452

Merged
merged 31 commits into from Sep 21, 2019

Conversation

@gantsevdenis
Copy link
Contributor

gantsevdenis commented Sep 3, 2019

Fixes #7190

@gantsevdenis gantsevdenis changed the title Fix 7190 Fix 7190:__init_subclass__ is not type-checked Sep 3, 2019
Copy link
Collaborator

ilevkivskyi left a comment

Thank you for PR! I like this approach. I left few minor comments, could you also please move the logic to a separate method? (Like e.g. check_protocol_variance() below.)

mypy/checker.py Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved
@@ -1647,6 +1647,24 @@ def visit_class_def(self, defn: ClassDef) -> None:
with self.scope.push_class(defn.info):
self.accept(defn.defs)
self.binder = old_binder
for base in typ.mro[1:]:
if base.name() != 'object' and base.defn.info:

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 4, 2019

Collaborator

Checking for base.defn.info looks suspicious, at this stage it must be always present, if it isn't, it is a bug elsewhere. Is this actually needed?

This comment has been minimized.

Copy link
@gantsevdenis

gantsevdenis Sep 4, 2019

Author Contributor

actually there are AssertionErrors during tests, but I didn't succeed to reproduce them myself..

mypy/checker.py Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved

class object:
def __init__(self) -> None: pass
def __init_subclass__(cls, **kwargs) -> None: pass

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 4, 2019

Collaborator

This doesn't match typeshed, see https://github.com/python/typeshed/blob/master/stdlib/2and3/builtins.pyi. I would prefer to keep such subtle things in fixtures closer to the truth.

Copy link
Collaborator

ilevkivskyi left a comment

Thanks for updates! I have few more comments, mostly just minor style tweak.

7 Child()
Base.__init_subclass__(thing=5) is called at line 4. This is what we simulate here
Child.__init_subclass__ is never called

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 5, 2019

Collaborator

Thanks for adding the docstring! Normally we use this style:

"""Short one line summary.

The rest of the description after empty line.
"""

The summary in your case may be Check that keywords in a class definition are valid arguments for __init_subclass__().

# 'object.__init_subclass__ is a dummy method with no arguments, always defined
# there is no use to call it
if base.name() != 'object' \
and base.defn.info: # there are "NOT_READY" instances

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 5, 2019

Collaborator

This looks dangerous. Could you please post examples of tests that failed and the traceback?

This comment has been minimized.

Copy link
@gantsevdenis

gantsevdenis Sep 6, 2019

Author Contributor

I have pasted traceback below (quite long though)

# we skip the current class itself
for base in typ.mro[1:]:
# 'object.__init_subclass__ is a dummy method with no arguments, always defined
# there is no use to call it

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 5, 2019

Collaborator

In some sense it still makes sense to check this, for example:

>>> class C(test=True): ...
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __init_subclass__() takes no keyword arguments

This comment has been minimized.

Copy link
@gantsevdenis

gantsevdenis Sep 6, 2019

Author Contributor

You are right. I made the changes accordingly

call_expr.line = defn.line
call_expr.column = defn.column
call_expr.end_line = defn.end_line
self.expr_checker.accept(call_expr,

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 5, 2019

Collaborator

Using accept() may actually be dangerous. For example if the base class is not in scope (defined in another module and not imported in the current one), this may trigger Name not defined or maybe even a crash. There are two possible solutions here:

  • Point name_expr.node to the base class TypeInfo before calling accept()
  • Use check_call() or similar methods

Please add a test for this.

This comment has been minimized.

Copy link
@gantsevdenis

gantsevdenis Sep 6, 2019

Author Contributor

Hm sorry, I don't understand. I tried to produce a mypy runtime exception with 2 modules, one of which defines a Base, and the other one doesn't import that Base, and it only gave me a nice error Name 'Base' is not defined. Which os OK I guess? this is what would happen at runtime anyway (NameError: name 'Base' is not defined)

I added a test for that

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 11, 2019

Collaborator

Which os OK I guess?

It is not what I wanted. I wanted something like this:

# file bases.py
class Base:
    def __init_subclass__(cls, **kwargs) -> None: ...
class MidBase(Base):
    ...
# file main.py
from bases import MidBase
class Main(MidBase, test=False):
    ...
mypy/checker.py Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved
cls.default_name = default_name
return


This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 5, 2019

Collaborator

I think it is better to have only one empty line here and below.

gantsevdenis and others added 9 commits Sep 5, 2019
Co-Authored-By: Ivan Levkivskyi <levkivskyi@gmail.com>
Co-Authored-By: Ivan Levkivskyi <levkivskyi@gmail.com>
Co-Authored-By: Ivan Levkivskyi <levkivskyi@gmail.com>
Co-Authored-By: Ivan Levkivskyi <levkivskyi@gmail.com>
Co-Authored-By: Ivan Levkivskyi <levkivskyi@gmail.com>
Copy link
Collaborator

ilevkivskyi left a comment

This is almost ready, here are some more comments.

for base in typ.mro[1:]:
# there are "NOT_READY" instances
# during the tests, so I filter them out...
if base.defn.info:

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 11, 2019

Collaborator

I think I have an idea why this may be missing, but anyway this is probably not important since you don't need this anyway, everywhere where you use base.defn.info you can use just base, this is the same object.

This comment has been minimized.

Copy link
@gantsevdenis

gantsevdenis Sep 15, 2019

Author Contributor

understood!

# there are "NOT_READY" instances
# during the tests, so I filter them out...
if base.defn.info:
for method_name, method_symbol_node in base.defn.info.names.items():

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 11, 2019

Collaborator

You don't need this to be a cycle, just use method_symbol_node = base.names.get('__init_subclass__').

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 11, 2019

Collaborator

Or even simpler (if you are not going to use method_symbol_node):

if '__init_subclass__' not in base.names:
    continue

This comment has been minimized.

Copy link
@gantsevdenis

gantsevdenis Sep 15, 2019

Author Contributor

you are right, I have modified accordingly

call_expr.line = defn.line
call_expr.column = defn.column
call_expr.end_line = defn.end_line
self.expr_checker.accept(call_expr,

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 11, 2019

Collaborator

Which os OK I guess?

It is not what I wanted. I wanted something like this:

# file bases.py
class Base:
    def __init_subclass__(cls, **kwargs) -> None: ...
class MidBase(Base):
    ...
# file main.py
from bases import MidBase
class Main(MidBase, test=False):
    ...
@gantsevdenis gantsevdenis force-pushed the gantsevdenis:fix_7190 branch 2 times, most recently from a6c5089 to 2281a86 Sep 15, 2019
@gantsevdenis gantsevdenis force-pushed the gantsevdenis:fix_7190 branch from 2fc04f3 to 2281a86 Sep 15, 2019
@gantsevdenis

This comment has been minimized.

Copy link
Contributor Author

gantsevdenis commented Sep 15, 2019

@ilevkivskyi Maybe it's better if I open another PR, with a clean history? this one starts being messy.. i am sorry about that

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Sep 17, 2019

Maybe it's better if I open another PR, with a clean history? this one starts being messy.. i am sorry about that

We squash commits anyway, so this doesn't really matter. I will take a look at this later today.

Copy link
Collaborator

ilevkivskyi left a comment

Thanks, looks good! I can merge this after you will consider few style comments and undo the typeshed pin move.

@@ -1699,6 +1700,51 @@ def visit_class_def(self, defn: ClassDef) -> None:
if typ.is_protocol and typ.defn.type_vars:
self.check_protocol_variance(defn)

def check_init_subclass(self, defn: ClassDef) -> None:
"""
Check that keywords in a class definition are valid arguments for __init_subclass__().

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Sep 17, 2019

Collaborator

Move this to the line above, right after quotes. Essentially we just follow https://www.python.org/dev/peps/pep-0257/

mypy/checker.py Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved
gantsevdenis and others added 4 commits Sep 21, 2019
Co-Authored-By: Ivan Levkivskyi <levkivskyi@gmail.com>
Co-Authored-By: Ivan Levkivskyi <levkivskyi@gmail.com>
@gantsevdenis

This comment has been minimized.

Copy link
Contributor Author

gantsevdenis commented Sep 21, 2019

thank you very much for helping

@ilevkivskyi ilevkivskyi merged commit 1f11538 into python:master Sep 21, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@srittau

This comment has been minimized.

Copy link
Contributor

srittau commented Sep 23, 2019

This PR seems to have broken typeshed tests: https://travis-ci.org/python/typeshed/jobs/588255463 Seems an easy enough fix, but I wonder why this passes in this repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.