check for incompatible overrides

docs/changelog.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Add check for incompatible overrides in child classes
+  (#371)
 - Add `pyanalyze.extensions.NoReturnGuard` (#370)
 - Infer call signatures for `Type[X]` (#369)
 - Support configuration in a `pyproject.toml` file (#368)

pyanalyze/attributes.py

@@ -47,6 +47,8 @@
 class AttrContext:
     root_composite: Composite
     attr: str
+    skip_mro: bool = False
+    skip_unwrap: bool = False
 
     @property
     def root_value(self) -> Value:
@@ -161,7 +163,7 @@ def _get_attribute_from_subclass(typ: type, ctx: AttrContext) -> Value:
 
 
 def _unwrap_value_from_subclass(result: Value, ctx: AttrContext) -> Value:
-    if not isinstance(result, KnownValue):
+    if not isinstance(result, KnownValue) or ctx.skip_unwrap:
         return result
     cls_val = result.val
     if (
@@ -245,7 +247,7 @@ def _substitute_typevars(
 
 
 def _unwrap_value_from_typed(result: Value, typ: type, ctx: AttrContext) -> Value:
-    if not isinstance(result, KnownValue):
+    if not isinstance(result, KnownValue) or ctx.skip_unwrap:
         return result
     typevars = result.typevars if isinstance(result, KnownValueWithTypeVars) else None
     cls_val = result.val
@@ -400,6 +402,8 @@ def _get_attribute_from_mro(
                 and Generic in base_cls.mro()
             ):
                 continue
+            if ctx.skip_mro and base_cls is not typ:
+                continue
             try:
                 # Make sure to use only __annotations__ that are actually on this
                 # class, not ones inherited from a base class.
@@ -421,19 +425,22 @@ def _get_attribute_from_mro(
             except Exception:
                 pass
 
-            typeshed_type = ctx.get_attribute_from_typeshed(base_cls, on_class=on_class)
+            typeshed_type = ctx.get_attribute_from_typeshed(
+                base_cls, on_class=on_class or ctx.skip_unwrap
+            )
             if typeshed_type is not UNINITIALIZED_VALUE:
                 return typeshed_type, base_cls, False
 
     attrs_type = get_attrs_attribute(typ, ctx)
     if attrs_type is not None:
         return attrs_type, typ, False
 
-    # Even if we didn't find it any __dict__, maybe getattr() finds it directly.
-    try:
-        return KnownValue(getattr(typ, ctx.attr)), typ, True
-    except Exception:
-        pass
+    if not ctx.skip_mro:
+        # Even if we didn't find it any __dict__, maybe getattr() finds it directly.
+        try:
+            return KnownValue(getattr(typ, ctx.attr)), typ, True
+        except Exception:
+            pass
 
     return UNINITIALIZED_VALUE, object, False
 

pyanalyze/config.py

@@ -13,7 +13,17 @@
 from unittest import mock
 import asyncio
 from types import ModuleType
-from typing import Any, Callable, Dict, Set, TYPE_CHECKING, Union
+from typing import (
+    Any,
+    Callable,
+    Dict,
+    Mapping,
+    Sequence,
+    Set,
+    TYPE_CHECKING,
+    Tuple,
+    Union,
+)
 
 from . import value
 from .extensions import CustomCheck
@@ -174,29 +184,29 @@ def should_ignore_class_attribute(self, cls_val: object) -> bool:
     # Set of dunder methods (e.g., '{"__lshift__"}') that pyanalyze is not allowed to call on
     # objects.
     # name_check_visitor.DisallowCallsToDunders
-    DISALLOW_CALLS_TO_DUNDERS = set()
+    DISALLOW_CALLS_TO_DUNDERS: Set[str] = set()
 
     # Decorators that are equivalent to asynq.asynq and asynq.async_proxy.
     # name_check_visitor.AsynqDecorators
-    ASYNQ_DECORATORS = {asynq.asynq}
+    ASYNQ_DECORATORS: Set[object] = {asynq.asynq}
     # name_check_visitor.AsyncProxyDecorators
-    ASYNC_PROXY_DECORATORS = {asynq.async_proxy}
+    ASYNC_PROXY_DECORATORS: Set[object] = {asynq.async_proxy}
 
     # name_check_visitor.ComprehensionLengthInferenceLimit
     # If we iterate over something longer than this, we don't try to infer precise
     # types for comprehensions. Increasing this can hurt performance.
-    COMPREHENSION_LENGTH_INFERENCE_LIMIT = 25
+    COMPREHENSION_LENGTH_INFERENCE_LIMIT: int = 25
     # name_check_visitor.UnionSimplificationLimit
     # We may simplify unions with more than this many values.
-    UNION_SIMPLIFICATION_LIMIT = 100
+    UNION_SIMPLIFICATION_LIMIT: int = 100
 
     #
     # Used for VariableNameValue
     #
 
     # List of VariableNameValue instances that create pseudo-types associated with certain variable
     # names
-    VARIABLE_NAME_VALUES = []
+    VARIABLE_NAME_VALUES: Sequence[value.VariableNameValue] = []
 
     @qcore.caching.cached_per_instance()
     def varname_value_map(self) -> Dict[str, value.VariableNameValue]:
@@ -241,16 +251,16 @@ def should_ignore_unused(
     # Normally, async calls to async functions are only enforced in functions that are already
     # async. In subclasses of classes listed here, all async functions must be called async.
     # asynq_checker.ClassesCheckedForAsynq
-    BASE_CLASSES_CHECKED_FOR_ASYNQ = set()
+    BASE_CLASSES_CHECKED_FOR_ASYNQ: Set[type] = set()
 
     # Async batching in these component methods isn't checked even when they exist on a class in
     # BASE_CLASSES_CHECKED_FOR_ASYNQ
     # asynq_checker.MethodsNotCheckedForAsynq
-    METHODS_NOT_CHECKED_FOR_ASYNQ = set()
+    METHODS_NOT_CHECKED_FOR_ASYNQ: Set[str] = set()
 
     # We ignore _async methdos in these modules.
     # asynq_checker.NonAsynqModules
-    NON_ASYNQ_MODULES = {"multiprocessing"}
+    NON_ASYNQ_MODULES: Set[str] = {"multiprocessing"}
 
     #
     # Used by method_return_type.py
@@ -259,14 +269,14 @@ def should_ignore_unused(
     # A dictionary of {base class: {method name: expected return type}}
     # Use this to ensure that all subclasses of a certain type maintain the same return type for
     # their methods
-    METHOD_RETURN_TYPES = {}
+    METHOD_RETURN_TYPES: Mapping[type, Mapping[str, type]] = {}
 
     #
     # Used by ClassAttributeChecker
     #
 
     # When these attributes are unused, they are not listed as such by the unused attribute finder
-    IGNORED_UNUSED_ATTRS = {
+    IGNORED_UNUSED_ATTRS: Set[str] = {
         # ABCs
         "_abc_cache",
         "_abc_negative_cache",
@@ -286,13 +296,13 @@ def should_ignore_unused(
 
     # List of pairs of (class, set of attribute names). When these attribute names are seen as
     # unused on a child or base class of the class, they are not listed.
-    IGNORED_UNUSED_ATTRS_BY_CLASS = []
+    IGNORED_UNUSED_ATTRS_BY_CLASS: Sequence[Tuple[type, Set[str]]] = []
 
     # Used in the check for object attributes that are accessed but not set. In general, the check
     # will only alert about attributes that don't exist when it has visited all the base classes of
     # the class with the possibly missing attribute. However, these classes are never going to be
     # visited (since they're builtin), but they don't set any attributes that we rely on.
-    IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING = {object, abc.ABC}
+    IGNORED_TYPES_FOR_ATTRIBUTE_CHECKING: Set[type] = {object, abc.ABC}
 
     #
     # Used by arg_spec.py

pyanalyze/error_code.py

@@ -87,6 +87,7 @@ class ErrorCode(enum.Enum):
     invalid_context_manager = 70
     suggested_return_type = 71
     suggested_parameter_type = 72
+    incompatible_override = 73
 
 
 # Allow testing unannotated functions without too much fuss
@@ -111,6 +112,7 @@ class ErrorCode(enum.Enum):
     ErrorCode.bare_ignore,
     # TODO: turn this on
     ErrorCode.implicit_reexport,
+    ErrorCode.incompatible_override,
 }
 
 ERROR_DESCRIPTION = {
@@ -199,6 +201,7 @@ class ErrorCode(enum.Enum):
     ErrorCode.invalid_context_manager: "Use of invalid object in with or async with",
     ErrorCode.suggested_return_type: "Suggested return type",
     ErrorCode.suggested_parameter_type: "Suggested parameter type",
+    ErrorCode.incompatible_override: "Class attribute incompatible with base class",
 }
 
 

pyanalyze/implementation.py

@@ -43,7 +43,6 @@
     KnownValue,
     MultiValuedValue,
     KNOWN_MUTABLE_TYPES,
-    UnboundMethodValue,
     Value,
     WeakExtension,
     concrete_values_from_iterable,
@@ -926,15 +925,7 @@ def _assert_is_value_impl(ctx: CallContext) -> Value:
 def _reveal_type_impl(ctx: CallContext) -> Value:
     if ctx.visitor._is_checking():
         value = ctx.vars["value"]
-        message = f"Revealed type is '{value!s}'"
-        if isinstance(value, KnownValue):
-            sig = ctx.visitor.arg_spec_cache.get_argspec(value.val)
-        elif isinstance(value, UnboundMethodValue):
-            sig = value.get_signature(ctx.visitor)
-        else:
-            sig = None
-        if sig is not None:
-            message += f", signature is {sig!s}"
+        message = f"Revealed type is {ctx.visitor.display_value(value)}"
         ctx.show_error(message, ErrorCode.inference_failure, arg="value")
     return KnownValue(None)
 

pyanalyze/name_check_visitor.py

@@ -137,6 +137,7 @@
     AnySource,
     AnyValue,
     CallableValue,
+    CanAssign,
     CanAssignError,
     ConstraintExtension,
     GenericBases,
@@ -237,7 +238,7 @@ def __init__(self, value: Value, node: ast.AST) -> None:
         self.node = node
 
 
-@dataclass
+@dataclass(init=False)
 class _AttrContext(attributes.AttrContext):
     visitor: "NameCheckVisitor"
     node: Optional[ast.AST]
@@ -248,11 +249,16 @@ def __init__(
         self,
         root_composite: Composite,
         attr: str,
-        node: Optional[ast.AST],
         visitor: "NameCheckVisitor",
-        ignore_none: bool,
+        *,
+        node: Optional[ast.AST] = None,
+        ignore_none: bool = False,
+        skip_mro: bool = False,
+        skip_unwrap: bool = False,
     ) -> None:
-        super().__init__(root_composite, attr)
+        super().__init__(
+            root_composite, attr, skip_mro=skip_mro, skip_unwrap=skip_unwrap
+        )
         self.node = node
         self.visitor = visitor
         self.ignore_none = ignore_none
@@ -469,6 +475,13 @@ def get_value_from_fallback(cls, fallback: Config) -> Sequence[str]:
         return list(fallback.IGNORED_END_OF_REFERENCE)
 
 
+class IgnoredForIncompatibleOverride(StringSequenceOption):
+    """These attributes are not checked for incompatible overrides."""
+
+    name = "ignored_for_incompatible_overrides"
+    default_value = ["__init__", "__eq__", "__ne__"]
+
+
 class ClassAttributeChecker:
     """Helper class to keep track of attributes that are read and set on instances."""
 
@@ -1001,7 +1014,7 @@ def __init__(
         self._has_used_any_match = False
         self._fill_method_cache()
 
-    def make_type_object(self, typ: Union[type, super]) -> TypeObject:
+    def make_type_object(self, typ: Union[type, super, str]) -> TypeObject:
         return self.checker.make_type_object(typ)
 
     def can_assume_compatibility(self, left: TypeObject, right: TypeObject) -> bool:
@@ -1230,10 +1243,77 @@ def _set_name_in_scope(
                 value, _ = current_scope.get_local(varname, node, self.state)
                 return value
         if scope_type == ScopeType.class_scope and isinstance(node, ast.AST):
+            self._check_for_incompatible_overrides(varname, node, value)
             self._check_for_class_variable_redefinition(varname, node)
         current_scope.set(varname, value, node, self.state)
         return value
 
+    def _check_for_incompatible_overrides(
+        self, varname: str, node: ast.AST, value: Value
+    ) -> None:
+        if self.current_class is None:
+            return
+        if varname in self.options.get_value_for(IgnoredForIncompatibleOverride):
+            return
+        for base_class in self.get_generic_bases(self.current_class):
+            if base_class is self.current_class:
+                continue
+            base_class_value = TypedValue(base_class)
+            ctx = _AttrContext(
+                Composite(base_class_value),
+                varname,
+                self,
+                skip_mro=True,
+                skip_unwrap=True,
+            )
+            base_value = attributes.get_attribute(ctx)
+            can_assign = self._can_assign_to_base(base_value, value)
+            if isinstance(can_assign, CanAssignError):
+                error = CanAssignError(
+                    children=[
+                        CanAssignError(f"Base class: {self.display_value(base_value)}"),
+                        CanAssignError(f"Child class: {self.display_value(value)}"),
+                        can_assign,
+                    ]
+                )
+                self._show_error_if_checking(
+                    node,
+                    f"Value of {varname} incompatible with base class {base_class}",
+                    ErrorCode.incompatible_override,
+                    detail=str(error),
+                )
+
+    def display_value(self, value: Value) -> str:
+        message = f"'{value!s}'"
+        if isinstance(value, KnownValue):
+            sig = self.arg_spec_cache.get_argspec(value.val)
+        elif isinstance(value, UnboundMethodValue):
+            sig = value.get_signature(self)
+        else:
+            sig = None
+        if sig is not None:
+            message += f", signature is {sig!s}"
+        return message
+
+    def _can_assign_to_base(self, base_value: Value, child_value: Value) -> CanAssign:
+        if base_value is UNINITIALIZED_VALUE:
+            return {}
+        if isinstance(base_value, KnownValue) and callable(base_value.val):
+            base_sig = self.signature_from_value(base_value)
+            if not isinstance(base_sig, (Signature, OverloadedSignature)):
+                return {}
+            child_sig = self.signature_from_value(child_value)
+            if not isinstance(child_sig, (Signature, OverloadedSignature)):
+                return CanAssignError(f"{child_value} is not callable")
+            base_bound = base_sig.bind_self()
+            if base_bound is None:
+                return {}
+            child_bound = child_sig.bind_self()
+            if child_bound is None:
+                return CanAssignError(f"{child_value} is missing a 'self' argument")
+            return base_bound.can_assign(child_bound, self)
+        return base_value.can_assign(child_value, self)
+
     def _check_for_class_variable_redefinition(
         self, varname: str, node: ast.AST
     ) -> None:
@@ -4091,7 +4171,9 @@ def _get_attribute_no_mvv(
         ignore_none: bool = False,
     ) -> Value:
         """Get an attribute. root_value must not be a MultiValuedValue."""
-        ctx = _AttrContext(root_composite, attr, node, self, ignore_none)
+        ctx = _AttrContext(
+            root_composite, attr, self, node=node, ignore_none=ignore_none
+        )
         return attributes.get_attribute(ctx)
 
     def _get_attribute_with_fallback(
@@ -4677,7 +4759,9 @@ def prepare_constructor_kwargs(cls, kwargs: Mapping[str, Any]) -> Mapping[str, A
         kwargs.setdefault("checker", Checker(cls.config, options))
         return kwargs
 
-    def is_enabled(self, error_code: ErrorCode) -> bool:
+    def is_enabled(self, error_code: enum.Enum) -> bool:
+        if not isinstance(error_code, ErrorCode):
+            return False
         return self.options.is_error_code_enabled(error_code)
 
     @classmethod