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

Make error messages from multiple inheritance compatibility check more accurate #5926

Merged
merged 8 commits into from Jan 9, 2019

Conversation

Projects
None yet
4 participants
@mkurnikov
Copy link
Contributor

mkurnikov commented Nov 20, 2018

Fixes #2871.

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

This comment has been minimized.

Copy link
Contributor

mthuurne commented Nov 21, 2018

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

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks for a PR!

Here are few comments.

Show resolved Hide resolved mypy/checker.py Outdated
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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 26, 2018

Collaborator

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).
Show resolved Hide resolved test-data/unit/check-multiple-inheritance.test Outdated

@mkurnikov mkurnikov force-pushed the mkurnikov:nested-classes-multiple-inheritance branch from 52284b1 to 9fe8046 Nov 28, 2018

@mkurnikov

This comment has been minimized.

Copy link
Contributor Author

mkurnikov commented Nov 28, 2018

@ilevkivskyi I addressed your comments.

@mkurnikov mkurnikov force-pushed the mkurnikov:nested-classes-multiple-inheritance branch from f4b055c to c039095 Nov 29, 2018

@mkurnikov 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

@mthuurne

This comment has been minimized.

Copy link
Contributor

mthuurne commented Nov 30, 2018

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"
@ethanhs

This comment has been minimized.

Copy link
Collaborator

ethanhs commented Nov 30, 2018

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

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

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.

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 comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 2, 2018

Collaborator

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()?

# 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 has been minimized.

@ilevkivskyi

ilevkivskyi Dec 2, 2018

Collaborator

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

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 has been minimized.

@ilevkivskyi

ilevkivskyi Dec 2, 2018

Collaborator

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__'.

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 comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 2, 2018

Collaborator

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).

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 2, 2018

Collaborator

No newline at the end of file.

@mkurnikov

This comment has been minimized.

Copy link
Contributor Author

mkurnikov commented Dec 2, 2018

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:

  1. Somehow specify some properties of an outer class objects.
  2. 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.

@mkurnikov mkurnikov force-pushed the mkurnikov:nested-classes-multiple-inheritance branch from c039095 to 1b41df3 Dec 3, 2018

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Dec 16, 2018

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
Copy link
Collaborator

ilevkivskyi left a comment

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

Show resolved Hide resolved test-data/unit/check-multiple-inheritance.test
# 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()))

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 22, 2018

Collaborator

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
Copy link
Collaborator

ilevkivskyi left a comment

Thanks! I have few more small comments.

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 24, 2018

Collaborator

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.

This comment has been minimized.

@mkurnikov

mkurnikov Dec 24, 2018

Author Contributor

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 24, 2018

Collaborator

I take too much of your time

Don't worry about this :-)

This comment has been minimized.

@mkurnikov

mkurnikov Dec 25, 2018

Author Contributor

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

@@ -20,6 +20,13 @@ def fill_typevars(typ: TypeInfo) -> Union[Instance, TupleType]:
return typ.tuple_type.copy_modified(fallback=inst)


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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 24, 2018

Collaborator

Could you please add a docstring here?

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 24, 2018

Collaborator

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

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Jan 8, 2019

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

mkurnikov and others added some commits Jan 8, 2019

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ilevkivskyi ilevkivskyi referenced this pull request Jan 9, 2019

Closed

class inside a class #3855

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment