-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Disallow direct item access of NotRequired TypedDict properties: … #12095
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
Changes from all commits
cc9615a
37f9e7b
04b260a
2c38fd3
e53e1b7
37aabde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4500,7 +4500,7 @@ def find_isinstance_check_helper(self, node: Expression) -> Tuple[TypeMap, TypeM | |
# types of literal string or enum expressions). | ||
|
||
operands = [collapse_walrus(x) for x in node.operands] | ||
operand_types = [] | ||
operand_types: List[Type] = [] | ||
narrowable_operand_index_to_hash = {} | ||
for i, expr in enumerate(operands): | ||
if expr not in type_map: | ||
|
@@ -4543,6 +4543,9 @@ def find_isinstance_check_helper(self, node: Expression) -> Tuple[TypeMap, TypeM | |
|
||
partial_type_maps = [] | ||
for operator, expr_indices in simplified_operator_list: | ||
if_map: TypeMap = {} | ||
else_map: TypeMap = {} | ||
|
||
if operator in {'is', 'is not', '==', '!='}: | ||
# is_valid_target: | ||
# Controls which types we're allowed to narrow exprs to. Note that | ||
|
@@ -4578,8 +4581,6 @@ def has_no_custom_eq_checks(t: Type) -> bool: | |
expr_types = [operand_types[i] for i in expr_indices] | ||
should_narrow_by_identity = all(map(has_no_custom_eq_checks, expr_types)) | ||
|
||
if_map: TypeMap = {} | ||
else_map: TypeMap = {} | ||
if should_narrow_by_identity: | ||
if_map, else_map = self.refine_identity_comparison_expression( | ||
operands, | ||
|
@@ -4609,34 +4610,28 @@ def has_no_custom_eq_checks(t: Type) -> bool: | |
elif operator in {'in', 'not in'}: | ||
assert len(expr_indices) == 2 | ||
left_index, right_index = expr_indices | ||
if left_index not in narrowable_operand_index_to_hash: | ||
continue | ||
|
||
item_type = operand_types[left_index] | ||
collection_type = operand_types[right_index] | ||
left_is_narrowable = left_index in narrowable_operand_index_to_hash | ||
right_is_narrowable = right_index in narrowable_operand_index_to_hash | ||
|
||
# We only try and narrow away 'None' for now | ||
if not is_optional(item_type): | ||
continue | ||
left_type = operand_types[left_index] | ||
right_type = operand_types[right_index] | ||
|
||
collection_item_type = get_proper_type(builtin_item_type(collection_type)) | ||
if collection_item_type is None or is_optional(collection_item_type): | ||
continue | ||
if (isinstance(collection_item_type, Instance) | ||
and collection_item_type.type.fullname == 'builtins.object'): | ||
continue | ||
if is_overlapping_erased_types(item_type, collection_item_type): | ||
if_map, else_map = {operands[left_index]: remove_optional(item_type)}, {} | ||
else: | ||
continue | ||
else: | ||
if_map = {} | ||
else_map = {} | ||
if left_is_narrowable: | ||
narrowed_left_type = self.refine_optional_in(left_type, right_type) | ||
if narrowed_left_type: | ||
if_map = {operands[left_index]: narrowed_left_type} | ||
|
||
elif right_is_narrowable: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If In particular should this just be |
||
narrowed_right_type = self.refine_typeddict_in(left_type, right_type) | ||
if narrowed_right_type: | ||
if_map = {operands[right_index]: narrowed_right_type} | ||
|
||
if operator in {'is not', '!=', 'not in'}: | ||
if_map, else_map = else_map, if_map | ||
|
||
partial_type_maps.append((if_map, else_map)) | ||
if if_map != {} or else_map != {}: | ||
partial_type_maps.append((if_map, else_map)) | ||
|
||
return reduce_conditional_maps(partial_type_maps) | ||
elif isinstance(node, AssignmentExpr): | ||
|
@@ -4865,6 +4860,56 @@ def replay_lookup(new_parent_type: ProperType) -> Optional[Type]: | |
expr = parent_expr | ||
expr_type = output[parent_expr] = make_simplified_union(new_parent_types) | ||
|
||
def refine_optional_in(self, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The narrowing support provided by this commit is very cool, but I don't have enough brainpower this evening to look at all the new code in this commit in detail. Requesting a second pair of eyes on the narrowing logic in this commit. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did review the narrowing support here. Additional review is no longer requested for this commit. |
||
item_type: Type, | ||
collection_type: Type, | ||
) -> Optional[Type]: | ||
""" | ||
Check whether a condition `optional_item in collection_type` can narrow away Optional. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably you meant:
(There is no |
||
|
||
Returns the narrowed item_type, if any narrowing is appropriate. | ||
""" | ||
if not is_optional(item_type): | ||
return None | ||
|
||
collection_item_type = get_proper_type(builtin_item_type(collection_type)) | ||
if collection_item_type is None or is_optional(collection_item_type): | ||
return None | ||
|
||
if (isinstance(collection_item_type, Instance) | ||
and collection_item_type.type.fullname == 'builtins.object'): | ||
return None | ||
if is_overlapping_erased_types(item_type, collection_item_type): | ||
return remove_optional(item_type) | ||
return None | ||
|
||
def refine_typeddict_in(self, | ||
literal_type: Type, | ||
collection_type: Type, | ||
) -> Optional[Type]: | ||
""" | ||
Check whether a condition `'literal' in typeddict` can narrow a non-required ite | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: "ite" -> "item" |
||
into a required item. | ||
|
||
Returns the narrowed collection_type, if any narrowing is appropriate. | ||
""" | ||
collection_type = get_proper_type(collection_type) | ||
if not isinstance(collection_type, TypedDictType): | ||
return None | ||
|
||
literals = try_getting_str_literals_from_type(literal_type) | ||
if literals is None or len(literals) > 1: | ||
return None | ||
|
||
key = literals[0] | ||
if key not in collection_type.items: | ||
return None | ||
|
||
if collection_type.is_required(key): | ||
return None | ||
|
||
return collection_type.copy_modified(required_keys=collection_type.required_keys | {key}) | ||
|
||
def refine_identity_comparison_expression(self, | ||
operands: List[Expression], | ||
operand_types: List[Type], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -238,37 +238,65 @@ def typed_dict_get_signature_callback(ctx: MethodSigContext) -> CallableType: | |
|
||
def typed_dict_get_callback(ctx: MethodContext) -> Type: | ||
"""Infer a precise return type for TypedDict.get with literal first argument.""" | ||
if (isinstance(ctx.type, TypedDictType) | ||
and len(ctx.arg_types) >= 1 | ||
and len(ctx.arg_types[0]) == 1): | ||
keys = try_getting_str_literals(ctx.args[0][0], ctx.arg_types[0][0]) | ||
if keys is None: | ||
return ctx.default_return_type | ||
if not ( | ||
isinstance(ctx.type, TypedDictType) | ||
and len(ctx.arg_types) >= 1 | ||
and len(ctx.arg_types[0]) == 1 | ||
): | ||
return ctx.default_return_type | ||
|
||
output_types: List[Type] = [] | ||
for key in keys: | ||
value_type = get_proper_type(ctx.type.items.get(key)) | ||
if value_type is None: | ||
return ctx.default_return_type | ||
|
||
if len(ctx.arg_types) == 1: | ||
output_types.append(value_type) | ||
elif (len(ctx.arg_types) == 2 and len(ctx.arg_types[1]) == 1 | ||
and len(ctx.args[1]) == 1): | ||
default_arg = ctx.args[1][0] | ||
if (isinstance(default_arg, DictExpr) and len(default_arg.items) == 0 | ||
and isinstance(value_type, TypedDictType)): | ||
# Special case '{}' as the default for a typed dict type. | ||
output_types.append(value_type.copy_modified(required_keys=set())) | ||
else: | ||
output_types.append(value_type) | ||
output_types.append(ctx.arg_types[1][0]) | ||
|
||
if len(ctx.arg_types) == 1: | ||
output_types.append(NoneType()) | ||
|
||
return make_simplified_union(output_types) | ||
return ctx.default_return_type | ||
keys = try_getting_str_literals(ctx.args[0][0], ctx.arg_types[0][0]) | ||
if keys is None: | ||
return ctx.default_return_type | ||
|
||
default_type: Optional[Type] | ||
if len(ctx.arg_types) == 1: | ||
default_type = None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like you could simplify by making this
|
||
elif len(ctx.arg_types) == 2 and len(ctx.arg_types[0]) == 1: | ||
default_type = ctx.arg_types[1][0] | ||
else: | ||
default_type = ctx.default_return_type | ||
|
||
output_types: List[Type] = [] | ||
|
||
for key in keys: | ||
value_type = get_proper_type(ctx.type.items.get(key)) | ||
if value_type is None: | ||
# It would be nice to issue a "TypedDict has no key {key}" failure here. However, | ||
# we don't do this because in the case where you have a union of typeddicts, and | ||
# one of them has the key but others don't, an error message is incorrect, and | ||
# the plugin API has no mechanism to distinguish these cases. | ||
output_types.append(default_type or NoneType()) | ||
continue | ||
|
||
if ctx.type.is_required(key): | ||
# Without unions we could issue an error for .get('required_key', default) because | ||
# the default doesn't make sense. But because of unions, we don't do that. | ||
output_types.append(value_type) | ||
continue | ||
Comment on lines
+265
to
+276
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks a bit unusual to me. However I am unfamilar with mypy's plugin system - used by this file - so can't comment on the workaround. Someone else familar with mypy plugins may want to look over this code more carefully. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JukkaL , it looks like you've made all previous notable modifications to Could you take a look at this code yourself, or suggest a different reviewer who would be familiar with the limits of mypy's plugin system? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did finish my own review of |
||
|
||
if default_type is None: | ||
output_types.extend([ | ||
value_type, | ||
NoneType(), | ||
]) | ||
continue | ||
|
||
# Special case '{}' as the default for a typed dict type. | ||
if len(ctx.args[1]) == 1: | ||
default_arg = ctx.args[1][0] | ||
if (isinstance(default_arg, DictExpr) and len(default_arg.items) == 0 | ||
and isinstance(value_type, TypedDictType)): | ||
|
||
output_types.append(value_type.copy_modified(required_keys=set())) | ||
continue | ||
|
||
output_types.extend([ | ||
value_type, | ||
default_type, | ||
]) | ||
|
||
return make_simplified_union(output_types) | ||
|
||
|
||
def typed_dict_pop_signature_callback(ctx: MethodSigContext) -> CallableType: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This documentation is so much more clear with this change. 👍 (No changes requested.)