Conversation
elazarg
force-pushed
the
cleanup-reverseop
branch
3 times, most recently
from
f475823 to
9865c6e
Compare
* 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
elazarg
force-pushed
the
cleanup-reverseop
branch
from
9865c6e to
49a4670
Compare
mypy/checker.py
Outdated
| if len(typ.arg_types) == 2: | ||
| # TODO check self argument kind | ||
| if len(reverse_type.arg_types) != 2: | ||
| return |
There was a problem hiding this comment.
I actually think this should raise an internal error instead of returning, since we should never reach there, now that the signatures are checked.
There was a problem hiding this comment.
Makes sense to me. Fixed.
msullivan
left a comment
msullivan
left a comment
There was a problem hiding this comment.
Looks good, modulo my one test request
mypy/checker.py
Outdated
| return | ||
| forward_name = nodes.normal_from_reverse_op[reverse_name] | ||
| forward_base = reverse_type.arg_types[1] | ||
| if isinstance(forward_base, (FunctionLike, TupleType, TypedDictType)): |
There was a problem hiding this comment.
Could you add a test for one or all of these cases, which look new?
There was a problem hiding this comment.
Added a test for TupleType. I'm not sure how to do it for the other two, if at all possible.
|
I'm not sure why the tests are failing - 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.) |
Hm, I double-checked, the import is there. @elazarg Could you please merge master, maybe it is some kind of subtle merge problem? |
|
Thanks! Yep, a merge problem - too many import changes - so it's my fault (obviously). Sorry. |
elazarg
force-pushed
the
cleanup-reverseop
branch
from
6816839 to
26f58bd
Compare
|
@JukkaL and/or @ilevkivskyi Can you review this so it may make it into 0.570 (if you think it's solid)? |
|
I'll look at this tomorrow. |
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
Looks mostly good, but there's one crash issue that should be fixed before this can be merged.
|
|
||
| if len(typ.arg_types) == 2: | ||
| # TODO check self argument kind | ||
| assert len(reverse_type.arg_types) == 2 |
There was a problem hiding this comment.
This assertion isn't safe. Type checking this program will cause a crash:
class B:
def __radd__(*self) -> int: passThere was a problem hiding this comment.
Handled by duplicating self
| forward_inst = forward_inst.fallback | ||
| if not (isinstance(forward_inst, (Instance, UnionType)) | ||
| and forward_inst.has_readable_member(forward_name)): | ||
| return |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Instead of a TODO, added handling of TypeType and TypeVar (+tests)
JukkaL
left a comment
JukkaL
left a comment
There was a problem hiding this comment.
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).
6 participants
messages.pycheck_overlapping_op_methods()terminologyAlso:
Was part of #3227
EDIT: may fix #3468, or at least avoid the crash (since it removes the unsafe cast).