Cleanup check_reverse_op_method #4017

elazarg · 2017-09-26T20:32:28Z

Remove old comments, cleanup bail-out logic
Use fallback to find Instance when possible
Remove cast; move formatting logic to messages.py
Rename variables to match check_overlapping_op_methods() terminology

Also:

Remove unused method from TypeInfo
Remove many unused imports

Was part of #3227
EDIT: may fix #3468, or at least avoid the crash (since it removes the unsafe cast).

* Remove old comments, clean bail-out logic * Use fallback to find Instance when possible * Remove cast; move formatting logic to messages.py * remove unused method * rename variables to match check_overlapping_op_methods terminology * remove many unused imports

emmatyping

Just one suggestion, otherwise looks good.

emmatyping · 2017-11-16T20:44:40Z

mypy/checker.py

-        if len(typ.arg_types) == 2:
-            # TODO check self argument kind
+        if len(reverse_type.arg_types) != 2:
+            return


I actually think this should raise an internal error instead of returning, since we should never reach there, now that the signatures are checked.

Makes sense to me. Fixed.

msullivan

Looks good, modulo my one test request

msullivan · 2018-01-26T03:28:59Z

mypy/checker.py

-                return
+        forward_name = nodes.normal_from_reverse_op[reverse_name]
+        forward_base = reverse_type.arg_types[1]
+        if isinstance(forward_base, (FunctionLike, TupleType, TypedDictType)):


Could you add a test for one or all of these cases, which look new?

Added a test for TupleType. I'm not sure how to do it for the other two, if at all possible.

@msullivan what do you think?

elazarg · 2018-02-09T15:07:25Z

I'm not sure why the tests are failing - the errors mention code that's not there at all.

ilevkivskyi · 2018-02-09T15:14:55Z

the errors mention code that's not there at all.

It's on master, you should merge to see it. (Btw it is my fault, missed this in my review, but I will be grateful if you add this import.)

ilevkivskyi · 2018-02-09T15:21:23Z

Btw it is my fault, missed this in my review, but I will be grateful if you add this import.

Hm, I double-checked, the import is there. @elazarg Could you please merge master, maybe it is some kind of subtle merge problem?

elazarg · 2018-02-10T16:25:27Z

Thanks! Yep, a merge problem - too many import changes - so it's my fault (obviously). Sorry.

gvanrossum · 2018-02-26T02:03:50Z

@JukkaL and/or @ilevkivskyi Can you review this so it may make it into 0.570 (if you think it's solid)?

JukkaL · 2018-02-26T16:19:48Z

I'll look at this tomorrow.

JukkaL

Looks mostly good, but there's one crash issue that should be fixed before this can be merged.

JukkaL · 2018-02-27T17:05:59Z

mypy/checker.py

        if isinstance(ret_type, AnyType):
            return
        if isinstance(ret_type, Instance):
            if ret_type.type.fullname() == 'builtins.object':
                return

-        if len(typ.arg_types) == 2:
-            # TODO check self argument kind
+        assert len(reverse_type.arg_types) == 2


This assertion isn't safe. Type checking this program will cause a crash:

class B: def __radd__(*self) -> int: pass

Handled by duplicating self

JukkaL · 2018-02-27T17:15:27Z

mypy/checker.py

+            forward_inst = forward_inst.fallback
+        if not (isinstance(forward_inst, (Instance, UnionType))
+                and forward_inst.has_readable_member(forward_name)):
+            return


Are there any other types that we could plausibly handle here instead of just returning, such as type variables? If yes, it may be worth adding a TODO comment.

Instead of a TODO, added handling of TypeType and TypeVar (+tests)

JukkaL

Thanks for the updates! LGTM now.

* master: New files shouldn't trigger a coarse-grained rebuild in fg cache mode (python#4669) Bump version to 0.580-dev Update revision history for 0.570 (python#4662) Fine-grained: Fix crashes when refreshing synthetic types (python#4667) Fine-grained: Support NewType and reset subtype caches (python#4656) Fine-grained: Detect changes in additional TypeInfo attributes (python#4659) Fine-grained: Apply semantic analyzer patch callbacks (python#4658) Optimize fine-grained update by using Graph as the cache (python#4622) Cleanup check_reverse_op_method (python#4017) Fine-grained: Fix AST merge issues (python#4652) Optionally check that we don't have duplicate nodes after AST merge (python#4647)

* Remove old comments, clean bail-out logic * Use fallback to find Instance when possible * Remove cast; move formatting logic to messages.py * Remove unused method * Rename variables to match check_overlapping_op_methods terminology * Remove many unused imports Potential fix to python#3468, or at least avoids the crash (since it removes the unsafe cast).

elazarg force-pushed the cleanup-reverseop branch 3 times, most recently from f475823 to 9865c6e Compare September 27, 2017 12:39

elazarg force-pushed the cleanup-reverseop branch from 9865c6e to 49a4670 Compare September 27, 2017 12:41

elazarg added 2 commits November 16, 2017 15:28

Merge branch 'master' into cleanup-reverseop

aa6e531

Update checker.py

40861f7

emmatyping requested changes Nov 16, 2017

View reviewed changes

Assert len(reverse_type.arg_types) == 2

d355ff4

emmatyping approved these changes Nov 16, 2017

View reviewed changes

elazarg mentioned this pull request Nov 28, 2017

AttributeError: 'UnionType' object has no attribute 'type' #3468

Closed

msullivan requested changes Jan 26, 2018

View reviewed changes

add test for overlapping TupleType

c51f1c7

Merge remote-tracking branch 'upstream/master' into cleanup-reverseop

26f58bd

elazarg force-pushed the cleanup-reverseop branch from 6816839 to 26f58bd Compare February 10, 2018 18:05

msullivan approved these changes Feb 23, 2018

View reviewed changes

JukkaL requested changes Feb 27, 2018

View reviewed changes

handle stararg self, TypeType and TypeVarType

40bc07d

JukkaL approved these changes Mar 1, 2018

View reviewed changes

JukkaL merged commit 574744e into python:master Mar 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup check_reverse_op_method #4017

Cleanup check_reverse_op_method #4017

elazarg commented Sep 26, 2017 •

edited

Loading

emmatyping left a comment •

edited

Loading

emmatyping Nov 16, 2017

elazarg Nov 16, 2017

msullivan left a comment

msullivan Jan 26, 2018

elazarg Feb 9, 2018

elazarg Feb 13, 2018

elazarg commented Feb 9, 2018

ilevkivskyi commented Feb 9, 2018

ilevkivskyi commented Feb 9, 2018

elazarg commented Feb 10, 2018

gvanrossum commented Feb 26, 2018

JukkaL commented Feb 26, 2018

JukkaL left a comment

JukkaL Feb 27, 2018

elazarg Feb 27, 2018

JukkaL Feb 27, 2018

elazarg Feb 27, 2018

JukkaL left a comment

Cleanup check_reverse_op_method #4017

Cleanup check_reverse_op_method #4017

Conversation

elazarg commented Sep 26, 2017 • edited Loading

emmatyping left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msullivan 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

elazarg commented Feb 9, 2018

ilevkivskyi commented Feb 9, 2018

ilevkivskyi commented Feb 9, 2018

elazarg commented Feb 10, 2018

gvanrossum commented Feb 26, 2018

JukkaL commented Feb 26, 2018

JukkaL 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

JukkaL left a comment

Choose a reason for hiding this comment

elazarg commented Sep 26, 2017 •

edited

Loading

emmatyping left a comment •

edited

Loading