Skip to content

Commit

Permalink
check incompatible field overrides
Browse files Browse the repository at this point in the history
Summary:
We weren't checking for incompatible field overrides at all. We were still sound,
because we actually just ignored field overrides entirely and went with the base
field type, but it's an odd user experience.

In principle we could allow type narrowing of attributes on subclasses, only if the
attribute is `Final`. But I think this would require changes in the runtime.

Reviewed By: DinoV

Differential Revision: D31177531

fbshipit-source-id: 7cd9742
  • Loading branch information
Carl Meyer authored and facebook-github-bot committed Sep 24, 2021
1 parent 8cbc640 commit 1475ebd
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 10 deletions.
25 changes: 15 additions & 10 deletions Lib/compiler/static/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -1118,13 +1118,22 @@ def is_subclass_of(self, src: Class) -> bool:
return any(self.is_subclass_of(t) for t in src.type_args)
return src.inexact_type() in self.mro_inexact

def incompatible_override(self, override: Value, inherited: Value) -> bool:
def check_incompatible_override(self, override: Value, inherited: Value) -> None:
# TODO: There's more checking we should be doing to ensure
# this is a compatible override
return type(override) != type(inherited) and (
if type(override) != type(inherited) and (
type(override) is not Function
or not isinstance(inherited, (BuiltinFunction, BuiltinMethodDescriptor))
)
):
raise TypedSyntaxError(f"class cannot hide inherited member: {inherited!r}")
if isinstance(override, Slot) and isinstance(inherited, Slot):
# TODO we could allow covariant type overrides for Final attributes
ot = override.type_ref
it = inherited.type_ref
if ot and it and ot.resolved(True) != (itr := it.resolved(True)):
raise TypedSyntaxError(
f"Cannot change type of inherited attribute (inherited type '{itr.instance.name}')"
)

def finish_bind(self, module: ModuleTable) -> None:
inherited = set()
Expand All @@ -1143,13 +1152,9 @@ def finish_bind(self, module: ModuleTable) -> None:

for base in self.mro[1:]:
value = base.members.get(name)
if value is not None and self.incompatible_override(
my_value, value
):
raise TypedSyntaxError(
f"class cannot hide inherited member: {value!r}"
)
elif isinstance(value, Class):
if value is not None:
self.check_incompatible_override(my_value, value)
if isinstance(value, Class):
value.finish_bind(module)
elif isinstance(value, Slot):
inherited.add(name)
Expand Down
26 changes: 26 additions & 0 deletions Lib/test/test_compiler/test_static/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -715,3 +715,29 @@ class C:
r"Class Finals are inferred ClassVar; do not nest with Final",
at="ClassVar[int]",
)

def test_incompatible_attr_override(self):
self.type_error(
"""
class A:
x: int
class B(A):
x: str
""",
r"Cannot change type of inherited attribute \(inherited type 'int'\)",
at="x: str",
)

def test_mutable_attr_invariant(self):
self.type_error(
"""
class A:
x: object
class B(A):
x: int
""",
r"Cannot change type of inherited attribute \(inherited type 'object'\)",
at="x: int",
)

0 comments on commit 1475ebd

Please sign in to comment.