Repository: python/mypy
Pull request: Check Liskov compatibility in fine-grained mode
Fixes #4960 
Fixes #4853 

The first issue is fixed by:
* Defining a _new_ `Var` if there is a `self` assignment with an explicit type in a subclass.
* Applying `check_compatibility_super` to `RefExpr` lvalues, not only `NameExpr`.

The second issue is fixed by adding two dependencies:
* If a variable is defined in class, recheck the current target (without recursing to methods).
* If a variable is defined via `self` in a method, recheck the defining method only.
* The above two are only added for classes with user-defined bases (this allows to avoid trivial deps that would only "pollute" many tests, but are useless in practice).

There was one more thing I tried to fix, there seems to be an inconsistency that lvalues corresponding to `self` definitions have their kind set to `None` instead of `MDEF` (like for names defined in class). But I abandoned this because some code relies on this. I think we can clean this in an upcoming refactoring.

Problem statement:
Liskov is not rechecked in fine grained if an attribute type is updated
This test doesn't show show any errors on the second run
```
[case testLiskovRechecked]
import b
class A(b.B):
    x: str
def f(x: b.B) -> None:
    x.x + int()
f(A())
[file b.py]
class B:
    x: str
[file b.py.2]
class B:
    x: int
[out]
```
Although it is not safe, in particular, `A.x` is incompatible with `B.x`. This actually looks hard, because I don't see yet how to fix this without a visible performance regression.

_UPDATE:_ Simplified the test case.

Redefinitions on self are ignored
Consider this code:
```python
class B:
    x: int

class A(B):
    def meth(self) -> None:
        self.x: str  # No error, although Liskov is violated

reveal_type(B.x)  # builtins.int
reveal_type(A.x)  # builtins.int
```
It looks like assignment on `self` does not create a new `Var`, which is I believe inconsistent.

Hints from issue discussion:
Here the idea is to add a dependency like `<m.C.x> -> m` to just recheck the assignment without recursing into methods. We need however to check that Liskov is also verified for methods after an update, this may require different dependency targets depending on whether this is an attribute or method. (We need to be especially careful with decorators, Liskov should be checked in the outer scope for them, i.e. after decorating.)
Just to clarify, I think only assignments on `self` with an _explicit type_ should create a new `Var`.

Commit message: Add dedicated tests

Patch to review:
diff --git a/mypy/checker.py b/mypy/checker.py
index 99ef6c8e6bd6..b108a8820e5d 100644
--- a/mypy/checker.py
+++ b/mypy/checker.py
@@ -1469,7 +1469,7 @@ def check_assignment(self, lvalue: Lvalue, rvalue: Expression, infer_lvalue_type
         else:
             lvalue_type, index_lvalue, inferred = self.check_lvalue(lvalue)
 
-            if isinstance(lvalue, NameExpr):
+            if isinstance(lvalue, RefExpr):
                 if self.check_compatibility_all_supers(lvalue, lvalue_type, rvalue):
                     # We hit an error on this line; don't check for any others
                     return
@@ -1534,13 +1534,12 @@ def check_assignment(self, lvalue: Lvalue, rvalue: Expression, infer_lvalue_type
                 self.infer_variable_type(inferred, lvalue, self.expr_checker.accept(rvalue),
                                          rvalue)
 
-    def check_compatibility_all_supers(self, lvalue: NameExpr, lvalue_type: Optional[Type],
+    def check_compatibility_all_supers(self, lvalue: RefExpr, lvalue_type: Optional[Type],
                                        rvalue: Expression) -> bool:
         lvalue_node = lvalue.node
-
         # Check if we are a class variable with at least one base class
         if (isinstance(lvalue_node, Var) and
-                lvalue.kind == MDEF and
+                lvalue.kind in (MDEF, None) and  # None for Vars defined via self
                 len(lvalue_node.info.bases) > 0):
 
             for base in lvalue_node.info.mro[1:]:
@@ -1578,7 +1577,7 @@ def check_compatibility_all_supers(self, lvalue: NameExpr, lvalue_type: Optional
                     break
         return False
 
-    def check_compatibility_super(self, lvalue: NameExpr, lvalue_type: Optional[Type],
+    def check_compatibility_super(self, lvalue: RefExpr, lvalue_type: Optional[Type],
                                   rvalue: Expression, base: TypeInfo, base_type: Type,
                                   base_node: Node) -> bool:
         lvalue_node = lvalue.node
diff --git a/mypy/semanal.py b/mypy/semanal.py
index a8469ae53ba3..6b1d6fe9afc4 100644
--- a/mypy/semanal.py
+++ b/mypy/semanal.py
@@ -1854,7 +1854,7 @@ def analyze_lvalue(self, lval: Lvalue, nested: bool = False,
                 self.check_lvalue_validity(lval.node, lval)
         elif isinstance(lval, MemberExpr):
             if not add_global:
-                self.analyze_member_lvalue(lval)
+                self.analyze_member_lvalue(lval, explicit_type)
             if explicit_type and not self.is_self_member_ref(lval):
                 self.fail('Type cannot be declared in assignment to non-self '
                           'attribute', lval)
@@ -1892,12 +1892,14 @@ def analyze_tuple_or_list_lvalue(self, lval: TupleExpr,
                 self.analyze_lvalue(i, nested=True, add_global=add_global,
                                     explicit_type = explicit_type)
 
-    def analyze_member_lvalue(self, lval: MemberExpr) -> None:
+    def analyze_member_lvalue(self, lval: MemberExpr, explicit_type: bool = False) -> None:
         lval.accept(self)
         if self.is_self_member_ref(lval):
             assert self.type, "Self member outside a class"
+            cur_node = self.type.names.get(lval.name, None)
             node = self.type.get(lval.name)
-            if node is None or isinstance(node.node, Var) and node.node.is_abstract_var:
+            if (node is None or isinstance(node.node, Var) and node.node.is_abstract_var or
+                    cur_node is None and explicit_type):
                 if self.type.is_protocol and node is None:
                     self.fail("Protocol members cannot be defined via assignment to self", lval)
                 else:
@@ -1911,6 +1913,7 @@ def analyze_member_lvalue(self, lval: MemberExpr) -> None:
                     v.is_ready = False
                     lval.def_var = v
                     lval.node = v
+                    # TODO: should we also set lval.kind = MDEF?
                     self.type.names[lval.name] = SymbolTableNode(MDEF, v, implicit=True)
         self.check_lvalue_validity(lval.node, lval)
 
diff --git a/mypy/server/deps.py b/mypy/server/deps.py
index 9befbda04724..c7ff4c0aa36a 100644
--- a/mypy/server/deps.py
+++ b/mypy/server/deps.py
@@ -239,6 +239,9 @@ def process_type_info(self, info: TypeInfo) -> None:
         self.add_type_alias_deps(self.scope.current_target())
         for name, node in info.names.items():
             if isinstance(node.node, Var):
+                # Recheck Liskov if needed, self definitions are checked in the defining method
+                if node.node.is_initialized_in_class and user_bases(info):
+                    self.add_dependency(make_trigger(info.fullname() + '.' + name))
                 for base_info in non_trivial_bases(info):
                     # If the type of an attribute changes in a base class, we make references
                     # to the attribute in the subclass stale.
@@ -353,6 +356,13 @@ def process_lvalue(self, lvalue: Expression) -> None:
                 for type_trigger in type_triggers:
                     self.add_dependency(type_trigger, attr_trigger)
         elif isinstance(lvalue, MemberExpr):
+            if self.is_self_member_ref(lvalue) and lvalue.is_new_def:
+                node = lvalue.node
+                if isinstance(node, Var):
+                    info = node.info
+                    if info and user_bases(info):
+                        # Recheck Liskov for self definitions
+                        self.add_dependency(make_trigger(info.fullname() + '.' + lvalue.name))
             if lvalue.kind is None:
                 # Reference to a non-module attribute
                 if lvalue.expr not in self.type_map:
@@ -369,6 +379,13 @@ def process_lvalue(self, lvalue: Expression) -> None:
                 self.process_lvalue(item)
         # TODO: star lvalue
 
+    def is_self_member_ref(self, memberexpr: MemberExpr) -> bool:
+        """Does memberexpr to refer to an attribute of self?"""
+        if not isinstance(memberexpr.expr, NameExpr):
+            return False
+        node = memberexpr.expr.node
+        return isinstance(node, Var) and node.is_self
+
     def get_non_partial_lvalue_type(self, lvalue: RefExpr) -> Type:
         if lvalue not in self.type_map:
             # Likely a block considered unreachable during type checking.
@@ -742,6 +759,12 @@ def non_trivial_bases(info: TypeInfo) -> List[TypeInfo]:
             if base.fullname() != 'builtins.object']
 
 
+def user_bases(info: TypeInfo) -> List[TypeInfo]:
+    # TODO: skip everything from typeshed?
+    return [base for base in info.mro[1:]
+            if base.module_name not in ('builtins', 'typing', 'enum')]
+
+
 def dump_all_dependencies(modules: Dict[str, MypyFile],
                           type_map: Dict[Expression, Type],
                           python_version: Tuple[int, int]) -> None:
diff --git a/test-data/unit/deps.test b/test-data/unit/deps.test
index 8efa3b9477f5..dab6fc1bf5b2 100644
--- a/test-data/unit/deps.test
+++ b/test-data/unit/deps.test
@@ -1024,3 +1024,45 @@ class B(Base):
 <mod.B> -> <m.f>, m.f
 <mod.Base> -> <m.f>, m.f
 <mod> -> m
+
+[case testDepsLiskovClass]
+from mod import A, C
+class D(C):
+    x: A
+[file mod.py]
+class C:
+    x: B
+class B:
+    pass
+class A(B):
+    pass
+[out]
+<m.D.x> -> m
+<m.D> -> m.D
+<mod.A> -> <m.D.x>, m
+<mod.C.__init__> -> <m.D.__init__>
+<mod.C.__new__> -> <m.D.__new__>
+<mod.C.x> -> <m.D.x>
+<mod.C> -> m, m.D
+
+[case testDepsLiskovMethod]
+from mod import A, C
+class D(C):
+    def __init__(self) -> None:
+        self.x: A
+[file mod.py]
+class C:
+    def __init__(self) -> None:
+        self.x: B
+class B:
+    pass
+class A(B):
+    pass
+[out]
+<m.D.x> -> m.D.__init__
+<m.D> -> m.D
+<mod.A> -> <m.D.x>, m, m.D.__init__
+<mod.C.__init__> -> <m.D.__init__>, m.D.__init__
+<mod.C.__new__> -> <m.D.__new__>
+<mod.C.x> -> <m.D.x>
+<mod.C> -> m, m.D
diff --git a/test-data/unit/fine-grained.test b/test-data/unit/fine-grained.test
index f05ab4be56d2..7a45929eb904 100644
--- a/test-data/unit/fine-grained.test
+++ b/test-data/unit/fine-grained.test
@@ -5800,3 +5800,185 @@ class M(type):
 [out]
 ==
 a.py:2: error: Argument 1 to "f" of "M" has incompatible type "int"; expected "str"
+
+[case testLiskovFineVariable]
+import b
+class A(b.B):
+    x: str
+def f(x: b.B) -> None:
+    x.x + int()
+f(A())
+[file b.py]
+class B:
+    x: str
+[file b.py.2]
+class B:
+    x: int
+[out]
+main:5: error: Unsupported operand types for + ("str" and "int")
+==
+main:3: error: Incompatible types in assignment (expression has type "str", base class "B" defined the type as "int")
+
+[case testLiskovFineVariableInFunction]
+from b import B
+def outer() -> None:
+    class A(B):
+        x: str
+    def f(x: B) -> None:
+        x.x + int()
+[file b.py]
+class B:
+    x: str
+[file b.py.2]
+class B:
+    x: int
+[out]
+main:6: error: Unsupported operand types for + ("str" and "int")
+==
+main:4: error: Incompatible types in assignment (expression has type "str", base class "B" defined the type as "int")
+
+[case testLiskovFineMethod]
+import b
+class A(b.B):
+    def m(self) -> str: pass
+def f(x: b.B) -> None:
+    x.m() + int()
+f(A())
+[file b.py]
+class B:
+    def m(self) -> str: pass
+[file b.py.2]
+class B:
+    def m(self) -> int: pass
+[out]
+main:5: error: Unsupported operand types for + ("str" and "int")
+==
+main:3: error: Return type of "m" incompatible with supertype "B"
+
+[case testLiskovFineDecorator]
+import b
+from c import deco
+class A(b.B):
+    @deco
+    def m(self) -> str: pass
+def f(x: b.B) -> None:
+    x.m() + int()
+f(A())
+[file b.py]
+from c import deco
+class B:
+    @deco
+    def m(self) -> str: pass
+[file b.py.2]
+from c import deco
+class B:
+    @deco
+    def m(self) -> int: pass
+[file c.py]
+from typing import Callable, TypeVar
+F = TypeVar('F', bound=Callable)
+def deco(f: F) -> F:
+    pass
+[out]
+main:7: error: Unsupported operand types for + ("str" and "int")
+==
+main:5: error: Return type of "m" incompatible with supertype "B"
+
+[case testLiskovFineVariableClean]
+import b
+class A(b.B):
+    x: str
+[file b.py]
+class B:
+    x: str
+[file b.py.2]
+class B:
+    x: int
+[out]
+==
+main:3: error: Incompatible types in assignment (expression has type "str", base class "B" defined the type as "int")
+
+[case testLiskovFineVariableCleanDefInMethod]
+import b
+class A(b.B):
+    def meth(self) -> None:
+        self.x: str
+[file b.py]
+class B:
+    x: str
+[file b.py.2]
+class B:
+    x: int
+[out]
+==
+main:4: error: Incompatible types in assignment (expression has type "str", base class "B" defined the type as "int")
+
+[case testLiskovFineVariableCleanDefInMethodNested]
+from b import B
+def outer() -> None:
+    class A(B):
+        def meth(self) -> None:
+            self.x: str
+[file b.py]
+class B:
+    x: str
+[file b.py.2]
+class B:
+    x: int
+[out]
+==
+main:5: error: Incompatible types in assignment (expression has type "str", base class "B" defined the type as "int")
+
+[case testLiskovFineVariableInFunctionClean]
+from b import B
+def outer() -> None:
+    class A(B):
+        x: str
+[file b.py]
+class B:
+    x: str
+[file b.py.2]
+class B:
+    x: int
+[out]
+==
+main:4: error: Incompatible types in assignment (expression has type "str", base class "B" defined the type as "int")
+
+[case testLiskovFineMethodClean]
+import b
+class A(b.B):
+    def m(self) -> str: pass
+[file b.py]
+class B:
+    def m(self) -> str: pass
+[file b.py.2]
+class B:
+    def m(self) -> int: pass
+[out]
+==
+main:3: error: Return type of "m" incompatible with supertype "B"
+
+[case testLiskovFineDecoratorClean]
+import b
+from c import deco
+class A(b.B):
+    @deco
+    def m(self) -> str: pass
+[file b.py]
+from c import deco
+class B:
+    @deco
+    def m(self) -> str: pass
+[file b.py.2]
+from c import deco
+class B:
+    @deco
+    def m(self) -> int: pass
+[file c.py]
+from typing import Callable, TypeVar
+F = TypeVar('F', bound=Callable)
+def deco(f: F) -> F:
+    pass
+[out]
+==
+main:5: error: Return type of "m" incompatible with supertype "B"


Code review comments:
diff_hunk: @@ -1892,12 +1892,14 @@ def analyze_tuple_or_list_lvalue(self, lval: TupleExpr,
                 self.analyze_lvalue(i, nested=True, add_global=add_global,
                                     explicit_type = explicit_type)
 
-    def analyze_member_lvalue(self, lval: MemberExpr) -> None:
+    def analyze_member_lvalue(self, lval: MemberExpr, explicit_type: bool = False) -> None:
         lval.accept(self)
         if self.is_self_member_ref(lval):
             assert self.type, "Self member outside a class"
+            cur_node = self.type.names.get(lval.name, None)
             node = self.type.get(lval.name)
-            if node is None or isinstance(node.node, Var) and node.node.is_abstract_var:
+            if (node is None or isinstance(node.node, Var) and node.node.is_abstract_var or
+                    cur_node is None and explicit_type):
path: mypy/semanal.py
text: @user1:
This condition is hard to follow. These could help:

* Use parentheses to avoid relying on the precedence of 'and' and 'or' operators.
* Explain possible each case in a comment.

diff_hunk: @@ -742,6 +759,12 @@ def non_trivial_bases(info: TypeInfo) -> List[TypeInfo]:
             if base.fullname() != 'builtins.object']
 
 
+def user_bases(info: TypeInfo) -> List[TypeInfo]:
path: mypy/server/deps.py
text: @user1:
Since the return is only used as a boolean, maybe rename this to `has_user_bases` and return a bool? This would be a bit simpler and faster.