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

SIM102 should be disabled for os.name and platform.system() checks (in addition to sys.platform) #16980

Open
real-or-random opened this issue Mar 26, 2025 · 2 comments
Labels
rule Implementing or modifying a lint rule

Comments

@real-or-random
Copy link

Summary

SIM102 (collapsible-if) has an exception for version and os checks. This currently matches sys.platform.

if sys.platform == "linux":  # no SIM102
    if foo:

Analogously, it should also be disabled os.name and platform.system():

if os.name == "posix": # SIM102
    if foo:

See https://stackoverflow.com/a/58071295 to learn about the various methods. (And if you've looked at this post, you're also in a good position to have an opinion on the otherwise unrelated #5622.)

@MichaReiser MichaReiser added the rule Implementing or modifying a lint rule label Mar 26, 2025
@MichaReiser
Copy link
Member

Can you tell me a bit more why you think SIM102 should be disabled. Is it because some type checkers don't support narrowing if it's an if-else expression?

@real-or-random
Copy link
Author

Can you tell me a bit more why you think SIM102 should be disabled. Is it because some type checkers don't support narrowing if it's an if-else expression?

Okay, TIL, and now that you ask, I'm not so sure anymore.

Until now, my point was merely readability: I think branching on a Python version or on an OS is a different thing than branching on business logic, so believe it's good to have it in a separate if: Then your brain can easily focus on the part that concerns a particular OS, and skip the other parts. But this is my personal view, and I'm not sure how objective it is. My thinking was simply that if it has been decided that checks for sys.platform and sys.version_info should be treated differently for readability, then the same should apply to os.name and platform.system().

But I was not aware that type checkers may be a reason. This goes back to PEP 484, which says "Type checkers are expected to understand simple version and platform checks, e.g.: (...) if sys.version_info[0] >= 3: (...) if sys.platform == 'win32':". The current docs are similar, but use sys.version_info >= (3,12) as an example.

After some research, I found this issue about the set of conditions being underspecified... It's a good summary of what is supported by different typecheckers. For example, pyright supports os.name but mypy doesn't. None of them support platform.system()`.

My feeling is that a linter should be conservative here. If someone uses os.name to make pyright happy, this seems fine. But given that this is still very much under debate, I'm not sure if it's the right time to change SIM102...

In either case, I think the comments here should be updated:

// Avoid suggesting ternary for `if sys.version_info >= ...`-style checks.
if is_sys_version_block(stmt_if, checker.semantic()) {
return;
}
// Avoid suggesting ternary for `if TYPE_CHECKING:`-style checks.
if is_type_checking_block(stmt_if, checker.semantic()) {
return;
}

If I'm not mistaken, this file is only about SIM102, so this is not about ternaries. Moreover, is_sys_version_block also checks for sys.platform, so the name is misleading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

2 participants