New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make descriptor assignments more like normal assignments #5853

Merged
merged 5 commits into from Nov 15, 2018

Conversation

Projects
None yet
3 participants
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi commented Oct 29, 2018

Fixes #5851

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.

@ilevkivskyi ilevkivskyi requested review from JukkaL and gvanrossum Oct 29, 2018

@gvanrossum
Copy link
Member

gvanrossum left a comment

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

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.

This comment has been minimized.

@gvanrossum

gvanrossum Oct 29, 2018

Member

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.)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 29, 2018

Collaborator

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

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

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 comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 1, 2018

Collaborator

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
Copy link
Collaborator

JukkaL left a comment

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

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.

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

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.

T = TypeVar('T')

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

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

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


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

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

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


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

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

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

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

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

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

"""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

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

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

@@ -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.

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

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?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 1, 2018

Collaborator

It is only for binder, I updated this.


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))],

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

Do we lose some type context from using Any here?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 1, 2018

Collaborator

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).

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 1, 2018

Collaborator

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

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Nov 1, 2018

@JukkaL Thanks for review! I addressed your comments.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Nov 15, 2018

@JukkaL Are you happy with the updates I made?

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

@JukkaL

JukkaL approved these changes Nov 15, 2018

Copy link
Collaborator

JukkaL left a comment

Looks good to me now!

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:fix-descr branch Nov 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment