-
Notifications
You must be signed in to change notification settings - Fork 287
Update specification for directives for sys.implementation and sys.platform checks. #2173
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,8 +145,13 @@ left undefined by the typing spec at this time. | |
| Version and platform checking | ||
| ----------------------------- | ||
|
|
||
| Type checkers are expected to understand simple version and platform | ||
| checks, e.g.:: | ||
|
|
||
| Type checkers should support narrowing based on ``sys.version_info``, ``sys.platform``, and ``sys.implementation.name`` checks. | ||
|
|
||
| sys.version_info checks | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Type checkers should support ``sys.version_info`` comparisons:: | ||
|
|
||
| import sys | ||
|
|
||
|
|
@@ -155,12 +160,65 @@ checks, e.g.:: | |
| else: | ||
| # Python 3.11 and lower | ||
|
|
||
| **Supported patterns:** | ||
|
|
||
| * Equality: ``sys.version_info == (3, 10)`` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This example doesn't make much sense, since it would never be true at runtime on any actual Python version. A realistic example that might actually return I would suggest not specifying that equality checks should be supported at all; I don't think they are usable for any realistic scenario.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that the specific version should be adjusted, but I don't think that testing for equality should be dismissed, or truncated to an arbitrary number of nodes.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand what the possible use case is here. Can you elaborate? To be clear, at runtime equality checks with From a type checker implementer perspective, I feel pretty strongly that we would never implement support for this, because it is a bunch of work and it is not useful in any realistic case. I see this as a blocker for this proposal.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for the clear feedback,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think that all I'm fine with supporting
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If all sys.version_info comparisons are only supported on the first two elements of the version tuple, the I think that therer is no need to support the explicit form
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with Carl. Equality and inequality checks don't make sense. In think the only operators that make sense are
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. adjusted accordingly. |
||
| * Inequality: ``sys.version_info != (3, 9)`` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think inequality checks should also not be supported, for the same reason discussed above for equality checks.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see your point; removed |
||
| * Comparison: ``sys.version_info >= (3, 10)``, ``sys.version_info < (3, 12)`` | ||
| * Tuple slicing: ``sys.version_info[:2] >= (3, 10)`` | ||
| * Element access: ``sys.version_info[0] >= 3`` | ||
|
Comment on lines
+168
to
+169
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think for these cases (and even for the basic comparison case) it's also important to specify which elements of the tuple are supported. For example, are type checkers expected to support I think we should be explicit that type checkers should only be expected to have special handling for the first two tuple elements (major and minor version); anything more than that adds a lot of complexity to type checker configuration for little to no gain.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I think that is a useful restriction.
Comment on lines
+168
to
+169
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about the named attributes? Should type checkers support e.g. (I don't think this is useful enough to require, but maybe we should be explicit that support for it is not expected.)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From a MicroPyton perspective I concur, as it does not support named attributes for this, but I don't necessarily want to force that on other implementations. |
||
|
|
||
| sys.platform checks | ||
| ^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Type checkers should support ``sys.platform`` comparisons:: | ||
|
|
||
| import sys | ||
|
|
||
| if sys.platform == 'win32': | ||
| # Windows specific definitions | ||
| else: | ||
| # Posix specific definitions | ||
|
|
||
| Don't expect a checker to understand obfuscations like | ||
| if sys.platform in ("linux", "darwin"): | ||
| # Platform-specific stubs for Linux and macOS | ||
| ... | ||
|
|
||
| **Supported patterns:** | ||
|
|
||
| * Equality: ``sys.platform == "linux"`` | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here also, I think it would be useful for the spec to be specific that
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense. |
||
| * Inequality: ``sys.platform != "win32"`` | ||
| * Membership: ``sys.platform in ("linux", "darwin")`` | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not convinced that containment should be supported here. There's already a way to express this in a way that all tools today support. I understand the argument that this is less verbose, but it's not that common for checks to include more than one platform, so I don't think there's a compelling argument to force all tools to support this additional form.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For deciding on issues like this it would be helpful to have a little summary of what type checkers currently support (like what I did in https://discuss.python.org/t/spec-change-clarify-that-tuple-should-not-be-prohibited-as-an-argument-to-type/105590/7). I'll gather a few variants and summarize it. I think part of this change will be codifying what is already supported universally, and another part will be adding support for more features. The first group should be uncontroversial, and in the second group we should only force work on type checker authors if there's a clear use case.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If possible this would really help with creating readable and maintainable type stubs for MicroPython. While the below would work the below is much simpler to understand and maintain. I think it would be reasonable to explicitly restrict this to "a tuple of literal strings",assuming that simplies the implementation. |
||
| * Negative membership: ``sys.platform not in ("win32", "cygwin")`` | ||
|
|
||
| sys.implementation.name checks | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think adding this has does have significant ecosystem costs (as outlined by @erictraut in https://discuss.python.org/t/proposal-to-improve-support-for-other-python-platforms-in-the-typing-specification/91877/3 ). In the end I think it is probably worth it, because without it, type-checking for alternative implementations of Python seems quite difficult. The cost to type checkers themselves seems relatively low: one new config option, defaulting to
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The ecosystem is larger than just one implementation. |
||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Type checkers should support ``sys.implementation.name`` comparisons:: | ||
|
|
||
| import sys | ||
| if sys.implementation.name == "cpython": | ||
| # CPython-specific stub | ||
| if sys.implementation.name == "micropython": | ||
| # MicroPython-specific stub | ||
|
|
||
| **Supported patterns:** | ||
|
|
||
| * Equality: ``sys.implementation.name == "cpython"`` | ||
| * Inequality: ``sys.implementation.name != "cpython"`` | ||
| * Membership: ``sys.implementation.name in ("pypy", "graalpy")`` | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here again, I don't think there's good justification for supporting a containment operator.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I addedd this for consistency, but I'm OK to remove |
||
| * Negative membership: ``sys.implementation.name not in ("cpython", "pypy")`` | ||
|
|
||
| Common values: ``"cpython"``, ``"pypy"``, ``"micropython"``, ``"graalpy"``, ``"jython"``, ``"ironpython"`` | ||
|
|
||
| Configuration | ||
| ^^^^^^^^^^^^^ | ||
|
|
||
| Type checkers should provide configuration to specify target version, platform, and implementation. The exact mechanism is implementation-defined. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Target version to what granularity?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point, I think; as you suggested; Major.Minor |
||
|
|
||
| Complex Expressions | ||
| ^^^^^^^^^^^^^^^^^^^ | ||
|
|
||
| Type checkers are not required to evaluate complex expressions involving these variables. | ||
| Therefore do not expect a checker to understand obfuscations like: | ||
| ``"".join(reversed(sys.platform)) == "xunil"``. | ||
|
|
||
| .. _`deprecated`: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's important for the language to be very specific about the exact syntactical forms that are supported. Type checkers need to evaluate these expressions early in the analysis process, prior to type evaluation. That means they need to do exact tree matching of the AST. That means it's important to reduce the variations that are supported. For example,
sys.version_info <= (3, 10)is fine, butversion_info <= (3, 10)is not, nor is(3, 10) > version_info.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(None of this is true for how ty implements
sys.version_infoorsys.platformbranches, FWIW)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that changes anything about what should be supported in the spec though, since it's true for all other type checkers.