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

[1.12] Regression in is None narrowing #18021

Closed
Dreamsorcerer opened this issue Oct 23, 2024 · 8 comments · Fixed by #18023
Closed

[1.12] Regression in is None narrowing #18021

Dreamsorcerer opened this issue Oct 23, 2024 · 8 comments · Fixed by #18023
Labels
bug mypy got something wrong topic-literal-types topic-type-narrowing Conditional type narrowing / binder topic-union-types

Comments

@Dreamsorcerer
Copy link
Contributor

Dreamsorcerer commented Oct 23, 2024

Bug Report

Regression in 1.12.

I'm not sure if this is deliberate or somehow got missed, but I thought aiohttp was in the mypy test suite, so should have been caught in the regression tests..

    def feed_data(
        self,
        data: bytes,
        SEP: Optional[_SEP] = None,
        *args: Any,
        **kwargs: Any,
    ) -> Tuple[List[Tuple[RawResponseMessage, StreamReader]], bool, bytes]:
        if SEP is None:
            SEP = b"\r\n" if DEBUG else b"\n"
        return super().feed_data(data, SEP, *args, **kwargs)

https://github.com/aio-libs/aiohttp/blob/01bfb4380742df87f910c2c62c619e0e9b9a0ed4/aiohttp/http_parser.py#L678-L684

Now results in:

aiohttp/http_parser.py:684:40: error: Argument 2 to "feed_data" of "HttpParser"
has incompatible type "Literal[b'\\r\\n', b'\\n'] | None"; expected
"Literal[b'\\r\\n', b'\\n']"  [arg-type]
            return super().feed_data(data, SEP, *args, **kwargs)
                                           ^~~

Expected Behavior

The variable can never be None at this point, so there should be no error (as in 1.11 and previous releases).

Your Environment

aio-libs/aiohttp#9527

@Dreamsorcerer Dreamsorcerer added the bug mypy got something wrong label Oct 23, 2024
@Dreamsorcerer Dreamsorcerer changed the title Regression in is None narrowing [1.12] Regression in is None narrowing Oct 23, 2024
@JukkaL
Copy link
Collaborator

JukkaL commented Oct 23, 2024

This bisects to #17427. Here is a complete, slightly simplified reproducer:

from typing import Literal

SEP = Literal["a", "b"]

class Base:
    def feed_data(
        self,
        sep: SEP,
    ) -> int:
        return 0

class C(Base):
    def feed_data(
        self,
        sep: SEP | None = None,
    ) -> int:
        if sep is None:
            sep = reveal_type("a" if int() else "b")  # Union[Literal['a'], Literal['b']?]
            reveal_type(sep)  # None
        # Incompatible type "Literal['a', 'b'] | None"; expected "Literal['a', 'b']"
        return super().feed_data(sep)

Interestingly, we used to infer the correct union type before #17427, so this is perhaps not just a pre-existing issue that #17427 exposed but something more subtle.

cc @ilevkivskyi

@Dreamsorcerer
Copy link
Contributor Author

Appears mypy_primer did catch it, but there were probably too many changes there for someone to notice the legitimate regression.

@JukkaL
Copy link
Collaborator

JukkaL commented Oct 23, 2024

Yeah. I think the vast majority of new errors were legitimate.

JukkaL pushed a commit that referenced this issue Oct 24, 2024
Fixes #18021

When I switched to (almost) always using unions as inferred from ternary
expressions, I had a choice, because before we used either full context
(i.e. l.h.s.) or the if type to infer the else type. After some playing
I found the second one usually works better. But as we see this is not
always the case, so I add some special-casing.
@Dreamsorcerer
Copy link
Contributor Author

Not sure this was fully resolved. The linked fix is in 1.14, but the aiohttp regression is still present:
https://github.com/aio-libs/aiohttp/actions/runs/12496766030/job/34868785013?pr=10222

@ilevkivskyi
Copy link
Member

@Dreamsorcerer could you please find a simple self-contained repro? (I guess there may be something special about aiohttp setup) because the above example works for me in a recent master.

@Dreamsorcerer
Copy link
Contributor Author

@ilevkivskyi Sorry for the delay, been a slow month.

I broke it down to a minimal reproducer, then realised that it's almost exactly the same as the above example. After some more testing I've realised that the reveal_type() is fixing the example. Simply removing the first reveal_type() reproduces the issue again:

from typing import Literal

SEP = Literal["a", "b"]

class Base:
    def feed_data(
        self,
        sep: SEP,
    ) -> int:
        return 0

class C(Base):
    def feed_data(
        self,
        sep: SEP | None = None,
    ) -> int:
        if sep is None:
            sep = "a" if int() else "b"  # Union[Literal['a'], Literal['b']?]
            reveal_type(sep)  # None
        # Incompatible type "Literal['a', 'b'] | None"; expected "Literal['a', 'b']"
        return super().feed_data(sep)

@ilevkivskyi
Copy link
Member

OK, nice, I have a fix, but it is too late, I will make a PR tomorrow.

@ilevkivskyi
Copy link
Member

@Dreamsorcerer here is the real fix #18545

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-literal-types topic-type-narrowing Conditional type narrowing / binder topic-union-types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants