Skip to content
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

Types not added to binder on initial assignment #2008

Open
ddfisher opened this issue Aug 10, 2016 · 37 comments
Open

Types not added to binder on initial assignment #2008

ddfisher opened this issue Aug 10, 2016 · 37 comments

Comments

@ddfisher
Copy link
Collaborator

from typing import Union
x = 0  # type: Union[int, str]
reveal_type(x)  # Revealed type is 'Union[builtins.int, builtins.str]'
x = 0
reveal_type(x)  # Revealed type is 'builtins.int'

This means if you want to declare a union and then immediately use it as one specific member, you'll have an awkward time. For example:

x = "foo" # type: Optional[str]
x + "bar"  # E: Unsupported operand types for + ("Union[str, None]" and "str")

To be consistent with other assignments, we should add types to the binder on initial assignment. I think a number of Union/Optional tests depend on the current behavior, so those will need to be updated.

@ddfisher
Copy link
Collaborator Author

(I came across this issue while working on #1955.)

@rwbarton
Copy link
Contributor

I also noticed this while working on the binder and wondered whether it was intentional. It seems unlikely that any real code relies on the current behavior, but as you note, probably most of the binder-related tests do, if they are to test what was intended. I wonder what a good replacement for the tests is, especially in a post-strict-optional world. Maybe add a generic function

T = TypeVar('T')
def something() -> T: pass

to the test fixtures and then replace initializations like

x = 1  # type: Union[int, str]

by

x = something()  # type: Union[int, str]

@ddfisher ddfisher added this to the 0.4.x milestone Aug 11, 2016
@ddfisher
Copy link
Collaborator Author

Making this change is pretty easy, but it causes 2 new errors when running mypy on mypy.
The first boils down to this:

class Super: pass
class Sub(Super): pass

def f() -> List[Super]:
    x = Sub()  # type: Super
    ret = [x]
    return ret  # E: Incompatible return value type (got List[Sub], expected List[Super])

This can be worked around by giving ret an explicit type, but still seems problematic and a potential source of confusion.

The second boils down to this:

y = [1]
x = y  # type: List[Any]
x.append("foo")  # E: Argument 1 to "append" of "list" has incompatible type "str"; expected "int"

This one should arguably be a cast instead (and that would be a reasonable workaround).

Maybe the conclusion is that we should only do this if the specified type is a Union?

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 25, 2016

Just doing this for unions sounds reasonable to me. More generally, this would likely be useful for conditional definitions, and unions are commonly used for conditional definitions.

For non-unions, this could be reasonable, but it would be much harder to support without breaking the examples given above:

if <cond>:
    e = Manager()  # type: Employee
    e.direct_reports = ...    # Only works for Manager, not Employee
else:
    e = Engineer()
    ...

@gvanrossum
Copy link
Member

I had a long diatribe about this but changed my mind several times while writing, so I guess it's complicated. However I don't see how changing this would break the last Example Jukka gave?

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 25, 2016

If we only make this change for unions, it would likely not break a lot of code. My example doesn't work right now, I think, so it's already a problem (but likely not a major one, so we can continue ignoring it).

@gvanrossum
Copy link
Member

gvanrossum commented Oct 25, 2016 via email

@gvanrossum
Copy link
Member

But the proposed change would fix that example wouldn't it?

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 26, 2016

Yes.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 26, 2016

(If we'd always adds types to binder, this would fix my example. If we'd only do this for union types then it wouldn't fix it.)

@gvanrossum
Copy link
Member

gvanrossum commented Oct 26, 2016 via email

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 26, 2016

Adding it always to the binder breaks some other things, such ones mentioned by David in #2008 (comment).

There are other things it could break. For example, what if we want to make a collection not support mutation:

x = [1, 2, 3]  # type: Sequence[int]    # Don't want anybody to modify this
# I wouldn't expect type of x to be List[int] here
x.append(4)  # This should be an error

Not being able to override the annotated type breaks some pretty basic use cases, and it can be surprising. For union types it is less of a problem, as they are usually used in very specific ways, and they are relatively rare.

All in all this looks like a pretty complex issue to me (though the implementation would be straightforward).

@gvanrossum
Copy link
Member

But the above code is already broken if you write it slightly differently:

x = None  # type: Sequence[int]
x = [1, 2]
x.append(3)  # Passes

@gvanrossum
Copy link
Member

This is marked for the 0.4.x milestone, but we can't even decide on whether to do it. Should we move it out? Or make a decision? Personally I'd rather just do it (for all types, not just for unions) and wait for possible fallout.

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 1, 2016

Doing it for all types would be the consistent thing to do, and experimenting with this sounds reasonable. We could update mypy documentation to mention this explicitly and describe a workaround, such as using a cast. Example workaround:

x = cast(Sequence[int], [])
x.append(4)  # Error

Assignments in the class body with a None initializer might need some special casing.

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 1, 2016

If we are going to change this, it's better to it early rather than later, as this will break some existing code.

@gvanrossum
Copy link
Member

OK, then let's do it!

@gvanrossum
Copy link
Member

Off-line, @ddfisher still objects, but it's possible that the problem is not so much with this (tiny) binder change, but with the issues brought up in an earlier comment. These have to do with invariance, where [x] is given the narrower type rather than the original wider type (e.g. List[Sub] instead of List[Super]). We'll be working through this with a whiteboard some time in the future.

@ddfisher
Copy link
Collaborator Author

ddfisher commented Nov 1, 2016

To be more specific, Guido has largely convinced me that the binder/inference behavior of the initial assignment and subsequent lines should be the same. However, now I'm not sure that our current behavior on subsequent lines is correct -- no other language that I know about will infer a more specific type based on assignments. I think the main reason we do this currently is for Unions, which feels natural in Python because it's dynamic and because Unions are unwrapped as opposed to living in an ADT.

I think we'll have to take a look at a bunch of potential examples to decide what's best.

@gvanrossum
Copy link
Member

Yeah, my counterexample would go something like this:

class Shape:
    def set_color(self, color: float): ...
class Circle(Shape):
    def set_radius(self, r: float): ...
class Square(Shape):
    def set_side(self, side: float): ...
def random_shape() -> Shape:
    shape: Shape  # PEP 526 syntax
    if random.random() < 0.5:
        shape = Square()
        shape.set_side(random.random())
    else:
        shape = Circle()
        shape.set_radius(random.random())
    shape.set_color(random.random())
    return shape

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 2, 2016

Here's another example -- I remember seeing code like this:

def f(x: Iterable[str]) -> None:
    x = set(x)
    # use set operations on x ...

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 2, 2016

Another example:

def is_fancy_circle(shape: Shape) -> bool: ...

def f(shape: Shape) -> None:
    if is_fancy_circle(shape):
        shape = cast(Circle, shape)
        ... # use Circle methods/attributes

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 18, 2017

Decreasing the priority of this since the current behavior seems to be mostly fine and I haven't heard many complaints about this in a while. I'm actually leaning towards just keeping the current behavior.

In Python 3.6 code the binder behavior can be more easily tweaked by annotating the variable before initialization:

x: Optional[int]
x = 1
reveal_type(x)  # int

@gvanrossum
Copy link
Member

I've started a discussion about this in typing-sig.

@gvanrossum
Copy link
Member

gvanrossum commented Sep 26, 2020

In the typing-sig discussion, @erictraut argued convincingly that type checkers should follow what PEP 526 says, and Pyre's @shannonzhu agreed, so I think we have to follow suit.

It should make no difference whether the assignment is combined with the declaration or separate. IOW, we should implement Proposal 3.

(PS. Everyone also agreed that narrowing should not occur if the declared type is Any. Mypy does this already. There is some discussion possible about whether it should occur for types containing Any, but how to narrow is a different discussion anyways.)

@ilevkivskyi ilevkivskyi mentioned this issue Sep 27, 2022
17 tasks
@randolf-scholz
Copy link
Contributor

There can be some unintentional? edge cases with special cased types such as tuple. Code sample in pyright playground:

from typing import Protocol, Self, overload, SupportsIndex

class SupportsSlicing[T_co](Protocol):
    @overload
    def __getitem__(self, index: SupportsIndex, /) -> T_co: ...
    @overload
    def __getitem__(self, index: slice, /) -> Self: ...

lst: list[int] = [1,2,3,4]
x: SupportsSlicing[int] = lst  # ✅ OK
tup: tuple[int, ...] = (1, 2, 3, 4)
y: SupportsSlicing[int] = tup  # ❌ type error

This is because the inferred type is tuple[Literal[1], Literal[2], Literal[3], Literal[4]].

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

No branches or pull requests

7 participants