Skip to content

Commit

Permalink
Support properties with setter type different from getter type (#18510)
Browse files Browse the repository at this point in the history
Fixes #3004
Fixes #11892
Fixes #12892
Fixes #14301

_Note:_ this PR should be reviewed with "hide whitespace" option (in
couple long functions I replace huge `if x: ...` with `if not x: return;
...` to reduce indent level).

The core logic is quite straightforward (almost trivial). The only
couple things are:
* We should be careful with binder (we simpy can't use it for properties
with setter type different from getter type, since we don't know
underlying property implementation)
* We need to handle gracefully existing settable properties that are
generated by plugins

The tricky part is subclassing and protocols. The summary is as
following:
* For protocols I simply implement everything the "correct way", i.e.
for settable attributes (whether it is a variable or a settable
property) compare getter types covariantly and setter types
contravariantly. The tricky part here is generating meaningful error
messages that are also not too verbose.
* For subclassing I cannot simply do the same, because there is a flag
about covariant mutable override, that is off by default. So instead
what I do is if either subclass node, or superclass node is a "custom
property" (i.e. a property with setter type different from getter type),
then I use the "correct way", otherwise the old logic (i.e. flag
dependent check) is used.

Two things that are not implemented are multiple inheritance, and new
generic syntax (inferred variance). In these cases setter types are
simply ignored. There is nothing conceptually difficult about these, I
simply run out of steam (and the PR is getting big). I left `TODO`s in
code for these. In most cases these will generate false negatives (and
they are already kind of corner cases) so I think it is OK to postpone
these indefinitely.
  • Loading branch information
ilevkivskyi authored Jan 25, 2025
1 parent 3ced11a commit 1eb9d4c
Show file tree
Hide file tree
Showing 15 changed files with 1,077 additions and 233 deletions.
550 changes: 356 additions & 194 deletions mypy/checker.py

Large diffs are not rendered by default.

18 changes: 15 additions & 3 deletions mypy/checkmember.py
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,10 @@ def analyze_descriptor_access(
if isinstance(descriptor_type, UnionType):
# Map the access over union types
return make_simplified_union(
[analyze_descriptor_access(typ, mx) for typ in descriptor_type.items]
[
analyze_descriptor_access(typ, mx, assignment=assignment)
for typ in descriptor_type.items
]
)
elif not isinstance(descriptor_type, Instance):
return orig_descriptor_type
Expand Down Expand Up @@ -776,7 +779,13 @@ def analyze_var(
# Found a member variable.
original_itype = itype
itype = map_instance_to_supertype(itype, var.info)
typ = var.type
if var.is_settable_property and mx.is_lvalue:
typ: Type | None = var.setter_type
if typ is None and var.is_ready:
# Existing synthetic properties may not set setter type. Fall back to getter.
typ = var.type
else:
typ = var.type
if typ:
if isinstance(typ, PartialType):
return mx.chk.handle_partial_var_type(typ, mx.is_lvalue, var, mx.context)
Expand Down Expand Up @@ -834,7 +843,10 @@ def analyze_var(
if var.is_property:
# A property cannot have an overloaded type => the cast is fine.
assert isinstance(expanded_signature, CallableType)
result = expanded_signature.ret_type
if var.is_settable_property and mx.is_lvalue and var.setter_type is not None:
result = expanded_signature.arg_types[0]
else:
result = expanded_signature.ret_type
else:
result = expanded_signature
else:
Expand Down
7 changes: 5 additions & 2 deletions mypy/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
codes.OVERRIDE,
}

allowed_duplicates: Final = ["@overload", "Got:", "Expected:"]
allowed_duplicates: Final = ["@overload", "Got:", "Expected:", "Expected setter type:"]

BASE_RTD_URL: Final = "https://mypy.rtfd.io/en/stable/_refs.html#code"

Expand Down Expand Up @@ -172,10 +172,12 @@ def __init__(
*,
filter_errors: bool | Callable[[str, ErrorInfo], bool] = False,
save_filtered_errors: bool = False,
filter_deprecated: bool = False,
) -> None:
self.errors = errors
self._has_new_errors = False
self._filter = filter_errors
self._filter_deprecated = filter_deprecated
self._filtered: list[ErrorInfo] | None = [] if save_filtered_errors else None

def __enter__(self) -> ErrorWatcher:
Expand All @@ -196,7 +198,8 @@ def on_error(self, file: str, info: ErrorInfo) -> bool:
ErrorWatcher further down the stack and from being recorded by Errors
"""
if info.code == codes.DEPRECATED:
return False
# Deprecated is not a type error, so it is handled on opt-in basis here.
return self._filter_deprecated

self._has_new_errors = True
if isinstance(self._filter, bool):
Expand Down
2 changes: 2 additions & 0 deletions mypy/fixup.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,8 @@ def visit_var(self, v: Var) -> None:
v.info = self.current_info
if v.type is not None:
v.type.accept(self.type_fixer)
if v.setter_type is not None:
v.setter_type.accept(self.type_fixer)

def visit_type_alias(self, a: TypeAlias) -> None:
a.target.accept(self.type_fixer)
Expand Down
72 changes: 61 additions & 11 deletions mypy/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
from mypy.subtypes import (
IS_CLASS_OR_STATIC,
IS_CLASSVAR,
IS_EXPLICIT_SETTER,
IS_SETTABLE,
IS_VAR,
find_member,
Expand Down Expand Up @@ -186,9 +187,13 @@ def filter_errors(
*,
filter_errors: bool | Callable[[str, ErrorInfo], bool] = True,
save_filtered_errors: bool = False,
filter_deprecated: bool = False,
) -> ErrorWatcher:
return ErrorWatcher(
self.errors, filter_errors=filter_errors, save_filtered_errors=save_filtered_errors
self.errors,
filter_errors=filter_errors,
save_filtered_errors=save_filtered_errors,
filter_deprecated=filter_deprecated,
)

def add_errors(self, errors: list[ErrorInfo]) -> None:
Expand Down Expand Up @@ -1164,6 +1169,20 @@ def overload_signature_incompatible_with_supertype(
note_template = 'Overload variants must be defined in the same order as they are in "{}"'
self.note(note_template.format(supertype), context, code=codes.OVERRIDE)

def incompatible_setter_override(
self, defn: Context, typ: Type, original_type: Type, base: TypeInfo
) -> None:
self.fail("Incompatible override of a setter type", defn, code=codes.OVERRIDE)
base_str, override_str = format_type_distinctly(original_type, typ, options=self.options)
self.note(
f' (base class "{base.name}" defined the type as {base_str},',
defn,
code=codes.OVERRIDE,
)
self.note(f" override has type {override_str})", defn, code=codes.OVERRIDE)
if is_subtype(typ, original_type):
self.note(" Setter types should behave contravariantly", defn, code=codes.OVERRIDE)

def signature_incompatible_with_supertype(
self,
name: str,
Expand Down Expand Up @@ -2201,22 +2220,34 @@ def report_protocol_problems(
):
type_name = format_type(subtype, self.options, module_names=True)
self.note(f"Following member(s) of {type_name} have conflicts:", context, code=code)
for name, got, exp in conflict_types[:MAX_ITEMS]:
for name, got, exp, is_lvalue in conflict_types[:MAX_ITEMS]:
exp = get_proper_type(exp)
got = get_proper_type(got)
setter_suffix = " setter type" if is_lvalue else ""
if not isinstance(exp, (CallableType, Overloaded)) or not isinstance(
got, (CallableType, Overloaded)
):
self.note(
"{}: expected {}, got {}".format(
name, *format_type_distinctly(exp, got, options=self.options)
"{}: expected{} {}, got {}".format(
name,
setter_suffix,
*format_type_distinctly(exp, got, options=self.options),
),
context,
offset=OFFSET,
code=code,
)
if is_lvalue and is_subtype(got, exp, options=self.options):
self.note(
"Setter types should behave contravariantly",
context,
offset=OFFSET,
code=code,
)
else:
self.note("Expected:", context, offset=OFFSET, code=code)
self.note(
"Expected{}:".format(setter_suffix), context, offset=OFFSET, code=code
)
if isinstance(exp, CallableType):
self.note(
pretty_callable(exp, self.options, skip_self=class_obj or is_module),
Expand Down Expand Up @@ -3029,12 +3060,12 @@ def get_missing_protocol_members(left: Instance, right: Instance, skip: list[str

def get_conflict_protocol_types(
left: Instance, right: Instance, class_obj: bool = False, options: Options | None = None
) -> list[tuple[str, Type, Type]]:
) -> list[tuple[str, Type, Type, bool]]:
"""Find members that are defined in 'left' but have incompatible types.
Return them as a list of ('member', 'got', 'expected').
Return them as a list of ('member', 'got', 'expected', 'is_lvalue').
"""
assert right.type.is_protocol
conflicts: list[tuple[str, Type, Type]] = []
conflicts: list[tuple[str, Type, Type, bool]] = []
for member in right.type.protocol_members:
if member in ("__init__", "__new__"):
continue
Expand All @@ -3044,10 +3075,29 @@ def get_conflict_protocol_types(
if not subtype:
continue
is_compat = is_subtype(subtype, supertype, ignore_pos_arg_names=True, options=options)
if IS_SETTABLE in get_member_flags(member, right):
is_compat = is_compat and is_subtype(supertype, subtype, options=options)
if not is_compat:
conflicts.append((member, subtype, supertype))
conflicts.append((member, subtype, supertype, False))
superflags = get_member_flags(member, right)
if IS_SETTABLE not in superflags:
continue
different_setter = False
if IS_EXPLICIT_SETTER in superflags:
set_supertype = find_member(member, right, left, is_lvalue=True)
if set_supertype and not is_same_type(set_supertype, supertype):
different_setter = True
supertype = set_supertype
if IS_EXPLICIT_SETTER in get_member_flags(member, left):
set_subtype = mypy.typeops.get_protocol_member(left, member, class_obj, is_lvalue=True)
if set_subtype and not is_same_type(set_subtype, subtype):
different_setter = True
subtype = set_subtype
if not is_compat and not different_setter:
# We already have this conflict listed, avoid duplicates.
continue
assert supertype is not None and subtype is not None
is_compat = is_subtype(supertype, subtype, options=options)
if not is_compat:
conflicts.append((member, subtype, supertype, different_setter))
return conflicts


Expand Down
15 changes: 15 additions & 0 deletions mypy/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -977,6 +977,7 @@ class Var(SymbolNode):
"_fullname",
"info",
"type",
"setter_type",
"final_value",
"is_self",
"is_cls",
Expand Down Expand Up @@ -1011,6 +1012,8 @@ def __init__(self, name: str, type: mypy.types.Type | None = None) -> None:
# TODO: Should be Optional[TypeInfo]
self.info = VAR_NO_INFO
self.type: mypy.types.Type | None = type # Declared or inferred type, or None
# The setter type for settable properties.
self.setter_type: mypy.types.CallableType | None = None
# Is this the first argument to an ordinary method (usually "self")?
self.is_self = False
# Is this the first argument to a classmethod (typically "cls")?
Expand Down Expand Up @@ -1076,6 +1079,7 @@ def serialize(self) -> JsonDict:
"name": self._name,
"fullname": self._fullname,
"type": None if self.type is None else self.type.serialize(),
"setter_type": None if self.setter_type is None else self.setter_type.serialize(),
"flags": get_flags(self, VAR_FLAGS),
}
if self.final_value is not None:
Expand All @@ -1087,7 +1091,18 @@ def deserialize(cls, data: JsonDict) -> Var:
assert data[".class"] == "Var"
name = data["name"]
type = None if data["type"] is None else mypy.types.deserialize_type(data["type"])
setter_type = (
None
if data["setter_type"] is None
else mypy.types.deserialize_type(data["setter_type"])
)
v = Var(name, type)
assert (
setter_type is None
or isinstance(setter_type, mypy.types.ProperType)
and isinstance(setter_type, mypy.types.CallableType)
)
v.setter_type = setter_type
v.is_ready = False # Override True default set in __init__
v._fullname = data["fullname"]
set_flags(v, data["flags"])
Expand Down
6 changes: 6 additions & 0 deletions mypy/server/astdiff.py
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,11 @@ def snapshot_definition(node: SymbolNode | None, common: SymbolSnapshot) -> Symb
impl = node
elif isinstance(node, OverloadedFuncDef) and node.impl:
impl = node.impl.func if isinstance(node.impl, Decorator) else node.impl
setter_type = None
if isinstance(node, OverloadedFuncDef) and node.items:
first_item = node.items[0]
if isinstance(first_item, Decorator) and first_item.func.is_property:
setter_type = snapshot_optional_type(first_item.var.setter_type)
is_trivial_body = impl.is_trivial_body if impl else False
dataclass_transform_spec = find_dataclass_transform_spec(node)
return (
Expand All @@ -258,6 +263,7 @@ def snapshot_definition(node: SymbolNode | None, common: SymbolSnapshot) -> Symb
is_trivial_body,
dataclass_transform_spec.serialize() if dataclass_transform_spec is not None else None,
node.deprecated if isinstance(node, FuncDef) else None,
setter_type, # multi-part properties are stored as OverloadedFuncDef
)
elif isinstance(node, Var):
return ("Var", common, snapshot_optional_type(node.type), node.is_final)
Expand Down
1 change: 1 addition & 0 deletions mypy/server/astmerge.py
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,7 @@ def visit_enum_call_expr(self, node: EnumCallExpr) -> None:
def visit_var(self, node: Var) -> None:
node.info = self.fixup(node.info)
self.fixup_type(node.type)
self.fixup_type(node.setter_type)
super().visit_var(node)

def visit_type_alias(self, node: TypeAlias) -> None:
Expand Down
Loading

0 comments on commit 1eb9d4c

Please sign in to comment.