Sanity checks for declared selftype #2381

elazarg · 2016-10-29T23:09:46Z

Issues:

The error message is clunky
~~The signature for type.__subclasses__ I suggested does not play well with the check. It can be easily fixed with an appropriate TypeVar~~
~~Many incompatibilities with typeshed, Reported in typeshed/#638~~

Added a Scope class, to handle both function_stack and class_context.

JukkaL · 2016-10-31T10:46:44Z

test-data/unit/check-selftype.test

+        return cls()
+
+class D:
+    def foo(self: str) -> str:  # E: The type of self 'builtins.str' is not a supertype its class '__main__.D'


Outside a stub file, we could allow a function like this in a function body, assuming that the first argument is not named self (see discussion at #2374 and #16). However, the function shouldn't be usable as an instance method. I've heard about production code that relies on this. It's a questionable idiom, as a missing self is usually an error, and if we allow this idiom it can result in false negatives.

@gvanrossum What do you think?

I would like to try this out on our two big internal repos. If this finds no issues, I think we should not do this. (If there are complaints we can relent, but if it's rare people can just use # type: ignore.)

FYI I ran this over our "S" repo and it found 7 legitimate issues, no cases where a function was meant to be called from inside the class, and one mystery that I still have to debug (it was a long-form Python 2.7 signature annotation on a class method, but it correctly left out the 'cls' argument's type -- there might be something wrong due to a lambda used as a default value in one case.

In the "C" repo, I found this:

class C: def __new__(cls: 'Type[C]') -> 'C': pass

Would be good to accept such usage.

In that repo this PR also found a few bad annotations but no occurrences of the mythical questionable idiom. :-)

Here's a repro for the first mystery; it's indeed related to a lambda:

from typing import Callable class C: @classmethod def foo(cls, arg: Callable[[int], str] = lambda a: '' ): pass

This gives an error on the 'def' line:

__tmp__.py: note: In member "foo" of class "C": __tmp__.py:4: error: The type of self 'builtins.int' is not a supertype its class '__tmp__.C'

This is actually the mythical questionable idiom! In some sense at least :)

gvanrossum · 2016-10-31T23:46:55Z

mypy/checker.py

+                            ref_type = mypy.types.TypeType(ref_type)
+                        erased = erase_to_bound(arg_type)
+                        if not is_subtype_ignoring_tvars(ref_type, erased):
+                            self.fail("The type of self '{}' is not a supertype its class '{}'"


Missing 'of'.

elazarg · 2016-11-01T01:53:30Z

__new__ does not allow passing Type[B] for Type[A] although B <: A. This is why testSuperWithNew fails.

gvanrossum · 2016-11-01T02:23:35Z

Thanks! This found 11 bad annotations in our internal code.

gvanrossum · 2016-11-01T20:31:03Z

python/typeshed#638 is now fixed. I am syncing typeshed in master, once that lands (in about 20 minutes I hope) can you rebase this and see if you can get the tests to pass?

elazarg · 2016-11-01T20:33:50Z

Sure. But note that the problem with __new__ is unrelated.

…ython#2382)

elazarg · 2016-11-01T21:20:55Z

I will try to solve the __new__ problem.

(One of these days I will learn how to rebase)

gvanrossum · 2016-11-01T21:32:51Z

But note that the problem with new is unrelated.

Unrelated to what? I see you added tests for it. If those tests were just to help you understand my bug report, that's fine, and you can remove them again.

elazarg · 2016-11-01T21:39:04Z

The current problem with __new__ is unrelated to typeshed, therefore the tests do not pass yet. I have pretty much fixed your bug report, and the tests I have added pass. It's an existing test that fails - testSuperWithNew - for what seems like a strange subtyping problem:

Argument 1 to "__new__" of "A" has incompatible type "B"; expected "A"

Even though class B inherits from A.

gvanrossum · 2016-11-01T21:50:24Z

Dang. Wasted a lot of bandwidth because you left out "typeshed". :-(

elazarg · 2016-11-01T22:04:15Z

Sorry.

elazarg · 2016-11-01T22:43:42Z

There was an actual type mismatch in testSuperWithNew. This is ready for a review now.

Any suggestions for a better error message? Perhaps there should be a different one for classmethods.

gvanrossum · 2016-11-01T22:48:08Z

Thanks, I'll see if I can dive into this once I'm done with another big code review.

gvanrossum

Gotta run, I didn't look at much of the code yet, but here's a start.

gvanrossum · 2016-11-01T23:48:01Z

mypy/checker.py

-            if class_type:
-                self.class_context.append(class_type)
+
+            # Shouldn't we freeze the entire scope?


That comment should go in the next function.

gvanrossum · 2016-11-01T23:50:29Z

mypy/checker.py

@@ -508,7 +502,6 @@ def check_func_item(self, defn: FuncItem,
        if isinstance(defn, FuncDef):
            fdef = defn

-        self.function_stack.append(defn)


Curious why you don't need scope.push_function() here?

Because the signature is checked in the scope of the class. I have moved the push to line 645, just before checking the body. Is it wrong?

Oh, that sounds fine.

gvanrossum · 2016-11-01T23:53:46Z

mypy/checker.py

+        return None
+
+    @contextmanager
+    def enter_function(self, item: FuncItem) -> Iterator[None]:


Shouldn't this use push/pop_function() to make it clear it's doing the right thing? Ditto enter_class().

gvanrossum · 2016-11-01T23:57:03Z

mypy/checker.py

+        yield
+        self.stack.pop()
+
+    def is_method(self) -> bool:


Can you give this a clearer name, e.g. is_within_method()? And isn't it really checking whether we're inside a class?

gvanrossum · 2016-11-01T23:57:30Z

mypy/checker.py

+        self.stack.append(item)
+
+    def pop_function(self) -> None:
+        assert isinstance(self.stack[-1], Type)


This seems oddly mismatched with the argument type of push_function(). And since the assert never failed in the tests I wonder if this code is exercised at all?

gvanrossum · 2016-11-01T23:58:05Z

mypy/checker.py

+
+    def pop_function(self) -> None:
+        assert isinstance(self.stack[-1], Type)
+        self.stack.pop()


Maybe assert that the stack is not now empty? (Then also in the other pop methods.)

gvanrossum · 2016-11-02T00:24:10Z

mypy/checker.py

-                self.class_context.append(class_type)
+
+            # Shouldn't we freeze the entire scope?
+            if active_class:


If you rewrite this as

if active_class: with self.scope.push_class(active_class): self.accept(node) else: self.accept(node)

you won't need the Scope.push/pop methods at all!

I just did this :)

gvanrossum · 2016-11-02T00:42:33Z

mypy/checker.py

-                                          arg_type)
-                        # FIX: if i == 0 and this is not a method then same as above
+                    ref_type = self.scope.active_class()
+                    if (isinstance(defn, FuncDef) and ref_type is not None and i == 0


I see the FuncDef check is to fix the lambda case I reported. This makes me think that was a pretty close call -- I found 1 case of that in over 300K LoC! And the problem was because check_func_def() is called for 'def' as well as for 'lambda', and there are various other checks to weed out static methods and to do the right thing for class and instance methods, but somehow a lambda anywhere in a class would also be checked.

(I'm just muttering to myself here, I just think it's interesting to see how time after time we find that there are more combinations of features than we can possibly test.)

For me it's "Lesson 8: The State Explosion Problem" :)

This also assumes foo in foo = lambda ... is not a method (from selftype perspective), which I think is fine, but I'd expect to happen once every hundred-thousand lines or so.

Yeah, foo = lambda ... is questionable, since when calling it via the class or instance, it will be treated as a method. But you can't have types on lambda args so it shouldn't matter what mypy thinks in that case. :-)

gvanrossum · 2016-11-02T16:43:25Z

mypy/checker.py

+    stack = None  # type: List[Union[Type, FuncItem, MypyFile]]
+
+    def __init__(self, module: MypyFile) -> None:
+        self.stack = [module]


I don't think the module is ever used. If I replace this with [None] all tests seem to pass. But I'll just merge now, you can fix this up later.

It is not used right now, but I don't see how None is more suitable as a sentinel in this case.

It was the quickest fix -- if I start with an empty stack I have to add guards against an empty stack in active_class(), which currently looks at stack[-1]. But I agree a guard is better.

elazarg added 3 commits October 29, 2016 23:15

make sure selftype is sensible

d3e99fb

add scope. still problems with typeshed

bea4c50

simplify

f0c3962

This was referenced Oct 29, 2016

Multiple incompatibilities for the type of self python/typeshed#638

Closed

Dedicated TypeVar for type.__subclasses__ python/typeshed#639

Merged

elazarg changed the title ~~Sanity checks for declared selftype~~ Sanity checks for declared selftype [depends on typeshed/#638] Oct 30, 2016

elazarg mentioned this pull request Oct 30, 2016

Check type variable bounds of self types #2351

Open

JukkaL reviewed Oct 31, 2016

View reviewed changes

gvanrossum reviewed Oct 31, 2016

View reviewed changes

elazarg added 3 commits November 1, 2016 02:35

fix typo

85bbae1

don't check selftype of lambda

02f73aa

support __new__. Does not allow calls with subtype for some reason

00874d4

gvanrossum changed the title ~~Sanity checks for declared selftype [depends on typeshed/#638]~~ Sanity checks for declared selftype Nov 1, 2016

gvanrossum and others added 3 commits November 1, 2016 23:04

Various updates to typing.py (hopefully that's all for Python 3.6b3) (p…

1521695

…ython#2382)

Sync typeshed (python#2387)

8a8ef50

Merge branch 'master' of https://github.com/python/mypy

73a485c

elazarg added 2 commits November 2, 2016 00:21

Fix signature of __new__ to be compatible

2c5cb14

lint

fee4a45

gvanrossum reviewed Nov 2, 2016

View reviewed changes

avoid push/pop

2450a44

gvanrossum reviewed Nov 2, 2016

View reviewed changes

rename enter_ to push_

14d5dca

gvanrossum reviewed Nov 2, 2016

View reviewed changes

gvanrossum merged commit a90841f into python:master Nov 2, 2016

Sanity checks for declared selftype #2381

Sanity checks for declared selftype #2381

Conversation

elazarg commented Oct 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elazarg commented Nov 1, 2016

gvanrossum commented Nov 1, 2016 via email

gvanrossum commented Nov 1, 2016

elazarg commented Nov 1, 2016

elazarg commented Nov 1, 2016

gvanrossum commented Nov 1, 2016

elazarg commented Nov 1, 2016 • edited Loading

gvanrossum commented Nov 1, 2016 via email

elazarg commented Nov 1, 2016

elazarg commented Nov 1, 2016

gvanrossum commented Nov 1, 2016

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elazarg commented Oct 29, 2016 •

edited

Loading

gvanrossum Oct 31, 2016 •

edited

Loading

elazarg commented Nov 1, 2016 •

edited

Loading