Added support for platform.system() #8461

joybh98 · 2020-02-29T11:09:28Z

What is this PR about ?
This PR is related to #8166 .
@brettcannon asked to include support for platform.system() with the existing sys.platform

Things I've Changed

Renamed platform to sys_platform to avoid confusion
I've added the platform = platform.system() in options.py
Added param platform in consider_sys_platform function

Tests
I've checked the changes with pep8(pycodestyle)

I think changing the platform to sys_platform may cause some errors if it is being used somewhere else.

ethanhs

Here are my initial comments, I think you have the right idea, but there are some things that need to be fixed.

ethanhs · 2020-03-07T11:30:15Z

mypy/options.py

@@ -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


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

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.

ethanhs · 2020-03-07T11:31:34Z

mypy/reachability.py

@@ -92,7 +92,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.sys_platform)


You seem not to be passing options.platform_system here, but you added it as an argument below.

ethanhs · 2020-03-07T11:33:00Z

mypy/reachability.py

-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


Please update the docstring to add that this also considers platform.system (see below).

ethanhs · 2020-03-07T11:40:30Z

mypy/reachability.py

+    # 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 == 'system':


Shouldn't this be if isinstance(expr.expr, CallExpr) and expr.expr.name == 'platform'?

ethanhs · 2020-03-07T11:42:05Z

mypy/reachability.py

+        elif platform:
+            if not is_sys_attr(expr.operands[0], 'platform'):
+                return TRUTH_VALUE_UNKNOWN
+            right = expr.operands[1]


This seems over-indented, it gets used below.

ethanhs · 2020-03-07T11:43:22Z

mypy/reachability.py

@@ -1,5 +1,5 @@
 """Utilities related to determining the reachability of code (in semantic analysis)."""
-
+import platform


I don't think this import is needed?

ethanhs · 2020-03-07T11:44:34Z

test-data/unit/check-unreachable-code.test

@@ -302,6 +302,8 @@ else:
 import sys
 if sys.platform == 'fictional':
    def foo() -> int: return 0
+if platform.system():


Suggested change

if platform.system():

if platform.system() == 'fake':

I think this test should test comparison.

joybh98 · 2020-03-08T10:18:44Z

@ethanhs thanks for reviewing this commit, will work on it ASAP

joybh98 · 2020-03-11T02:55:59Z

@ethanhs I've added the requested changes, please let me know what else needs to be done, and then I'll start to fix the checks!

joybh98 · 2020-03-13T06:13:29Z

@ethanhs I've renamed the variable, but I still think there is some logic inconsistencies

ethanhs · 2020-03-13T09:54:26Z

@joybhallaa I will take a look at this later.

ethanhs · 2020-03-16T09:21:22Z

mypy/reachability.py

-            return TRUTH_VALUE_UNKNOWN
-        right = expr.operands[1]
+        # check if either platform or platform_system is being used
+        if platform_system:


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.

ethanhs · 2020-03-16T09:55:45Z

Also, user configuration/command line input should override this. I'm not certain what exactly to do here, but I think mapping sys.platform values to platform.system() values would be best. (@JukkaL thoughts?)

Looking at the sys.platform and platform.system() docs, the following mapping makes sense:

{
     'win32': 'Windows',
     'linux': 'Linux',
     'darwin': 'Darwin',
}

There is some awkwardness with cygwin https://stackoverflow.com/a/32612402 and I would be surprised if anyone uses mypy on AIX, so I wouldn't worry about those.

JukkaL

I think mapping sys.platform values to platform.system() values would be best.

I agree that this seems like the best option. Something like if platform.system() == 'Linux' should always mean exactly the same as if sys.platform == 'linux'. Thus we don't need to (and shouldn't) import platform at runtime.

JukkaL · 2020-03-18T16:25:42Z

test-data/unit/check-unreachable-code.test

@@ -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


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

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

hauntsaninja · 2021-08-04T09:10:07Z

PR is pretty stale, feel free to re-open if you want to pick this back up and address feedback / broken tests

added supprt for platform.system()

8eb7b7d

joybh98 changed the title ~~Added supprot for platform.system()~~ Added support for platform.system() Feb 29, 2020

joybh98 added 2 commits March 4, 2020 07:50

added support for compar expr for platform.system()

dfad35a

added test

0794d77

ethanhs reviewed Mar 7, 2020

View reviewed changes

added requested changes

2df738e

fixed called variable to platform from sys_platform

92a6aa2

ethanhs reviewed Mar 16, 2020

View reviewed changes

JukkaL reviewed Mar 18, 2020

View reviewed changes

JelleZijlstra mentioned this pull request Jul 26, 2020

How can I make this # type: ignore platform specific? #8547

Closed

Akuli mentioned this pull request Jun 21, 2021

literal return type for platform.system() python/typeshed#5671

Closed

atakiar mentioned this pull request Jun 21, 2021

feat: reachability recognizes platform.system() #10686

Open

hauntsaninja closed this Aug 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for platform.system() #8461

Added support for platform.system() #8461

joybh98 commented Feb 29, 2020

ethanhs left a comment

ethanhs Mar 7, 2020

ethanhs Mar 11, 2020

ethanhs Mar 7, 2020

ethanhs Mar 7, 2020

ethanhs Mar 7, 2020

ethanhs Mar 7, 2020

ethanhs Mar 7, 2020

ethanhs Mar 7, 2020

joybh98 commented Mar 8, 2020

joybh98 commented Mar 11, 2020

joybh98 commented Mar 13, 2020

ethanhs commented Mar 13, 2020

ethanhs Mar 16, 2020 •

edited

Loading

ethanhs commented Mar 16, 2020 •

edited

Loading

JukkaL left a comment

JukkaL Mar 18, 2020

hauntsaninja commented Aug 4, 2021

	self.platform = sys.platform
	self.sys_platform = sys.platform

		@@ -1,5 +1,5 @@
		"""Utilities related to determining the reachability of code (in semantic analysis)."""

		import platform

Added support for platform.system() #8461

Added support for platform.system() #8461

Conversation

joybh98 commented Feb 29, 2020

ethanhs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joybh98 commented Mar 8, 2020

joybh98 commented Mar 11, 2020

joybh98 commented Mar 13, 2020

ethanhs commented Mar 13, 2020

ethanhs Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

ethanhs commented Mar 16, 2020 • edited Loading

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hauntsaninja commented Aug 4, 2021

ethanhs Mar 16, 2020 •

edited

Loading

ethanhs commented Mar 16, 2020 •

edited

Loading