Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix `issubclass()` to narrow down types of type variables #7930

Merged
merged 8 commits into from Nov 14, 2019

Conversation

@TH3CHARLie
Copy link
Contributor

TH3CHARLie commented Nov 12, 2019

resolves #7920

Currently, no testcases break so I did not look into meet.py for further investigation

Copy link
Collaborator

ilevkivskyi left a comment

Thanks for PR! Here I have few comments.

mypy/checker.py Outdated Show resolved Hide resolved
test-data/unit/check-classes.test Outdated Show resolved Hide resolved
test-data/unit/check-classes.test Show resolved Hide resolved
@TH3CHARLie

This comment has been minimized.

Copy link
Contributor Author

TH3CHARLie commented Nov 13, 2019

Thanks for the detailed review! I patched it up in df62cbc
A short summary of what I did:

  • change the logic of how the code handles TypeVarType
  • refactor the issubclass logic into a helper function
  • modify testcase by using the one from the original issue which uses different base types
@TH3CHARLie TH3CHARLie requested a review from ilevkivskyi Nov 13, 2019
Copy link
Collaborator

ilevkivskyi left a comment

Thanks for updates! Here are some more comments.

@@ -3726,32 +3726,7 @@ def find_isinstance_check(self, node: Expression
elif refers_to_fullname(node.callee, 'builtins.issubclass'):
if len(node.args) != 2: # the error will be reported elsewhere
return {}, {}
expr = node.args[0]
if literal(expr) == LITERAL_TYPE:

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 13, 2019

Collaborator

I would keep the above two lines in this function. To make the refactoring more 1-to-1.

@@ -4194,6 +4169,40 @@ def push_type_map(self, type_map: 'TypeMap') -> None:
for expr, type in type_map.items():
self.binder.put(expr, type)

def infer_issubclass_maps(self, node: CallExpr,
type_map: Dict[Expression, Type]
) -> Tuple[TypeMap, TypeMap]:

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 13, 2019

Collaborator

Please add a short docstring for the new function.

return other.field
return 0

[builtins fixtures/isinstancelist.pyi]

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Nov 13, 2019

Collaborator

OK, now the two classes don't look the same, but you didn't do what I asked, I wanted type variables with different upper bounds, like Type[SomeClass], not just type.

@TH3CHARLie TH3CHARLie requested a review from ilevkivskyi Nov 13, 2019
Copy link
Collaborator

ilevkivskyi left a comment

Some more comments here. Also please don't hurry, it is not a competition.

mypy/checker.py Outdated Show resolved Hide resolved
mypy/checker.py Outdated Show resolved Hide resolved
test-data/unit/check-classes.test Outdated Show resolved Hide resolved
test-data/unit/check-classes.test Show resolved Hide resolved
test-data/unit/check-classes.test Outdated Show resolved Hide resolved
@TH3CHARLie

This comment has been minimized.

Copy link
Contributor Author

TH3CHARLie commented Nov 13, 2019

You are right and I shouldn't be in such a hurry.

I had some misunderstanding about this issue from the very beginning and that mistake continues in my testcase. I apologize for that and am trying my best to fill my knowledge gap on type systems(mainly by reading TAPL)

Now I look it again and realize Foo does not have to be there, so I believe this time the testcase is correct and serves its purpose

TH3CHARLie and others added 2 commits Nov 13, 2019
Copy link
Collaborator

ilevkivskyi left a comment

Looks good now, thanks!

@TH3CHARLie

This comment has been minimized.

Copy link
Contributor Author

TH3CHARLie commented Nov 14, 2019

Thanks for the guidance! Again, quite an education on both software engineering and type system aspects and that means a lot to me.

@ilevkivskyi ilevkivskyi merged commit 15280bf into python:master Nov 14, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.