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

More improvements to pow stubs #7737

Merged
merged 1 commit into from
Apr 28, 2022
Merged

More improvements to pow stubs #7737

merged 1 commit into from
Apr 28, 2022

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Apr 28, 2022

  • Split the Literal[0] int.__pow__ overload into two so that the tests in test_pow.py pass mypy.
  • Add two overloads to float.__rpow__ and two overloads to builtins.pow() using the good-old _PositiveInteger/_NegativeInteger hack.

Resolves #7733.

@TylerYep, this doesn't fix the problem you raised in a general way, but doing so is impossible unfortunately, since there's no general way (in the current type system) of mypy distinguishing between positive arguments and negative arguments. Returning Any is sometimes the least-bad option, which is why the third overload in float.__rpow__ still has to return Any.

@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

core (https://github.com/home-assistant/core)
+ homeassistant/util/color.py:300: error: Redundant cast to "float"  [redundant-cast]

@AlexWaygood AlexWaygood merged commit c3ebc7e into python:master Apr 28, 2022
@AlexWaygood AlexWaygood deleted the pow branch April 28, 2022 16:49
@Akuli
Copy link
Collaborator

Akuli commented Apr 28, 2022

I don't understand the primer output. The cast is cast(float, pow(x, (1.0 / 2.4))), where x is a float. Why is it now redundant but not before? As far as I can tell, pow(float, float) had type Any before this PR, and this PR shouldn't affect it.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Apr 28, 2022

I don't understand the primer output. The cast is cast(float, pow(x, (1.0 / 2.4))), where x is a float. Why is it now redundant but not before? As far as I can tell, pow(float, float) had type Any before this PR, and this PR shouldn't affect it.

Oh interesting. I don't think x has to be float here: mypy has to do some inference as it's an (unannotated, obviously) parameter to a lambda function.

https://github.com/home-assistant/core/blob/7fbc3f63643c314d8c8097e6fd4bc9a1b2314dc2/homeassistant/util/color.py#L298

@Akuli
Copy link
Collaborator

Akuli commented Apr 28, 2022

Here's a self-contained version:

from typing import cast


def color_xy_brightness_to_RGB(
    vX: float, vY: float, ibrightness: int
) -> tuple[int, int, int]:
    """Convert from XYZ to RGB."""
    brightness = ibrightness / 255.0
    if brightness == 0.0:
        return (0, 0, 0)

    Y = brightness

    if vY == 0.0:
        vY += 0.00000000001

    X = (Y / vY) * vX
    Z = (Y / vY) * (1 - vX - vY)

    # Convert to RGB using Wide RGB D65 conversion.
    r = X * 1.656492 - Y * 0.354851 - Z * 0.255038
    g = -X * 0.707196 + Y * 1.655397 + Z * 0.036152
    b = X * 0.051713 - Y * 0.121364 + Z * 1.011530

    # Apply reverse gamma correction.
    r, g, b = map(
        lambda x: (12.92 * x)
        if (x <= 0.0031308)
        else ((1.0 + 0.055) * cast(float, pow(x, (1.0 / 2.4))) - 0.055),
        [r, g, b],
    )

    # Bring all negative components to zero.
    r, g, b = map(lambda x: max(0, x), [r, g, b])

    # If one component is greater than 1, weight components by that value.
    max_component = max(r, g, b)
    if max_component > 1:
        r, g, b = map(lambda x: x / max_component, [r, g, b])

    ir, ig, ib = map(lambda x: int(x * 255), [r, g, b])

    return (ir, ig, ib)

It generates the same mypy error as in the primer output. But the cast is in fact not redundant. If you remove it, you get a new error: Returning Any from function declared to return "object".

Also, if you replace x with reveal_type(x), you once again see that mypy is trying two different types for x:

asd.py:29: error: Redundant cast to "float"
asd.py:29: note: Revealed type is "Any"
asd.py:29: note: Revealed type is "builtins.float"

I'm not yet sure about what's going on and where Any is coming from, but getting an error about an unnecessary cast that is in fact needed doesn't feel like an improvement to me.

@Akuli
Copy link
Collaborator

Akuli commented Apr 28, 2022

And here's a smaller reproducer:

from typing import cast


def with_cast() -> None:
    a, b = map(
        lambda x: cast(float, pow(x, (1.0 / 2.4))),
        [1.2, 3.4],
    )

def without_cast() -> None:
    a, b = map(
        lambda x: pow(x, (1.0 / 2.4)),
        [1.2, 3.4],
    )

The with_cast() function gives an error Redundant cast to "float", because pow(x, 1.0 / 2.4) returns Any which shouldn't need any casting. But the without_cast() function also gives an error, Returning Any from function declared to return "object", because pow(float, float) now returns Any, and for whatever reason that I still don't understand, mypy has warn-return-any enabled by default.

@Akuli
Copy link
Collaborator

Akuli commented Apr 28, 2022

And now that I compare the output of reveal_type(pow(1.2, 1.2)) before and after this PR, it always outputs Any. I don't understand how it worked before this PR, but this seems to be a mypy weirdness that isn't this PR's fault anyway, and not something that many projects will run into.

tl;dr I created a long wall of text that can be ignored :D

@AlexWaygood
Copy link
Member Author

I don't understand how it worked before this PR, but this seems to be a mypy weirdness that isn't this PR's fault anyway

Agreed :)

@satyanash
Copy link

Returning Any is sometimes the least-bad option, which is why the third overload in float.rpow still has to return Any.

@AlexWaygood I don't get it. Why not declare the return type to be Union[complex, float].
This is much better than Any, right? Forces the user to handle both possible cases.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Sep 21, 2022

Returning Any is sometimes the least-bad option, which is why the third overload in float.rpow still has to return Any.

@AlexWaygood I don't get it. Why not declare the return type to be Union[complex, float]. This is much better than Any, right? Forces the user to handle both possible cases.

We have a general, longstanding policy against using union return types, in the name of preferring "false negative" errors over "false positive" errors. More on that here: python/mypy#1693. It's reasonable to discuss whether that should continue to be typeshed's policy in 2022. But that discussion should happen in an issue rather than a PR that was merged five months ago :)

Also, note that proposals to use union return types for pow are generally pretty disruptive (breaks a lot of people's code). Most of the time when people do x ** y, they know for a fact that both x and y will be > 0, so forcing them to do the extra isinstance() check isn't really adding to type safety; it's just adding one more annoyance to their life.

Here's another PR where I made some changes to the pow stubs: #6287. In that PR, I initially proposed changing the return type of one of the pow overloads to Union[int, float]. If you look at the first (minimised) comment from mypy_primer on that PR thread, you can see that it would have been very disruptive.

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

Successfully merging this pull request may close these issues.

pow float return value is Any in 0.950
4 participants