Skip to content

Cleanup check_reverse_op_method - #4017

Merged
JukkaL merged 7 commits into
python:masterfrom
elazarg:cleanup-reverseop
Mar 1, 2018
Merged

Cleanup check_reverse_op_method#4017
JukkaL merged 7 commits into
python:masterfrom
elazarg:cleanup-reverseop

Conversation

@elazarg

@elazargelazarg commented Sep 26, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor
  • 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).

@elazarg
elazargforce-pushed the cleanup-reverseop branch 3 times, most recently from f475823 to 9865c6eCompareSeptember 27, 2017 12:39
@elazarg
Cleanup check_reverse_op_method
49a4670
* 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
emmatyping requested changes Nov 16, 2017
View reviewed changes

@emmatypingemmatyping left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one suggestion, otherwise looks good.

Comment threadmypy/checker.py Outdated
if len(typ.arg_types) == 2:
# TODO check self argument kind
if len(reverse_type.arg_types) != 2:
return

@emmatypingemmatypingNov 16, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@elazargelazargNov 16, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. Fixed.

@elazargelazarg mentioned this pull request Nov 28, 2017
Closed
msullivan
msullivan requested changes Jan 26, 2018
View reviewed changes

@msullivanmsullivan left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, modulo my one test request

Comment threadmypy/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)):

@msullivanmsullivanJan 26, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@elazargelazargFeb 9, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@elazargelazargFeb 13, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msullivan what do you think?

@elazarg

elazarg commented Feb 9, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@ilevkivskyi

ilevkivskyi commented Feb 9, 2018

Copy link
Copy Markdown
Member

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

ilevkivskyi commented Feb 9, 2018

Copy link
Copy Markdown
Member

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

elazarg commented Feb 10, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@gvanrossum

gvanrossum commented Feb 26, 2018

Copy link
Copy Markdown
Member

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

@JukkaL

JukkaL commented Feb 26, 2018

Copy link
Copy Markdown
Collaborator

I'll look at this tomorrow.

JukkaL
JukkaL requested changes Feb 27, 2018
View reviewed changes

@JukkaLJukkaL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment threadmypy/checker.py

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

@JukkaLJukkaLFeb 27, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

classB:
def__radd__(*self) ->int: pass

@elazargelazargFeb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled by duplicating self

Comment threadmypy/checker.py
forward_inst = forward_inst.fallback
if not (isinstance(forward_inst, (Instance, UnionType))
and forward_inst.has_readable_member(forward_name)):
return

@JukkaLJukkaLFeb 27, 2018

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@elazargelazargFeb 27, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

JukkaL
JukkaL approved these changes Mar 1, 2018
View reviewed changes

@JukkaLJukkaL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! LGTM now.

@JukkaL
JukkaL merged commit 574744e into python:masterMar 1, 2018
carljm added a commit to carljm/mypy that referenced this pull request Mar 6, 2018
@carljm
Merge branch 'master' into import-cycle
8a222da
* 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)
yedpodtrzitko pushed a commit to kiwicom/mypy that referenced this pull request Mar 15, 2018
@elazarg@yedpodtrzitko
Cleanup check_reverse_op_method (python#4017)
dd117af
* 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).
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@msullivanmsullivanmsullivan approved these changes
@JukkaLJukkaLJukkaL approved these changes
@emmatypingemmatypingemmatyping approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

6 participants

@elazarg@ilevkivskyi@gvanrossum@JukkaL@msullivan@emmatyping