Repository: quora/pyanalyze
Pull request #371 (2021-12-28 10:53:38)

check for incompatible overrides


Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index d8f4121..5ea5194 100644
--- a/docs/changelog.md
+++ b/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)
diff --git a/pyanalyze/attributes.py b/pyanalyze/attributes.py
index 3c4317c..7b9a205 100644
--- a/pyanalyze/attributes.py
+++ b/pyanalyze/attributes.py
@@ -47,6 +47,8 @@ NoneType = type(None)
 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,7 +425,9 @@ 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
 
@@ -429,11 +435,12 @@ def _get_attribute_from_mro(
     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
 
diff --git a/pyanalyze/config.py b/pyanalyze/config.py
index 4a17227..4552183 100644
--- a/pyanalyze/config.py
+++ b/pyanalyze/config.py
@@ -13,7 +13,17 @@ import qcore
 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,21 +184,21 @@ class Config(object):
     # 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
@@ -196,7 +206,7 @@ class Config(object):
 
     # 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 @@ class Config(object):
     # 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 @@ class Config(object):
     # 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 @@ class Config(object):
 
     # 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
diff --git a/pyanalyze/error_code.py b/pyanalyze/error_code.py
index e734402..1351e67 100644
--- a/pyanalyze/error_code.py
+++ b/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 @@ DISABLED_BY_DEFAULT = {
     ErrorCode.bare_ignore,
     # TODO: turn this on
     ErrorCode.implicit_reexport,
+    ErrorCode.incompatible_override,
 }
 
 ERROR_DESCRIPTION = {
@@ -199,6 +201,7 @@ ERROR_DESCRIPTION = {
     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",
 }
 
 
diff --git a/pyanalyze/implementation.py b/pyanalyze/implementation.py
index e30cd75..4530caa 100644
--- a/pyanalyze/implementation.py
+++ b/pyanalyze/implementation.py
@@ -43,7 +43,6 @@ from .value import (
     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)
 
diff --git a/pyanalyze/name_check_visitor.py b/pyanalyze/name_check_visitor.py
index b9adb7e..3019fe1 100644
--- a/pyanalyze/name_check_visitor.py
+++ b/pyanalyze/name_check_visitor.py
@@ -137,6 +137,7 @@ from .value import (
     AnySource,
     AnyValue,
     CallableValue,
+    CanAssign,
     CanAssignError,
     ConstraintExtension,
     GenericBases,
@@ -237,7 +238,7 @@ class _StarredValue(Value):
         self.node = node
 
 
-@dataclass
+@dataclass(init=False)
 class _AttrContext(attributes.AttrContext):
     visitor: "NameCheckVisitor"
     node: Optional[ast.AST]
@@ -248,11 +249,16 @@ class _AttrContext(attributes.AttrContext):
         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 @@ class IgnoredEndOfReference(StringSequenceOption):
         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 @@ class NameCheckVisitor(
         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 @@ class NameCheckVisitor(
                 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 @@ class NameCheckVisitor(
         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 @@ class NameCheckVisitor(
         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
diff --git a/pyanalyze/options.py b/pyanalyze/options.py
index 20b76d0..00872b6 100644
--- a/pyanalyze/options.py
+++ b/pyanalyze/options.py
@@ -116,8 +116,6 @@ class BooleanOption(ConfigOption[bool]):
 
 
 class IntegerOption(ConfigOption[int]):
-    default_value = False
-
     @classmethod
     def parse(cls: "Type[IntegerOption]", data: object, source_path: Path) -> int:
         if isinstance(data, int):
@@ -156,7 +154,7 @@ class StringSequenceOption(ConcatenatedOption[str]):
 
 
 class PathSequenceOption(ConfigOption[Sequence[Path]]):
-    default_value = ()
+    default_value: ClassVar[Sequence[Path]] = ()
 
     @classmethod
     def parse(
@@ -172,7 +170,7 @@ class PathSequenceOption(ConfigOption[Sequence[Path]]):
 class PyObjectSequenceOption(ConfigOption[Sequence[T]]):
     """Represents a sequence of objects parsed as Python objects."""
 
-    default_value = ()
+    default_value: ClassVar[Sequence[T]] = ()
 
     @classmethod
     def parse(
@@ -238,6 +236,8 @@ class Options:
         fallback = option.get_fallback_option(self.fallback)
         if fallback is not None:
             instances = [*instances, fallback]
+        else:
+            instances = [*instances, option(option.default_value)]
         return option.get_value_from_instances(instances, self.module_path)
 
     def is_error_code_enabled(self, code: ErrorCode) -> bool:
diff --git a/pyproject.toml b/pyproject.toml
index 0a0859f..2cbfc98 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -39,3 +39,4 @@ unused_variable = true
 value_always_true = true
 suggested_parameter_type = true
 suggested_return_type = true
+incompatible_override = true


Test patch:
diff --git a/pyanalyze/test_attributes.py b/pyanalyze/test_attributes.py
index 6a59a1e..0a7c002 100644
--- a/pyanalyze/test_attributes.py
+++ b/pyanalyze/test_attributes.py
@@ -224,7 +224,7 @@ class TestAttributes(TestNameCheckVisitorBase):
         import enum
 
         class E(enum.Enum):
-            name = 1
+            name = 1  # E: incompatible_override
             no_name = 2
 
         def capybara():
diff --git a/pyanalyze/test_implementation.py b/pyanalyze/test_implementation.py
index c357d42..5c90b12 100644
--- a/pyanalyze/test_implementation.py
+++ b/pyanalyze/test_implementation.py
@@ -49,7 +49,7 @@ class TestSuperCall(TestNameCheckVisitorBase):
                 pass
 
         class Canaanimys(Gaudeamus):
-            def eat(self, grass):
+            def eat(self, grass=None):
                 super(Canaanimys, self).eat(grass)
 
     @assert_fails(ErrorCode.incompatible_call)
diff --git a/pyanalyze/test_name_check_visitor.py b/pyanalyze/test_name_check_visitor.py
index c682b46..ebcbae5 100644
--- a/pyanalyze/test_name_check_visitor.py
+++ b/pyanalyze/test_name_check_visitor.py
@@ -1667,17 +1667,20 @@ class TestSubscripting(TestNameCheckVisitorBase):
 
     @assert_passes()
     def test_permissive_subclass(self):
+        from typing import Any
+
         # Inspired by pyspark.sql.types.Row
         class LetItAllThrough(tuple):
-            def __getitem__(self, idx: object) -> object:
+            # TODO: make Sequence.__getitem__ args pos-only in typeshed
+            def __getitem__(self, idx: object) -> Any:  # E: incompatible_override
                 if isinstance(idx, (int, slice)):
                     return super().__getitem__(idx)
                 else:
                     return "whatever"
 
         def capybara(liat: LetItAllThrough) -> None:
-            assert_is_value(liat["x"], TypedValue(object))
-            assert_is_value(liat[0], TypedValue(object))
+            assert_is_value(liat["x"], AnyValue(AnySource.explicit))
+            assert_is_value(liat[0], AnyValue(AnySource.explicit))
 
     @assert_passes()
     def test_slice(self):
@@ -2652,3 +2655,29 @@ class TestCompare(TestNameCheckVisitorBase):
             if 1 < i < 3 != x:
                 assert_is_value(i, KnownValue(2))
                 assert_is_value(x, KnownValue(4))
+
+
+class TestIncompatibleOverride(TestNameCheckVisitorBase):
+    @assert_passes()
+    def test_simple(self):
+        from typing_extensions import Literal
+
+        class A:
+            x: str
+            y: int
+
+            def capybara(self, x: int) -> None:
+                pass
+
+            def pacarana(self, b: int) -> None:
+                pass
+
+        class B(A):
+            x: int  # E: incompatible_override
+            y: Literal[1]
+
+            def capybara(self, x: str) -> None:  # E: incompatible_override
+                pass
+
+            def pacarana(self, b: int) -> None:
+                pass