Skip to content

Added support for platform.system() #8461

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
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions mypy/options.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import re
import pprint
import sys
import platform

from typing_extensions import Final
from typing import Dict, List, Mapping, Optional, Pattern, Set, Tuple, Callable, Any
Expand Down Expand Up @@ -71,6 +72,7 @@ def __init__(self) -> None:
# then mypy does not search for PEP 561 packages.
self.python_executable = sys.executable # type: Optional[str]
self.platform = sys.platform
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you didn't rename this to be consistent with your usage below. You should also change any usage of this elsewhere.

Suggested change
self.platform = sys.platform
self.sys_platform = sys.platform

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you missed updating this based on the test failure. In your call to consider_sys_platform you use options.sys_platform, but here you are assigning to options.platform those two need to be consistently named.

self.platform_system = platform.system()
self.custom_typing_module = None # type: Optional[str]
self.custom_typeshed_dir = None # type: Optional[str]
self.mypy_path = [] # type: List[str]
Expand Down
28 changes: 21 additions & 7 deletions mypy/reachability.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
"""Utilities related to determining the reachability of code (in semantic analysis)."""

from typing import Tuple, TypeVar, Union, Optional
from typing_extensions import Final

Expand Down Expand Up @@ -92,7 +91,7 @@ def infer_condition_value(expr: Expression, options: Options) -> int:
else:
result = consider_sys_version_info(expr, pyversion)
if result == TRUTH_VALUE_UNKNOWN:
result = consider_sys_platform(expr, options.platform)
result = consider_sys_platform(expr, options.platform,options.platform_system)
if result == TRUTH_VALUE_UNKNOWN:
if name == 'PY2':
result = ALWAYS_TRUE if pyversion[0] == 2 else ALWAYS_FALSE
Expand Down Expand Up @@ -150,8 +149,8 @@ def consider_sys_version_info(expr: Expression, pyversion: Tuple[int, ...]) -> i
return TRUTH_VALUE_UNKNOWN


def consider_sys_platform(expr: Expression, platform: str) -> int:
"""Consider whether expr is a comparison involving sys.platform.
def consider_sys_platform(expr: Expression, platform: str, platform_system: str) -> int:
"""Consider whether expr is a comparison involving sys.platform and platform.system()

Return ALWAYS_TRUE, ALWAYS_FALSE, or TRUTH_VALUE_UNKNOWN.
"""
Expand All @@ -166,9 +165,14 @@ def consider_sys_platform(expr: Expression, platform: str) -> int:
op = expr.operators[0]
if op not in ('==', '!='):
return TRUTH_VALUE_UNKNOWN
if not is_sys_attr(expr.operands[0], 'platform'):
return TRUTH_VALUE_UNKNOWN
right = expr.operands[1]
# check if either platform or platform_system is being used
if platform_system:
Copy link
Member

@emmatyping emmatyping Mar 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now you are saying the truth value is unknown if the expression is not a platform attribute, but you don't want to do that. Both platform and platform_system should be truthy, since they will both be set in options.

if not is_platform_attr(expr.operands[0], 'platform_system'):
return TRUTH_VALUE_UNKNOWN
elif platform:
if not is_sys_attr(expr.operands[0], 'platform'):
return TRUTH_VALUE_UNKNOWN
right = expr.operands[1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems over-indented, it gets used below.

if not isinstance(right, (StrExpr, UnicodeExpr)):
return TRUTH_VALUE_UNKNOWN
return fixed_comparison(platform, op, right.value)
Expand Down Expand Up @@ -261,6 +265,16 @@ def is_sys_attr(expr: Expression, name: str) -> bool:
return False


def is_platform_attr(expr: Expression, name: str) -> bool:
# Platform lib calls methods, it differs from sys lib
# sys.platform is of str class and platform.system is of
# function class
if isinstance(expr, MemberExpr) and expr.name == name:
if isinstance(expr.expr, CallExpr) and expr.expr.name == 'platform':
return True
return False


def mark_block_unreachable(block: Block) -> None:
block.is_unreachable = True
block.accept(MarkImportsUnreachableVisitor())
Expand Down
2 changes: 2 additions & 0 deletions test-data/unit/check-unreachable-code.test
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,8 @@ else:
import sys
if sys.platform == 'fictional':
def foo() -> int: return 0
if platform.system() == 'fake':
def foo() -> int: return 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should also be a test that has a successful platform check.

Also test that --platform affects a platform.system() check.

else:
def foo() -> str: return ''
foo() + ''
Expand Down