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

Don't crash when partial types are used in inherited attribute #6766

Merged
merged 3 commits into from May 13, 2019

Conversation

@onlined
Copy link
Contributor

commented May 5, 2019

Fix #4552.

Fix
@ilevkivskyi
Copy link
Collaborator

left a comment

Thanks, the idea seems reasonable to me. I have several suggestions for additional tests.

@@ -2689,3 +2689,13 @@ reveal_type(x) # E: Revealed type is 'builtins.list[Any]'
reveal_type(y) # E: Revealed type is 'builtins.dict[Any, Any]'

[builtins fixtures/dict.pyi]

[case testInheritedAttributeNoStrictOptional]
# flags: --no-strict-optional

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

I would also add a test case with --strict-optional as well (to avoid regressions).


class B(A):
x = None
x = ''

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

Could you please also add a test where this causes an incompatible override error (an int)?

class B(A):
x = None
x = ''
reveal_type(x) # E: Revealed type is 'builtins.str'

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

No new line at the end of file.

@@ -1833,6 +1833,11 @@ def check_assignment(self, lvalue: Lvalue, rvalue: Expression, infer_lvalue_type
# Try to infer a partial type. No need to check the return value, as
# an error will be reported elsewhere.
self.infer_partial_type(lvalue_type.var, lvalue, rvalue_type)
# Handle PartialType's super type checking here, after it's resolved.
if (isinstance(lvalue, RefExpr) and
self.check_compatibility_all_supers(lvalue, lvalue_type, rvalue)):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 6, 2019

Collaborator

The exception you added above removes the check for all kinds of partial types, while this only re-adds the check for partial None types. I don't say it is necessarily wrong, but I would at least add test cases with partial list and dict types (e.g. with an empty list in subclass that is subsequently gets an .append()).

This comment has been minimized.

Copy link
@onlined

onlined May 7, 2019

Author Contributor

Yes, you are right. I will change the code, actually.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 7, 2019

Collaborator

Are you going to add the tests with an empty list in the subclass?

This comment has been minimized.

Copy link
@onlined

onlined May 7, 2019

Author Contributor

I don't think I should add them anymore, because now I make the exception for only None partial types.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 7, 2019

Collaborator

But there may be a similar crash (or a regression later), I think it is better to have one in any case.

This comment has been minimized.

Copy link
@onlined

onlined May 13, 2019

Author Contributor

I tried this on both master and this branch, it didn't give an error:

class A:
    x = ['a', 'b']

class B(A):
    x = []
    x.append(2)

I think it should give an error, but I could not be sure. What do you think? @ilevkivskyi

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi May 13, 2019

Collaborator

Yes, this should be an error. It looks like another consequence of #4547. Let's then merge this PR as is, and consider adding this test when #4547 is fixed. Could you please add a comment in that issue?

onlined added some commits May 7, 2019

@ilevkivskyi ilevkivskyi merged commit e0b1329 into python:master May 13, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

PattenR added a commit to PattenR/mypy that referenced this pull request Jun 23, 2019

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.