Repository navigation
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 5 commits
e34cbc7
33c4477
2c4e700
98e3697
4ca8fd6
c0639dd
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -154,23 +154,160 @@ 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 understand code paths as definitely reachable or not reachable due to comparison tests against these symbols: | ||||||
| * ``sys.version_info`` | ||||||
| * ``sys.platform`` | ||||||
| * ``sys.implementation.version`` | ||||||
| * ``sys.implementation.name`` | ||||||
|
|
||||||
| import sys | ||||||
| Type checkers should support combining these checks with: | ||||||
| * A ``not`` unary operator | ||||||
| * An ``and`` or ``or`` binary operator | ||||||
|
|
||||||
| if sys.version_info >= (3, 12): | ||||||
| # Python 3.12+ | ||||||
| else: | ||||||
| # Python 3.11 and lower | ||||||
| Type checkers are only required to support the fully-qualified form (e.g., ``sys.platform``). | ||||||
| Support for aliases or import variants (e.g., ``from sys import platform``) is not required, though type checkers may choose to support them. | ||||||
|
|
||||||
| if sys.platform == 'win32': | ||||||
| # Windows specific definitions | ||||||
| else: | ||||||
| # Posix specific definitions | ||||||
| The comparison patterns for these variables are described in more detail in the following paragraphs. | ||||||
|
|
||||||
| Don't expect a checker to understand obfuscations like | ||||||
| ``"".join(reversed(sys.platform)) == "xunil"``. | ||||||
| sys.version_info checks | ||||||
| ^^^^^^^^^^^^^^^^^^^^^^^^ | ||||||
|
|
||||||
| Type checkers should support the following comparison patterns: | ||||||
| * ``sys.version_info >= <2-tuple>`` | ||||||
|
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 leaves the meaning of I think the intent here is that it must be an actual tuple literal containing two integer literals. We should make that explicit.
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 understand your point, but in not quite sure what notation works best for this , Is it suffient to say or would this be clearer :
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 would probably just append ", where |
||||||
| * ``sys.version_info < <2-tuple>`` | ||||||
|
|
||||||
| Comparison checks are only supported against the first two elements of the version tuple. | ||||||
| Type checkers may choose to also support the 3-tuple ``sys.version_info >= <3-tuple>``. | ||||||
| Type checkers are not expected to support comparisons with named attributes of `sys.version_info`. | ||||||
|
|
||||||
| .. code-block:: python | ||||||
| :caption: Example `sys.version_info` | ||||||
| :emphasize-lines: 2 | ||||||
|
|
||||||
| import sys | ||||||
| if sys.version_info >= (3, 12): | ||||||
| # Python 3.12+ | ||||||
| elif sys.version_info >= (3, 11): | ||||||
| # Python 3.11 | ||||||
| else: | ||||||
| # Python 3.10 and lower | ||||||
|
|
||||||
| sys.platform checks | ||||||
| ^^^^^^^^^^^^^^^^^^^ | ||||||
|
|
||||||
| Type checkers should support the following comparison patterns: | ||||||
| * ``sys.platform == <string literal>`` | ||||||
| * ``sys.platform != <string literal>`` | ||||||
| * ``sys.platform.startswith(<string literal>)`` | ||||||
| * ``sys.platform in <tuple of string literals>`` | ||||||
| * ``sys.platform not in <tuple of string literals>`` | ||||||
|
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. Ruff (PLR6201) will report an error for this tuple membership check, and I tend to agree with ruff that a set literal would be more idiomatic here.
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. In the wild I've seen things like
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. ty already supports Supporting set literals will be a little tricky in ty, but should be doable, and I'm not opposed to requiring support for it. I don't think the performance motivation of PLR6201 typically applies much 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. change to set and added
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. To be clear, I think that both tuple and set literals should be specified as supported -- it would be surprising IMO if sets work and tuples don't. This is perhaps an argument against supporting sets -- it opens a bit of a slippery slope: why not lists? etc
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. my original proposal was tuples, as that is one of the core building blocks of Python. I'd like it to be straight and narrow though , Pausing edits until there is consensus
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. After considering it more, I think we should just leave sets out entirely. I wouldn't even mention them as optional: there's no point, since type checkers are always free to support anything they want beyond the spec. I don't believe there is any actual performance benefit at this scale, and it's not necessary to add complexity to the spec or to type checkers when comparing to a tuple is perfectly usable and functional. The ruff rule is unstable and I think if this spec change is merged, the ruff rule should just be updated to specifically exclude these typing-supported comparisons, so there is no conflict. I don't think the ruff rule should motivate adding sets to the spec. |
||||||
| Type checkers may also support the following comparison patterns: | ||||||
| * ``sys.platform in <set of string literals>`` | ||||||
| * ``sys.platform not in <set of string literals>`` | ||||||
|
|
||||||
| Common values: ``"linux"``, ``"darwin"``, ``"win32"``, ``"emscripten"``, ``"wasi"`` | ||||||
|
|
||||||
| The membership checks ``in`` and ``not in`` only support simple containment testing with a set of literal strings. | ||||||
|
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 should be updated for consistency with the decision about sets vs tuples.
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. So can I consider the tuple/set discussion decided in favor of (literal) tuples ony?
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. @carljm You wrote:
What is your reasoning about that? Why not support sets/lists as well? To me that would have been the most natural way. I understand that type checkers may support more constructs, but I think it's generally preferable for type checkers to have the same behavior. My intuition is that normal users would not see a difference between sets / tuples / lists and just use the one they feel most comfortable.
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. @davidhalter This same logic could be used to expand the required support in various other ways, too. It's not very intuitive to support Ultimately we are talking about special casing sufficient patterns to allow expressing what needs to be expressed; we have to draw the line somewhere, so why require support for extra things that don't add expressive utility? If sets and lists, why not dictionary keys? Why not variables that have been assigned a list or set or tuple? All of these are also intuitive extensions from a user perspective.
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.
Because nobody in their right mind would use I just don't really understand why we should omit this. Do you have reservations because of inference of sets/lists where you would do something like Jelle implemented it for Mypy already and it looks pretty straightforward: python/mypy#21913
I'm open to that as well. I probably disagree that it's more obvious, but I do agree that it's unintuitive.
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 think if you support I still lean towards supporting lists/sets, but I'm totally fine if people disagree and we go the tuple-only way. Maybe @jorenham, @JelleZijlstra or @rchen152 can quickly weigh in here.
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'd prefer also supporting lists and sets. That way, users will be more likely to have code work the way they expect regardless of arbitrary choices, and that's generally a good thing. The cost is that type checkers will need to implement some more complicated support, but that cost seems low enough here that it's worth choosing the user-friendlier version.
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. Sets also have my preference, because for containment checks sets are the idiomatic choice, for which there's also a ruff rule: https://docs.astral.sh/ruff/rules/literal-membership/
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. Do I understand correctly that there is consensus to support tuple , set and list of literals ?
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. All typing council members have now commented in this thread in favor of (or fine with) going ahead with tuple/list/set support, except for @rchen152. Given pyrefly already supports all three, I would not expect @rchen152 to object. So I think it's safe to consider this consensus now. |
||||||
|
|
||||||
| .. code-block:: python | ||||||
| :caption: Example `sys.platform` | ||||||
| :emphasize-lines: 2,4 | ||||||
|
|
||||||
| import sys | ||||||
| if sys.platform == 'win32': | ||||||
| # Windows specific definitions | ||||||
| if sys.platform in ("linux", "darwin"): | ||||||
|
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. That's a tuple, not a set, and the current text says only sets are supported. My instinct would be to support both, but I wouldn't mind narrowing it down if that's the consensus.
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. Indeed it is no longer consistent due to other changes.
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 recently learned that a set in this case will br optimized using the peephole optimizer, so the performance benefits are real.
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. But ty cannot support it, so the disadvantages are even more real.
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 don't not what the peephole optimizer optimizes here, but unless I see actual numbers I would not be surprised if small tuples are as fast.
Please do not argue like that. There are good arguments above why we should or should not support sets. It is possible for Ty to support it and Carl even argued that it likely should be supported (and then argued against it for another reason). Even if Ty doesn't support it, it still might be helpful to have it in the spec, because other people use other type checkers.
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 have changed this section to: Type checkers must support the following comparison patterns:
<snip>
* ``sys.platform in <tuple of string literals>``
* ``sys.platform not in <tuple of string literals>``
Type checkers may also support the following comparison patterns:
* ``sys.platform in <set of string literals>``
* ``sys.platform not in <set of string literals>`` |
||||||
| # Platform-specific stubs for Linux and macOS | ||||||
| ... | ||||||
|
|
||||||
|
|
||||||
| sys.implementation.name checks | ||||||
|
Josverl marked this conversation as resolved.
|
||||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||||||
|
|
||||||
| Type checkers should support comparison patterns: | ||||||
|
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.
Suggested change
|
||||||
| * ``sys.implementation.name == <string literal>`` | ||||||
| * ``sys.implementation.name != <string literal>`` | ||||||
| * ``sys.implementation.name in <set of string literals>`` | ||||||
|
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 should also be updated for consistency with the sets vs tuples discussion above. |
||||||
| * ``sys.implementation.name not in <set of string literals>`` | ||||||
|
|
||||||
| Default value: ``"cpython"``, unless configured otherwise. | ||||||
| Common values: ``"cpython"``, ``"pypy"``, ``"micropython"``, ``"graalpy"``, ``"jython"``, ``"ironpython"`` | ||||||
|
|
||||||
|
|
||||||
| .. code-block:: python | ||||||
| :caption: Example `sys.implementation.name` | ||||||
| :emphasize-lines: 2,4 | ||||||
|
|
||||||
| import sys | ||||||
| if sys.implementation.name == "cpython": | ||||||
| # CPython-specific stub | ||||||
| if sys.implementation.name == "micropython": | ||||||
| # MicroPython-specific stub | ||||||
|
Josverl marked this conversation as resolved.
|
||||||
|
|
||||||
|
|
||||||
| sys.implementation.version 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. Same as above, I think we should probably be explicit here that when a type checker has no "implementation" information, it should assume "CPython, and implementation version matches It's less clear to me what should happen if a type-checker is told that the implementation is not CPython, but is not given any specific version information. I guess this could be an error? Otherwise I'm not sure how type-checkers should guess at the implementation version.
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. there should be no guessing involved, And defaulting to CPyton would be the most logical thing to do. As an example a device that happens to be connected: If I want to typecheck an app for this device and firmware I would need to supply the typechecker with:
using the relevant configuration options for that checker
If not provided explicit information through: typechecker config, environment or switches, or detected python runtime sys.implementation.version should fall-back to sys.version.
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. Added a table in the config section to add clarity.
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. Is defaulting to sys.version_info really the right call here? On MicroPython that's apparently meaningless, since the version is 1.x instead of 3.x.
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 can not think of anything other than a cross implementation lookup table, that would come with its own maintenance and distribution chalenges, to solve this for all or even most Python implementations. And as I mentioned before - it is quite similar to type checking for Windows+ Python 3.10 from 3.14 venv on Linux
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. Commented on the table below, but I agree with @JelleZijlstra that "fallback 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. I think there might be a misunderstanding; On CPython default to the value used for sys.version. On other implementations, the value must be provided according to the type checker’s configuration options. I hope that sufficiently clarifies this , if not; I would welcome a text suggestion to improve. |
||||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||||||
|
|
||||||
| ``sys.implementation.version`` is a tuple, in the same format as sys.version_info. However it represents the version of the Python implementation | ||||||
|
Josverl marked this conversation as resolved.
Outdated
|
||||||
| rather than the version of the Python language. This has a distinct meaning from the specific version of the Python language to which the currently | ||||||
| running interpreter conforms. For CPython (``sys.implementation.name == "cpython"``) this is the same as `sys.version_info`. | ||||||
|
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.
Suggested change
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 should be word-wrapped to around 80 columns -- it doesn't look like we've specified that anywhere, but that's the existing de facto convention in almost all spec files. |
||||||
|
|
||||||
| Type checkers should support the following comparison patterns: | ||||||
|
Josverl marked this conversation as resolved.
|
||||||
| * ``sys.implementation.version >= <2-tuple>`` | ||||||
| * ``sys.implementation.version < <2-tuple>`` | ||||||
|
|
||||||
| Comparison checks are only supported against the first two elements of the implementation version tuple. | ||||||
| Type checkers are not required to support comparisons against named attributes of `sys.implementation.version`. | ||||||
|
|
||||||
| .. code-block:: python | ||||||
| :caption: Example `sys.implementation.version` | ||||||
| :emphasize-lines: 2,4 | ||||||
|
|
||||||
| import sys | ||||||
| if sys.implementation.name == "pypy" and sys.implementation.version >= (7, 3): | ||||||
| # PyPy version 7.3 and above | ||||||
| if sys.implementation.name == "micropython" and sys.implementation.version >= (1, 24): | ||||||
| # MicroPython version 1.24 and above | ||||||
|
|
||||||
|
|
||||||
| No support for complex expressions | ||||||
|
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. Let's be more careful with the wording here. "Complex" is not clearly defined, so it's not clear what this heading is saying. Not all unsupported forms can be reasonably called "complex". And "no support" also implies forbidding type checkers from supporting something, which we are not ever doing.
Suggested change
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 have reworded the sub-section heading and prose and example comments based on the above feedback. |
||||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||||||
|
|
||||||
| Type checkers are required to support the above patterns, and are not required to evaluate other comparisons or other syntax variants. | ||||||
|
|
||||||
| Therefore checkers are **not required** to understand obfuscations such as: | ||||||
|
Josverl marked this conversation as resolved.
Outdated
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. My previous comment here was marked resolved but was not addressed. Not all of the patterns below are "obfuscations" and we should not describe them pejoratively.
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.
Aplogies for 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. as above : I have tried to improve. |
||||||
|
|
||||||
| .. code-block:: python | ||||||
| :caption: Examples of unsupported or overly complex version/platform 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.
Suggested change
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 have reworded the sub-section heading and prose and example comments based on the above feedback. |
||||||
| :emphasize-lines: 4,6,8 | ||||||
|
|
||||||
| import sys | ||||||
| from sys import platform | ||||||
|
|
||||||
| if "".join(reversed(sys.platform)) == "xunil": | ||||||
| # Typecheckers will not be required to understand this obfuscated check | ||||||
| if platform == "linux": | ||||||
| # Typecheckers will not be required to understand this import alias for sys.platform | ||||||
| if "win" not in sys.platform: | ||||||
| # Typecheckers will not be required to understand this reversed membership check | ||||||
|
|
||||||
|
|
||||||
| Configuration | ||||||
| ^^^^^^^^^^^^^ | ||||||
|
|
||||||
| Type checkers must be able to retrieve the information from the python implementation's runtime environment, or provide configuration or CLI options to specify target ``sys.version``, ``sys.platform``, ``sys.implementation.name`` and ``sys.implementation.version``. | ||||||
|
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. Should be Also word-wrap to 80 columns (throughout).
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. Corrected to version_info In order to blanace width with readability, I have wrapped the table at 110 columns, which is still slightly narrower than the HTML render. |
||||||
|
|
||||||
| ================================ ========================== ============== =========================================================================== | ||||||
| Symbol Suggested Format Example Suggested Default | ||||||
| ================================ ========================== ============== =========================================================================== | ||||||
| ``sys.version`` string ``"major.minor"`` ``"3.11"`` The version of the Python interpreter used to run the type checker. | ||||||
|
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.
Suggested change
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, corrected. |
||||||
| ``sys.platform`` lowercase string ``"linux"`` The platform of the Python interpreter used to run the type checker. | ||||||
| ``sys.implementation.name`` lowercase string ``"cpython"`` ``"cpython"`` unless configured otherwise. | ||||||
| ``sys.implementation.version`` string ``"major.minor"`` ``"3.14"`` The value used for ``sys.version`` unless configured otherwise. | ||||||
|
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 the fallback to It's simply wrong to fall back to
Suggested change
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. That was never my intent and I do not think I ever mentioned that in the spec. |
||||||
| ================================ ========================== ============== =========================================================================== | ||||||
|
|
||||||
| The configuration options should allow users to specify the target values for these symbols, so that type checkers can evaluate the version and platform checks correctly. | ||||||
| The exact mechanism and name for these configuration options is implementation-specific, and defined by each type checker. | ||||||
|
|
||||||
| .. _`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.
It seems like we haven't really discussed the conformance suite in this PR yet, but I think we should add conformance tests for whatever new requirements we decide on in this PR.
conformance/tests/directives_version_platform.pycurrently covers basic version and platform comparisons, but none of the implementation checks, membership checks,startswith, or boolean combinations added here.I would expect coverage of both reachable and unreachable branches, including combinations using
not,and, andor. It would also be useful to distinguish required tuple membership from optional set membership and three-element version comparisons, so the tests pin down the intended minimum support.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.
Makes sense to me, but TBH I personally would need to study how to define these, And happy to do that either in this PR or as a follow-up , but perhaps agree on the 'rules' first