Skip to content

Conditional import of a class follows imports of all branches #19009

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

Closed
ofek opened this issue May 1, 2025 · 10 comments
Closed

Conditional import of a class follows imports of all branches #19009

ofek opened this issue May 1, 2025 · 10 comments
Labels
bug mypy got something wrong pending Issues that may be closed topic-reachability Detecting unreachable code

Comments

@ofek
Copy link

ofek commented May 1, 2025

Bug Report

When you have the following provider module:

if sys.platform == "win32":
    from my_pkg._pty.windows import PtySession
else:
    from my_pkg._pty.unix import PtySession

and attempt importing this symbol from somewhere else, Mypy does not respect the platform condition.

For example, if you call pty.openpty() inside my_pkg._pty.unix and run Mypy on Windows it will show:

Module has no attribute "openpty"  [attr-defined]

To Reproduce

Create the following structure:

mypy-issue
├── pyproject.toml
└── pkg
    ├── __init__.py
    ├── main.py
    └── _pty
        ├── __init__.py
        ├── interface.py
        ├── session.py
        ├── unix.py
        └── windows.py

with the following contents (__init__.py files are empty):

pyproject.toml
[tool.mypy]
pretty = true
pkg/main.py
from __future__ import annotations


class SubprocessRunner:
    def run(self) -> bool:
        from ._pty.session import PtySession

        return PtySession is not None
pkg/_pty/interface.py
from __future__ import annotations

from abc import ABC, abstractmethod


class PtySessionInterface(ABC):
    def __init__(self, command: list[str]) -> None:
        self.command = command

    @abstractmethod
    def get_exit_code(self) -> int | None: ...
pkg/_pty/session.py
from __future__ import annotations

import sys

if sys.platform == "win32":
    from .windows import PtySession
else:
    from .unix import PtySession

__all__ = ["PtySession"]
pkg/_pty/unix.py
from __future__ import annotations

import pty

from .interface import PtySessionInterface


class PtySession(PtySessionInterface):
    def __init__(self, command: list[str]) -> None:
        super().__init__(command)

        self._fd, self._child_fd = pty.openpty()

    def get_exit_code(self) -> int | None:
        return None
pkg/_pty/windows.py
from __future__ import annotations

from .interface import PtySessionInterface


class PtySession(PtySessionInterface):
    def __init__(self, command: list[str]) -> None:
        super().__init__(command)

        self._fd, self._child_fd = (0, 0)

    def get_exit_code(self) -> int | None:
        return None

Finally, enter the mypy-issue directory and run mypy pkg on Windows:

$ mypy pkg
pkg\_pty\unix.py:12: error: Module has no attribute "openpty"  [attr-defined]
            self._fd, self._child_fd = pty.openpty()
                                       ^~~~~~~~~~~
Found 1 error in 1 file (checked 7 source files)

Expected Behavior

I would expect that Mypy respects the platform condition like it does in other circumstances I've experienced and is documented.

Your Environment

  • Mypy version used: mypy 1.15.0 (compiled: yes)
  • Python version used: 3.12
@ofek ofek added the bug mypy got something wrong label May 1, 2025
@JelleZijlstra
Copy link
Member

Thanks for putting this together, and sorry I didn't notice the below looking at previous iterations of it.

The sys.platform checks works as expected, the mismatch with your expectations is that mypy still type checks every file in the directory, even if it's not imported on that platform. To fix this you'll have to (I believe) wrap the whole body of unix.py in another if sys.platform block.

@ofek
Copy link
Author

ofek commented May 1, 2025

Thanks for the quick response! Your answer perplexes me. Are you saying that Mypy doesn't check what code is actually executed based on conditionals and missing attributes like this don't often occur in the wild because libraries typically set to None rather than not assigning a symbol at all? And if they did, we would see this more frequently? What about standard library modules that only exist on certain platforms?

@sterliakov
Copy link
Collaborator

No, Jelle says that invoking mypy pkg checks all files in pkg. So it will check pkg/_pty/windows.py on linux and vice versa. You can either exclude these files depending on the platform (no built-in way to do that AFAIC, write a bash wrapper), wrap the whole file with if sys.platform == ... guard or use a file-level assertion (see #5308).

If there are no related regressions, just

assert sys.platform == 'win32'

after imports in pkg/_pty/windows.py and equivalent assert in pkg/_pty/unix.py should do the trick.

@sterliakov sterliakov added topic-reachability Detecting unreachable code pending Issues that may be closed labels May 1, 2025
@sterliakov
Copy link
Collaborator

sterliakov commented May 1, 2025

(note that you'll need to actually run mypy in CI on different platforms and/or with different --platform flags separately to check all that code, otherwise errors in files intended for other platforms will pass unnoticed)

@ofek
Copy link
Author

ofek commented May 1, 2025

The file-level assertion worked! Is it possible to achieve this with a comment instead so that one doesn't have to potentially unnecessarily import sys and immediately assert, often messing up the import blocks?

@sterliakov
Copy link
Collaborator

None that I'm aware of, but you don't have to mess up the imports - unless some of your imports are only available on certain platforms, you can assert right after all imports.

@sterliakov
Copy link
Collaborator

sterliakov commented May 1, 2025

Oh, and you may also try --always-true and --always-false feature to avoid importing sys altogether, but that might be less trivial to implement - you still have to define/import names you intend to use that way.

@ofek
Copy link
Author

ofek commented May 1, 2025

I tried inline configuration but it looks like this has no effect:

# mypy: platform=linux

@sterliakov
Copy link
Collaborator

sterliakov commented May 1, 2025

Yes, the manual section for the platform flags says:

This option may only be set in the global section ([mypy]).

All such global flags cannot be set inline. And it really makes little sense to set a platform per-file: you can't guarantee that no other file (or even external users, underscores don't prevent me from importing that file) imports it on another platform or without such guard.

@ofek
Copy link
Author

ofek commented May 1, 2025

Thanks for the help! I'm going to close this since there is nothing to be done (I think?) and will be watching the feature request that I just opened: #19013

@ofek ofek closed this as completed May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong pending Issues that may be closed topic-reachability Detecting unreachable code
Projects
None yet
Development

No branches or pull requests

3 participants