Repository navigation
Don't crash when partial types are used in inherited attribute - #6766
Conversation
ilevkivskyi
left a comment
ilevkivskyi
left a comment
There was a problem hiding this comment.
Thanks, the idea seems reasonable to me. I have several suggestions for additional tests.
| [builtins fixtures/dict.pyi] | ||
|
|
||
| [case testInheritedAttributeNoStrictOptional] | ||
| # flags: --no-strict-optional |
There was a problem hiding this comment.
I would also add a test case with --strict-optional as well (to avoid regressions).
|
|
||
| class B(A): | ||
| x = None | ||
| x = '' |
There was a problem hiding this comment.
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' No newline at end of file |
There was a problem hiding this comment.
No new line at the end of file.
| 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)): |
There was a problem hiding this comment.
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()).
There was a problem hiding this comment.
Yes, you are right. I will change the code, actually.
There was a problem hiding this comment.
Are you going to add the tests with an empty list in the subclass?
There was a problem hiding this comment.
I don't think I should add them anymore, because now I make the exception for only None partial types.
There was a problem hiding this comment.
But there may be a similar crash (or a regression later), I think it is better to have one in any case.
There was a problem hiding this comment.
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
…n#6766) Fixes python#4552 by deferring the compatibility check.
2 participants
Fix #4552.