Make descriptor assignments more like normal assignments #5853

ilevkivskyi · 2018-10-29T16:08:32Z

This makes descriptor assignments behave more like normal assignments. This includes:

The same error message as for normal assignment
Using binder to restrict types (but under a bit more strict condition)

I was not able to refactor all the logic to checkmember.py, but this is probably OK. Also this PR can introduce potential unsafety as discussed in the issue, but this is probably not much different from using binder in some other situations.

gvanrossum

I'll leave this to @JukkaL's judgment.

gvanrossum · 2018-10-29T16:38:24Z

mypy/checker.py

+        set_type = inferred_dunder_set_type.arg_types[1]
+        # Special case: if the rvalue_type is a subtype of both '__get__' and '__set__' types,
+        # then we invoke the binder to narrow type by this assignment. Technically, this is not
+        # safe, but in practice this is what a user expects.


I'm still worried about edge cases here. E.g. what about this pair:

def __get__(self, obj, typ) -> float: return self._val def __set__(self, obj, value) -> None: self._val = float(value)

and then the usage

if not isinstance(a.foo, int): a.foo = int(a.foo) blah[a.foo] # Suppose blah is a List, so the index must be int

Perhaps I would worry less if we only went after real unions here, not subtypes. (Though in most cases subtypes will also work -- it's really mostly float/int, where casting int to float changes the representation.)

Yes, this is clearly a trade off. @JukkaL what do you think?

What about only doing the binder magic when the relevant types in __get__ and __set__ are the same? We might need some logic to deal with overloads. Now if __set__ does some type conversion, it will likely accept a more general type. This would still be unsafe, but I that's unavoidable.

This is a bit complicated because of the overload case. I propose to instead to apply binder only if "get" type is a subtype of "set" type, and the rvalue type is a subtype of "get" type. This covers the situation when the "get" and "set" types are the same. And in general looks reasonably safe. I added this in latest commit.

JukkaL

Looks reasonable. Left an idea about a potential way to partially deal with unsafety.

JukkaL · 2018-10-31T11:35:26Z

mypy/checker.py

+        set_type = inferred_dunder_set_type.arg_types[1]
+        # Special case: if the rvalue_type is a subtype of both '__get__' and '__set__' types,
+        # then we invoke the binder to narrow type by this assignment. Technically, this is not
+        # safe, but in practice this is what a user expects.


What about only doing the binder magic when the relevant types in __get__ and __set__ are the same? We might need some logic to deal with overloads. Now if __set__ does some type conversion, it will likely accept a more general type. This would still be unsafe, but I that's unavoidable.

JukkaL · 2018-10-31T11:36:24Z

test-data/unit/check-classes.test

+T = TypeVar('T')
+
+class IntDescr:
+  def __get__(self, obj: T, typ: Type[T]) -> Optional[int]: ...


Style nit: use 4-space indent (here and elsewhere).

JukkaL · 2018-10-31T11:37:26Z

test-data/unit/check-classes.test

+
+  def meth_spec(self) -> B:
+    self.spec = B()
+    return self.spec


Use reveal_type(self.spec) since plausibly the inferred type could be Any, which would be wrong.

JukkaL · 2018-10-31T11:38:00Z

test-data/unit/check-classes.test

+
+  def meth_spec(self) -> A:
+    self.spec = A()
+    return self.spec


Use reveal_type() to make sure self.spec is not Any, for example.

JukkaL · 2018-10-31T11:38:30Z

test-data/unit/check-classes.test

+  def meth_spec(self) -> int:
+    if self.spec is None:
+      self.spec = 0
+    return self.spec


Use reveal_type(self.spec) to get more certainty about the inferred type.

JukkaL · 2018-10-31T11:39:15Z

mypy/checker.py

        """Type member assignment.

        This defers to check_simple_assignment, unless the member expression
        is a descriptor, in which case this checks descriptor semantics as well.

        Return the inferred rvalue_type and whether to infer anything about the attribute type.
+        Note: this method exists here and not in checkmember.py, because we need to take


Add an empty line before "Note: ..." to make it more prominent.

JukkaL · 2018-10-31T11:39:53Z

mypy/checker.py

@@ -2491,59 +2491,67 @@ def check_simple_assignment(self, lvalue_type: Optional[Type], rvalue: Expressio
            return rvalue_type

    def check_member_assignment(self, instance_type: Type, attribute_type: Type,
-                                rvalue: Expression, context: Context) -> Tuple[Type, bool]:
+                                rvalue: Expression, context: Context) -> Tuple[Type, Type, bool]:
        """Type member assignment.

        This defers to check_simple_assignment, unless the member expression
        is a descriptor, in which case this checks descriptor semantics as well.

        Return the inferred rvalue_type and whether to infer anything about the attribute type.


Document the new return type. Could you also rewrite "whether to infer anything about the attribute type", as it wasn't immediately clear to me what it means. Maybe explicitly mention binder here, as "infer" usually implies inferring the real type of an attribute. Or is it also used for inferring the type of an attribute on initial assignment?

It is only for binder, I updated this.

JukkaL · 2018-10-31T11:42:21Z

mypy/checker.py


        function = function_type(dunder_set, self.named_type('builtins.function'))
        bound_method = bind_self(function, attribute_type)
        typ = map_instance_to_supertype(attribute_type, dunder_set.info)
        dunder_set_type = expand_type_by_instance(bound_method, typ)

        _, inferred_dunder_set_type = self.expr_checker.check_call(
-            dunder_set_type, [TempNode(instance_type), rvalue],
+            dunder_set_type, [TempNode(instance_type), TempNode(AnyType(TypeOfAny.special_form))],


Do we lose some type context from using Any here?

Initially I thought this is is probably OK, but now I am not so sure. Anyway, to be on the safe side, we can check the call twice. Once we use the real expression but turn off errors, just to infer the type (if it is invalid, we will later show a nice "Invalid type in assignment..." error later). Second time we check with the Any type to show the remaining ___set__ specific errors (like wrong type of first argument).

(The above is what I already did in the latest commit.)

ilevkivskyi · 2018-11-01T23:50:21Z

@JukkaL Thanks for review! I addressed your comments.

ilevkivskyi · 2018-11-15T00:49:04Z

@JukkaL Are you happy with the updates I made?

JukkaL

Looks good to me now!

ilevkivskyi added 2 commits October 29, 2018 14:10

Fix binder interaction with __set__

165354f

Actually use binder; update tests

2a0e411

ilevkivskyi requested review from JukkaL and gvanrossum October 29, 2018 16:08

Fix whitespace

fe5336d

gvanrossum reviewed Oct 29, 2018

View reviewed changes

Fix whitespace for real

320c780

JukkaL reviewed Oct 31, 2018

View reviewed changes

Address CR

ab375e5

JukkaL changed the title ~~Make descriptor assignments more like normal assignments.~~ Make descriptor assignments more like normal assignments Nov 15, 2018

JukkaL approved these changes Nov 15, 2018

View reviewed changes

ilevkivskyi merged commit 3bea26f into python:master Nov 15, 2018

ilevkivskyi deleted the fix-descr branch November 15, 2018 15:40

Uh oh!

Make descriptor assignments more like normal assignments #5853

Make descriptor assignments more like normal assignments #5853

Uh oh!

Conversation

ilevkivskyi commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented Nov 1, 2018

Uh oh!

ilevkivskyi commented Nov 15, 2018

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ilevkivskyi commented Oct 29, 2018 •

edited

Loading