Make error messages from multiple inheritance compatibility check more accurate by mkurnikov · Pull Request #5926 · python/mypy

mkurnikov · 2018-11-20T16:02:38Z

Initially, in discussion with @ilevkivskyi in Gitter, he suggested to just remove error, if there are two nested classes in a multiple inheritance with the same name

class Mixin1:
    class Meta:
        pass
class Mixin2:
    class Meta:
        pass
class A(Mixin1, Mixin2):
    pass

However, later we decided to make it safe and emit a better error message, including for cases with nested class and non-class for obvious cases. Note that for class objects we ignore the __init__ method signature of nested class and only check subclassing relationship between them.

mthuurne · 2018-11-21T18:57:53Z

This indeed solves the issue I had in #5914, over 50 fewer false positives in the output now. Thanks!

ilevkivskyi

Thanks for a PR!

Here are few comments.

ilevkivskyi · 2018-11-26T01:01:34Z

-        if second_type is None and isinstance(second.node, FuncBase):
-            second_type = self.function_type(second.node)
+        if isinstance(first.node, TypeInfo) and isinstance(second.node, TypeInfo):
+            # allow nested classes with the same name


In general I agree it is better to admit this little unsafety and fix a common pain point.

Please add a more detailed comment with motivation/trade-offs. Here are some points to mention:

This is technically unsafe due to possible errors when accessing members of the nested classes.

But this creates many false positives for Django.

Also mypy anyway doesn't flag this for single inheritance.

A potential solution may be to require at least structural compatibility (since requiring nominal one is impractical).

mkurnikov · 2018-11-28T12:52:38Z

@ilevkivskyi I addressed your comments.

mthuurne · 2018-11-30T03:29:28Z

When checking the test case from #5914, I get one error message reported when using c039095, while it is zero when using its parent commit 9fe8046. (For comparison, mypy master reports two errors.) So for my use case, the latest changes are a regression compared to the earlier proposed solution.

Edit: Looking at it in more detail, the error message is new and not a remaining old message, so it's not really a regression. However, it does mean mypy is rejecting the situation where an inner class from one parent overrides an inner class from another parent, which may be too strict for the default settings.

testcase.py:12: error: Definition of "Inner" in base class "OuterBase" is incompatible with definition in base class "OuterMixin"

emmatyping · 2018-11-30T05:40:41Z

It looks like the Windows test failure is a flake. I am looking into it.

ilevkivskyi

OK, so it looks like you changed your mind a bit. IIUC you want to allow only type safe (actually or forced by an Any base) overrides. I like this, but please consider that class Mixin(Any): ... actually fails with TypeError at runtime. This only works in stubs, at runtime one needs to do something like Unsafe: Any = object; class Mixin(Unsafe): ..., or are you going to add fallback_to_any by the plugin for all classes nested in models?

@mthuurne The current version of the PR actually type safe. I would say it makes sense to special-case this for Django (you will need to use the mypy plugin @mkurnikov is writing) instead of making this pattern non-type safe everywhere.

ilevkivskyi · 2018-12-02T18:21:15Z

+            if ((isinstance(first_type, CallableType)
+                 and first_type.fallback.type.fullname() == 'builtins.type')
+                    and (isinstance(second_type, CallableType)
+                         and second_type.fallback.type.fullname() == 'builtins.type')):


This check is unreliable:

What if the class constructor is overloaded?

What if there is a custom metaclass?

Maybe you are trying to reinvent FunctionLike.is_type_obj()?

ilevkivskyi · 2018-12-02T18:21:45Z

+                # Both members are classes (not necessary nested), check if compatible
+                ok = is_subtype(first_type.ret_type, second_type.ret_type)
+            else:
+                # Method override


This comment adds nothing here, I would just remove it.

ilevkivskyi · 2018-12-02T18:25:16Z

+                 and first_type.fallback.type.fullname() == 'builtins.type')
+                    and (isinstance(second_type, CallableType)
+                         and second_type.fallback.type.fullname() == 'builtins.type')):
+                # Both members are classes (not necessary nested), check if compatible


This comment is not clear. Probably you want to say something like: # For class objects only check the subtype relationship of the classes, since we allow incompatible overrides of '__init__'/'__new__'.

ilevkivskyi · 2018-12-02T18:28:06Z

+                    and (isinstance(second_type, CallableType)
+                         and second_type.fallback.type.fullname() == 'builtins.type')):
+                # Both members are classes (not necessary nested), check if compatible
+                ok = is_subtype(first_type.ret_type, second_type.ret_type)


This may cause surprises if the classes are generic, so the return types contain some (independent) type variables. I think you can just erase the type variables here (erasetype.erase_type_vars IIRC).

ilevkivskyi · 2018-12-02T18:28:31Z

+        pass
+class A(Mixin1, Mixin2):
+    pass
+[out]


No newline at the end of file.

mkurnikov · 2018-12-02T19:44:53Z

are you going to add fallback_to_any by the plugin for all classes nested in models?

Yes, I tried it, it worked.
https://github.com/mkurnikov/django-stubs/blob/master/mypy_django_plugin/plugins/meta_inner_class.py#L5

In the wild I've encountered only nested classes used to:

Somehow specify some properties of an outer class objects.
Create class semantically bound to outer class, like User.DoesNotExist for exceptions in Django.

Both of them are handled by the framework in very specific way, I wouldn't expect mypy to understand them without plugin, and errors are more or less OK there.

ilevkivskyi · 2018-12-16T03:04:47Z

Also please don't force push, GitHub doesn't send e-mail notifications for this either. I will take a look at the updates tomorrow.

ilevkivskyi

Thanks! This is almost ready, I have just two more comments.

ilevkivskyi · 2018-12-22T21:33:59Z

+                # For class objects only check the subtype relationship of the classes,
+                # since we allow incompatible overrides of '__init__'/'__new__'
+                ok = is_subtype(left=fill_typevars(first_type.type_object()),
+                                right=fill_typevars(second_type.type_object()))


I think using fill_typevars here is dangerous, since it may produce some false positives for generic classes (it creates an instance with type variable types). You need to apply erase_typevars. Or even better, add a helper function to typevars.py that will be very similar to fill_typevars, but would insert correct number of Any as type arguments.

ilevkivskyi

Thanks! I have few more small comments.

ilevkivskyi · 2018-12-24T12:17:05Z

+T = TypeVar('T')
+class ParentGeneric(Generic[T]):
+    pass
+class ChildGeneric(ParentGeneric, Generic[T]):


Sorry, but this is still not good. You should rather write (here and below):

class ChildGeneric(ParentGeneric[T]): pass

Otherwise the subclass maps to ParentGeneric[Any]. You can read some info about defining generic classes in the mypy docs.

No problem, I feel like I need to read pep484 and docs end to end once more, I take too much of your time. I'll fix everything till the end of the day.

I take too much of your time

Don't worry about this :-)

Thanks!=)
Sorry, haven't found time, but I'm sure I will in a day or two.

ilevkivskyi · 2018-12-24T12:17:46Z

    return typ.tuple_type.copy_modified(fallback=inst)


+def fill_typevars_with_any(typ: TypeInfo) -> Union[Instance, TupleType]:


Could you please add a docstring here?

ilevkivskyi · 2018-12-24T12:26:55Z

+        def __new__(cls, *args, **kwargs: None) -> Mixin1.Meta:
+            pass
+        @overload
+        def __new__(cls, *args, **kwargs: Dict[str, Any]) -> Mixin1.Meta:


This is irrelevant for this PR (IIUC you are just testing that the overloaded constructor actually works), but this signature looks suspicious. If you have def func(**kwds: int): ..., then this call is valid func(x=1, y=2). From your signature it looks like every keyword arguments to the constructor are either all None, or all of them are dictionaries: Mixin1(x=None, y=None), or Mixin1(x={'a': 42, 'b': None}).

No need to fix this, just wanted to avoid confusions.

ilevkivskyi · 2019-01-08T12:42:48Z

@mkurnikov What is the situation here? Is any input from my side needed?

ilevkivskyi reviewed Nov 26, 2018

View reviewed changes

mkurnikov force-pushed the nested-classes-multiple-inheritance branch from 52284b1 to 9fe8046 Compare November 28, 2018 12:17

mkurnikov force-pushed the nested-classes-multiple-inheritance branch from f4b055c to c039095 Compare November 29, 2018 21:37

mkurnikov changed the title ~~Ignore error if there are two nested classes with the same name in multiple inheritance~~ Make error messages from multiple inheritance compatibility check more accurate Nov 29, 2018

ilevkivskyi reviewed Dec 2, 2018

View reviewed changes

better error message for nested classes multiple inheritance

1b41df3

mkurnikov force-pushed the nested-classes-multiple-inheritance branch from c039095 to 1b41df3 Compare December 3, 2018 14:20

ilevkivskyi reviewed Dec 22, 2018

View reviewed changes

mkurnikov added 2 commits December 23, 2018 23:03

Merge branch 'master' into nested-classes-multiple-inheritance

21ca9eb

address latest comments

d8f4b26

ilevkivskyi approved these changes Dec 24, 2018

View reviewed changes

mkurnikov and others added 5 commits January 9, 2019 01:29

merge upstream

aeee4b5

fix removed import

b38a5cf

add docstring to new helper function

860665c

address comments

37af755

Fine-tune the docstring

0a11287

ilevkivskyi merged commit 8179952 into python:master Jan 9, 2019

ilevkivskyi mentioned this pull request Jan 9, 2019

class inside a class #3855

Closed

		return typ.tuple_type.copy_modified(fallback=inst)


		def fill_typevars_with_any(typ: TypeInfo) -> Union[Instance, TupleType]:

Conversation

mkurnikov commented Nov 20, 2018 • edited by ilevkivskyi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mthuurne commented Nov 21, 2018

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mkurnikov commented Nov 28, 2018

Uh oh!

mthuurne commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emmatyping commented Nov 30, 2018

Uh oh!

ilevkivskyi left a comment • 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!

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!

mkurnikov commented Dec 2, 2018

Uh oh!

ilevkivskyi commented Dec 16, 2018

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi 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!

ilevkivskyi commented Jan 8, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mkurnikov commented Nov 20, 2018 •

edited by ilevkivskyi

Loading

mthuurne commented Nov 30, 2018 •

edited

Loading

ilevkivskyi left a comment •

edited

Loading