pow function return type overly broad #285

Closed
johnthagen opened this Issue Jun 10, 2016 · 15 comments

Comments

Projects
None yet
7 participants
@johnthagen
Contributor

johnthagen commented Jun 10, 2016

I happened to be reading through the typeshed code because of a completely unrelated issue I was tracking down, and noticed that the pow function return type seems to be overly broad:

https://github.com/python/typeshed/blob/master/stdlib/3/builtins.pyi#L703

def pow(x: int, y: int) -> Any: ...  # The return type can be int or float, depending on y

Should that be?

def pow(x: int, y: int) -> Union[int, float]: ...
@matthiaskramm

This comment has been minimized.

Show comment
Hide comment
@matthiaskramm

matthiaskramm Jun 10, 2016

Contributor

That does sound better! Could you send a pull request?

Contributor

matthiaskramm commented Jun 10, 2016

That does sound better! Could you send a pull request?

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Jun 10, 2016

Contributor

I'd rather not do this. Basically this would be equivalent to having the
return as float and integer only code would have to cast or convert the
result to an int or risk type errors or invalid inferred types.
On Fri, Jun 10, 2016 at 21:26 Matthias Kramm notifications@github.com
wrote:

That does sound better! Could you send a pull request?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#285 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABDnx-cnxGbXF1Qu9omWITIQSK2L1X-rks5qKch9gaJpZM4IzQ82
.

Contributor

JukkaL commented Jun 10, 2016

I'd rather not do this. Basically this would be equivalent to having the
return as float and integer only code would have to cast or convert the
result to an int or risk type errors or invalid inferred types.
On Fri, Jun 10, 2016 at 21:26 Matthias Kramm notifications@github.com
wrote:

That does sound better! Could you send a pull request?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#285 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABDnx-cnxGbXF1Qu9omWITIQSK2L1X-rks5qKch9gaJpZM4IzQ82
.

@matthiaskramm

This comment has been minimized.

Show comment
Hide comment
@matthiaskramm

matthiaskramm Jun 10, 2016

Contributor

That argument works for any Union[x, y] return type with x more common than y. What should our general approach be for that?

Also, the inferred type here would be Union[float, int], which is better than Any?

Contributor

matthiaskramm commented Jun 10, 2016

That argument works for any Union[x, y] return type with x more common than y. What should our general approach be for that?

Also, the inferred type here would be Union[float, int], which is better than Any?

@johnthagen

This comment has been minimized.

Show comment
Hide comment
@johnthagen

johnthagen Jun 10, 2016

Contributor

@JukkaL I don't understand. Won't leaving it as Any not only be "incorrect" but also leave type errors from being detected?

Consider:

def fun(y: str) -> None:
    y.capitalize()
    ...

x = pow(2, 2)
fun(x)  # type error missed because pow annotated as returning Any
Contributor

johnthagen commented Jun 10, 2016

@JukkaL I don't understand. Won't leaving it as Any not only be "incorrect" but also leave type errors from being detected?

Consider:

def fun(y: str) -> None:
    y.capitalize()
    ...

x = pow(2, 2)
fun(x)  # type error missed because pow annotated as returning Any
@matthiaskramm

This comment has been minimized.

Show comment
Hide comment
Contributor

matthiaskramm commented Jun 14, 2016

Also see python/mypy#1693

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Jun 14, 2016

Contributor

@johnthagen Any is never incorrect (it is a valid type for an arbitrary value), though it would be imprecise. The design of PEP 484 allows Any to be used whenever the type system is not expressive enough to give a precise type for something. In this case, we can't give a precise type for pow since the return type depends on the values of the arguments, not only their the types, and PEP 484 only works at the level of types. Falling back to Any is a valid thing to do here and we've done a similar thing elsewhere. PEP 484 defines a gradual type system where it's accepted that we don't have precise types for all programs.

This is a tradeoff: by occasionally falling back to Any we are able to do more precise type checking elsewhere, where Any is not used.

@matthiaskramm Yes, any Union[x, y] where x is a subtype of y is equivalent to y (at least if none of them are Any -- otherwise the waters are a bit muddy). Also, PEP 484 implies that int is a subtype of float, so Union[int, float] is equivalent to float. The problem with this return type is that calls such as file.read(2**16) would be rejected, since read expects an int argument, not a float.

If a type system allows the above call to file.read, it seems to me that it could logically declare float to be a subtype of int. However, I don't consider that to be desirable since it would make type checking less effective -- in many contexts a float is not valid at runtime when an int is expected.

Contributor

JukkaL commented Jun 14, 2016

@johnthagen Any is never incorrect (it is a valid type for an arbitrary value), though it would be imprecise. The design of PEP 484 allows Any to be used whenever the type system is not expressive enough to give a precise type for something. In this case, we can't give a precise type for pow since the return type depends on the values of the arguments, not only their the types, and PEP 484 only works at the level of types. Falling back to Any is a valid thing to do here and we've done a similar thing elsewhere. PEP 484 defines a gradual type system where it's accepted that we don't have precise types for all programs.

This is a tradeoff: by occasionally falling back to Any we are able to do more precise type checking elsewhere, where Any is not used.

@matthiaskramm Yes, any Union[x, y] where x is a subtype of y is equivalent to y (at least if none of them are Any -- otherwise the waters are a bit muddy). Also, PEP 484 implies that int is a subtype of float, so Union[int, float] is equivalent to float. The problem with this return type is that calls such as file.read(2**16) would be rejected, since read expects an int argument, not a float.

If a type system allows the above call to file.read, it seems to me that it could logically declare float to be a subtype of int. However, I don't consider that to be desirable since it would make type checking less effective -- in many contexts a float is not valid at runtime when an int is expected.

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Jun 14, 2016

Contributor

I wonder if it might be worth adding an UnsafeUnion, which is similar to Union, except it's considered valid if any of the union'd types work. The return type here could then be expressed as UnsafeUnion[int, float].

Obviously, this wouldn't catch int/float errors (which I think we consider overly onerous to do safely in this instance anyway), but would prevent all other misuses.

Contributor

ddfisher commented Jun 14, 2016

I wonder if it might be worth adding an UnsafeUnion, which is similar to Union, except it's considered valid if any of the union'd types work. The return type here could then be expressed as UnsafeUnion[int, float].

Obviously, this wouldn't catch int/float errors (which I think we consider overly onerous to do safely in this instance anyway), but would prevent all other misuses.

@matthiaskramm

This comment has been minimized.

Show comment
Hide comment
@matthiaskramm

matthiaskramm Jun 14, 2016

Contributor

We primarily want "safe" Unions for None checking, right?

Maybe Optional[x] (or Union[x, None]) should be treated special?

Going with @ddfisher's idea, maybe every Union[x, y] should be UnsafeUnion[x, y], and every Optional[x] should be SafeUnion[x, None]?

Contributor

matthiaskramm commented Jun 14, 2016

We primarily want "safe" Unions for None checking, right?

Maybe Optional[x] (or Union[x, None]) should be treated special?

Going with @ddfisher's idea, maybe every Union[x, y] should be UnsafeUnion[x, y], and every Optional[x] should be SafeUnion[x, None]?

@ddfisher

This comment has been minimized.

Show comment
Hide comment
@ddfisher

ddfisher Jun 14, 2016

Contributor

I don't think that's the only reason we want safe Unions, actually. It's not uncommon to accept multiple argument types, and it makes sense for the type checker to ensure you're using them safely. A somewhat contrived example:

def my_print(x: Union[str, List[str]]) -> None:
  if isinstance(x, list):
    x = ", ".join(x)
  print(x)

This is basically Python's version of sum typing, and I think it's reasonably important that it be safe by default.

Contributor

ddfisher commented Jun 14, 2016

I don't think that's the only reason we want safe Unions, actually. It's not uncommon to accept multiple argument types, and it makes sense for the type checker to ensure you're using them safely. A somewhat contrived example:

def my_print(x: Union[str, List[str]]) -> None:
  if isinstance(x, list):
    x = ", ".join(x)
  print(x)

This is basically Python's version of sum typing, and I think it's reasonably important that it be safe by default.

@JukkaL

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Jun 15, 2016

Contributor

If mypy had a way of special casing type checking of certain functions (discussed at python/mypy#1240) it would likely resolve this issue and would still enable precise type checking for at least many call sites. Then having a union return type for pow in typeshed would be fine for mypy.

UnsafeUnion[...] would be a better return type than Any, but it feels like too heavy machinery for an edge case. Also, if unions would be unsafe by default, users couldn't trust union type annotations to be correct. Consider an arbitrary function with a correct annotation:

def log_message(msg: str) -> None:
    ...  # a complex body

Now we could modify it to this (with an arbitrary BadType) and mypy wouldn't complain, because the body is valid for str:

def log_message(msg: Union[str, BadType]) -> None:
    ...  # a complex body

log_message(BadType())   # ouch: error but mypy wouldn't catch it

Unions can be used when we have a few similar but not identical types. Mypy can verify that they are used in a way that is compatible with all items. Consider this somewhat realistic example:

def remote_connection() -> Union[HTTPRemote, SSHRemote]:
    if config.connection_type == HTTP:
        return HTTPRemote(...)
    else:
        return SSHRemote(..)

conn = remote_connection()
conn.read(4)  # mypy will verify that both remote classes support read
Contributor

JukkaL commented Jun 15, 2016

If mypy had a way of special casing type checking of certain functions (discussed at python/mypy#1240) it would likely resolve this issue and would still enable precise type checking for at least many call sites. Then having a union return type for pow in typeshed would be fine for mypy.

UnsafeUnion[...] would be a better return type than Any, but it feels like too heavy machinery for an edge case. Also, if unions would be unsafe by default, users couldn't trust union type annotations to be correct. Consider an arbitrary function with a correct annotation:

def log_message(msg: str) -> None:
    ...  # a complex body

Now we could modify it to this (with an arbitrary BadType) and mypy wouldn't complain, because the body is valid for str:

def log_message(msg: Union[str, BadType]) -> None:
    ...  # a complex body

log_message(BadType())   # ouch: error but mypy wouldn't catch it

Unions can be used when we have a few similar but not identical types. Mypy can verify that they are used in a way that is compatible with all items. Consider this somewhat realistic example:

def remote_connection() -> Union[HTTPRemote, SSHRemote]:
    if config.connection_type == HTTP:
        return HTTPRemote(...)
    else:
        return SSHRemote(..)

conn = remote_connection()
conn.read(4)  # mypy will verify that both remote classes support read
@Dakkaron

This comment has been minimized.

Show comment
Hide comment
@Dakkaron

Dakkaron Jun 16, 2016

Contributor

Shouldn't this case be fixable with overloading?

@overload
def pow(x: int, y: int) -> int
@overload
def pow(x: float, y: int) -> float
@overload
def pow(x: int, y: float) -> float
@overload
def pow(x: float, y: float) -> float

The only thing this would not catch is that pow(2,100) returns a long and not an int.

Other than that, pow() seems to return quite predictable types.
But I would also like to see UnsafeUnions. Especially in user code this can be quite useful, because that way you don't have to litter your code with isinstance()-checks.

Contributor

Dakkaron commented Jun 16, 2016

Shouldn't this case be fixable with overloading?

@overload
def pow(x: int, y: int) -> int
@overload
def pow(x: float, y: int) -> float
@overload
def pow(x: int, y: float) -> float
@overload
def pow(x: float, y: float) -> float

The only thing this would not catch is that pow(2,100) returns a long and not an int.

Other than that, pow() seems to return quite predictable types.
But I would also like to see UnsafeUnions. Especially in user code this can be quite useful, because that way you don't have to litter your code with isinstance()-checks.

@matthiaskramm

This comment has been minimized.

Show comment
Hide comment
@matthiaskramm

matthiaskramm Jun 16, 2016

Contributor

pow(2, -1) returns a float.

Contributor

matthiaskramm commented Jun 16, 2016

pow(2, -1) returns a float.

@Dakkaron

This comment has been minimized.

Show comment
Hide comment
@Dakkaron

Dakkaron Jun 16, 2016

Contributor

Good point, I did not test that^^

Contributor

Dakkaron commented Jun 16, 2016

Good point, I did not test that^^

@gaborbernat

This comment has been minimized.

Show comment
Hide comment
@gaborbernat

gaborbernat Aug 7, 2017

Contributor

can we conclude that this is a won't fix issue?

Contributor

gaborbernat commented Aug 7, 2017

can we conclude that this is a won't fix issue?

@matthiaskramm

This comment has been minimized.

Show comment
Hide comment
@matthiaskramm

matthiaskramm Aug 7, 2017

Contributor

Yeah, we're leaving this one alone.

Contributor

matthiaskramm commented Aug 7, 2017

Yeah, we're leaving this one alone.

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