Type check arguments for "super" #3919

JukkaL · 2017-09-05T16:50:36Z

This is not quite as strict as we could be, but this is still better
than what we had before. A full implementation will be somewhat
complicated.

Remaining issues:

Assume that the first argument refers to the enclosing class, even
though that's not generally the case.
Assume that a type object as the second argument is always fine.
Don't validate some kinds of type object types as the first argument.
Single-argument variant is not supported.

Fixes #526 (modulo the open issues).

Also, I no longer think that defining messages as constants in
mypy.messages is generally a good idea so I'm not following
that pattern here. I'll officially update the convention in a
separate PR.

This is not quite as strict as we could be, but this is still better than what we had before. A full implementation will be somewhat complicated. Remaining issues: * Assume that the first argument refers to the enclosing class, even though that's not generally the case. * Assume that a type object as the second argument is always fine. * Don't validate some kinds of type object types as the first argument. * Single-argument variant is not supported. Fixes #526 (modulo the open issues). Also, I no longer think that defining messages as constants in `mypy.messages` is generally a good idea so I'm not following that pattern here. I'll officially update the convention in a separate PR.

JukkaL · 2017-09-05T16:52:12Z

This is ready for review but this is not ready to be merged yet, as this requires some changes to internal Dropbox code.

I'll create follow-up issues for any remaining open issues after this has been merged.

emmatyping · 2017-09-05T17:06:26Z

Appveyor failure is a flake (see #3895)

ilevkivskyi

Thanks! This is now the oldest high priority issue, I am glad it will be fixed now.

ilevkivskyi · 2017-09-05T18:20:05Z

mypy/checkexpr.py

+        elif len(e.call.args) > 2:
+            self.chk.fail('Too many arguments for "super"', e)
+        elif self.chk.options.python_version[0] == 2 and len(e.call.args) == 0:
+            self.chk.fail('Too few arguments for "super"', e)


I would move all the logic until this point to semanal.py, but it is optional.

I'm keeping the logic here because we want to execute the final elif block only if all the previous checks are false, and having all the checks here makes it explicit. Also, we check the argument counts of other calls during type checking (though super() is special so we could do it during semantic analysis as well).

ilevkivskyi · 2017-09-05T18:22:03Z

mypy/checkexpr.py

+            elif isinstance(type_obj_type, AnyType):
+                return
+            else:
+                self.chk.fail('First argument for "super" must be a type object', e)


I would add Got: "<actual type>"

Updated, but I still don't display the class name, since it looked confusing (due to type of instance C looking like a reference C to type object).

ilevkivskyi · 2017-09-05T18:25:48Z

mypy/checkexpr.py

+                    instance_type = instance_type.upper_bound
+                    if not isinstance(instance_type, (Instance, TupleType)):
+                        # Too tricky, give up.
+                        self.chk.fail('Unsupported argument 2 for "super"', e)


This is a bit inconsistent with the first argument logic above, where we give up by a simple return without any error message, why it is so?

I now generate error above as well.

ilevkivskyi · 2017-09-05T18:32:23Z

mypy/checkexpr.py

+                # TODO: Check whether this is a valid type object here.
+                pass
+            elif not isinstance(instance_type, AnyType):
+                self.chk.fail('Unsupported argument 2 for "super"', e)


My question above about consistency also applies here. Also in this case the error message is repeated, so maybe it makes sense to define a constant for it?

Updated to use a constant for the error message.

ilevkivskyi · 2017-09-05T18:45:00Z

mypy/checkexpr.py

+            elif not isinstance(instance_type, AnyType):
+                self.chk.fail('Unsupported argument 2 for "super"', e)
+
    def analyze_super(self, e: SuperExpr, is_lvalue: bool) -> Type:


I remember looking at the function analyze_super and something worried me. Namely, look at the error:
super() requires at least one positional argument several lines below (can't comment there), it is misleading since it can appear on Python 3 as well but we support super() without arguments. Also the logic there seems a bit wrong.

The message refers to the enclosing function not having any positional arguments. Updated the wording. If it's still incorrect, we can create a new issue about it, since this looks like a pretty marginal scenario. The logic is kind of convoluted though -- most of the for loop body is executed at most once, which is not immediately obvious.

You already have a list of things still left to fix in the PR description, if you feel this deserves a separate issue, then OK, otherwise you can just combine this with first item in your list.

It's still unclear to me what the issue is (other than the code being hard to understand). Can you create a new issue (or add a comment to an existing relevant issue) about this?

ilevkivskyi · 2017-09-05T18:46:10Z

mypy/treetransform.py


    def visit_super_expr(self, node: SuperExpr) -> SuperExpr:
-        new = SuperExpr(node.name)
+        new = SuperExpr(node.name, cast(CallExpr, self.expr(node.call)))


Why a cast needed here? Is it possible to replace it with an assert?

Replaced with an assert.

ilevkivskyi · 2017-09-05T18:49:29Z

test-data/unit/check-super.test

+        super(C, self).xyz
+
+[case testSuperWithTypeType]
+from typing import Type


You are not using this import, have you forgotten to test something here?

Yes, added another thing to test.

ilevkivskyi · 2017-09-05T18:53:07Z

test-data/unit/check-super.test

+        return self
+
+[case testSuperWithTypeVarValues]
+from typing import TypeVar, Generic


You imported Generic but didn't use it. Did you want to add another test also here? I would actually add some tests with Generic.

Added a test that uses Generic.

ilevkivskyi · 2017-09-05T18:54:22Z

test-data/unit/check-super.test

+    def f(self) -> None: pass
+
+class C(B):
+    def f(self) -> None: pass


I would make return types different here and in superclass (e.g. float and int), so that we could be sure in reveal_type that the correct one is picked up.

ilevkivskyi

Just one more question. You can merge this when the internal changes you mentioned are ready.

ilevkivskyi · 2017-09-06T15:34:44Z

mypy/checkexpr.py

+                if not isinstance(item, Instance):
+                    # A complicated type object type. Too tricky, give up.
+                    # TODO: Do something more clever here.
+                    self.chk.fail(messages.UNSUPPORTED_ARGUMENT_2_FOR_SUPER, e)


Should it be actually argument 1 here?

Yes! Added a test case and fixed the error message.

ilevkivskyi reviewed Sep 5, 2017

View reviewed changes

JukkaL added 7 commits September 6, 2017 12:40

Merge branch 'master' into type-check-super

c2d28b3

Merge branch 'master' into type-check-super

8d21272

Update tests based on feedback

de06c5f

Replace cast with assert

cf4502b

Add test case and update confusing error message

ddac7bc

Update based on review

7360246

Update error message

9c2a324

ilevkivskyi approved these changes Sep 6, 2017

View reviewed changes

Fix invalid error message

66e4405

JukkaL merged commit 2247870 into master Sep 6, 2017

JukkaL deleted the type-check-super branch September 6, 2017 16:00

This was referenced Sep 6, 2017

Use first super() argument as the target type #3926

Closed

Support single-argument form of "super" #3927

Closed

Check type object as the second super() argment #3928

Open

Support additional Type[x] values as first super() argument #3929

Open

ilevkivskyi mentioned this pull request Sep 6, 2017

Check the type of the first argument of the function enclosing super() #3931

Closed

Uh oh!

Type check arguments for "super" #3919

Type check arguments for "super" #3919

Uh oh!

Conversation

JukkaL commented Sep 5, 2017

Uh oh!

JukkaL commented Sep 5, 2017

Uh oh!

emmatyping commented Sep 5, 2017

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!

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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ilevkivskyi left a comment •

edited

Loading