Warn on type arguments in isinstance #3040
Conversation
mypy/messages.py
Outdated
| @@ -864,6 +864,11 @@ def typeddict_item_name_not_found(self, | |||
| self.fail('\'{}\' is not a valid item name; expected one of {}'.format( | |||
| item_name, format_item_name_list(typ.items.keys())), context) | |||
|
|
|||
| def type_arguments_not_allowed(self, | |||
| context: Context, | |||
| ) -> None: | |||
There was a problem hiding this comment.
Can it be formatted in one line? (maximum allowed length in mypy is 99 chars)
mypy/checkexpr.py
Outdated
| or isinstance(typ, NameExpr) | ||
| and self.chk.lookup_qualified(typ.name).kind == nodes.TYPE_ALIAS): | ||
| self.msg.type_arguments_not_allowed(e) | ||
| except KeyError: |
There was a problem hiding this comment.
This is a bit strange. Do you understand why it could fail?
If this happens only for actual undefined names, you could add a comment "This error is reported elsewhere" (if this is true of course).
In any case, I would put only self.chk.lookup_qualified(typ.name) inside try ... except ... block, to avoid masking possible bugs later.
There was a problem hiding this comment.
Not sure. Here's where it happens when running mypy mypy:
# stubgenc.py
all_bases = obj.mro()
# ...
for base in all_bases:
if not any(issubclass(b, base) for b in bases): # builtins.KeyError: 'Failed lookup: bases'
bases.append(base)
and here:
# testextensions.py
def assertIsSubclass(self, cls, class_or_tuple, msg=None):
if not issubclass(cls, class_or_tuple): # failed lookup
and here:
Emp = TypedDict('Emp', {'name': str, 'id': int})
with self.assertRaises(TypeError):
isinstance({}, Emp) # failed lookup
In all cases, the names are typed as Any. Why weren't they added to the symbol table, and why then didn't mypy complain about using undefined names?
There was a problem hiding this comment.
OK, this looks like something unrelated.
There was a problem hiding this comment.
Regardless, I don't find "except KeyError" acceptable. Which subexpression are you expecting to raise KeyError? And why is it raised?
There was a problem hiding this comment.
@gvanrossum This is an outdated version of PR, in the new version try ... except surrounds only lookup_qualified.
test-data/unit/check-isinstance.test
Outdated
| isinstance(x, Iterable[T]) # E: Type arguments not allowed in this context | ||
| It2 = Iterable[T] | ||
| isinstance(x, It2[int]) # E: Type arguments not allowed in this context | ||
| isinstance(x, It2) # E: Type arguments not allowed in this context |
There was a problem hiding this comment.
I would add few tests with issubclass just in case. Also maybe split the test into smaller tests (e.g. one without aliases, one with aliases, and one with issubclass).
mypy/messages.py
Outdated
| def type_arguments_not_allowed(self, | ||
| context: Context, | ||
| ) -> None: | ||
| self.fail('Type arguments not allowed in this context', context) |
There was a problem hiding this comment.
I would prefer a more detailed error message explaining that this is because of isinstance. For example: "Type arguments not allowed in instance and type checks"
|
@pkch Thanks! I just added two explanatory comments. |
Fixes #3037