[pyupgrade] Extend version detection to include sys.version_info.major (UP036)#18633
[pyupgrade] Extend version detection to include sys.version_info.major (UP036)#18633ntBre merged 4 commits intoastral-sh:mainfrom
pyupgrade] Extend version detection to include sys.version_info.major (UP036)#18633Conversation
added sys.version_info attribute .major support added fixtures to UP036_5.py updated snapshot issue: astral-sh#18165
|
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks for working on this! I think this looks good, I just had one suggestion for making the check a bit stricter. What do you think?
| .resolve_qualified_name(map_subscript(left)) | ||
| .is_some_and(|qualified_name| { | ||
| matches!(qualified_name.segments(), ["sys", "version_info"]) | ||
| matches!( | ||
| qualified_name.segments(), | ||
| ["sys", "version_info"] | ["sys", "version_info", "major"] | ||
| ) |
There was a problem hiding this comment.
This seems pretty minor, especially because the fix is always unsafe, but this also calls map_subscript on sys.version_info.major, which we might not really want to allow. In other words, I think this would fix code like
import sys
if sys.version_info.major[1] > 3:
print(3)
else:
print(2)to avoid the TypeError that would otherwise occur:
>>> import sys
>>> sys.version_info.major[1]
Traceback (most recent call last):
File "<python-input-1>", line 1, in <module>
sys.version_info.major[1]
~~~~~~~~~~~~~~~~~~~~~~^^^
TypeError: 'int' object is not subscriptableIt's much more convenient to add the check here, but I might lean slightly toward a separate resolve_qualified_name call to avoid map_subscript on major.
There was a problem hiding this comment.
This isn't related to your changes, but I guess this rule is pretty lenient with these comparisons anyway. For example, it still applies to comparisons like these that would also fail at runtime:
import sys
if sys.version_info < 3: ...
if sys.version_info[0] < (3, 0): ...There was a problem hiding this comment.
@ntBre Thanks for the review. Point taken. I've added a dedicated resolve_qualified_name call for the .major case. Could you take a look at the revised approach?
I also want to implement the fix for your second remark. Should this go into the current PR or would you prefer a separate PR?
added separate fn is_valid_version_info split and added separate .major check without map_subscript()
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Nice, thanks!
I also want to implement the fix for your second remark. Should this go into the current PR or would you prefer a separate PR?
Let's go ahead and merge the changes from this PR and leave the rest for a follow-up. We should probably also open an issue for the invalid comparisons to see if anyone else has feedback on possible fixes. There's currently a related rule (unrecognized-version-info-check (PYI003), and really PYI003-PYI006) that I think only applies in .pyi files. So there are already at least two options for detecting these issues: adding them to UP036 or PYI003.
crates/ruff_linter/src/rules/pyupgrade/rules/outdated_version_block.rs
Outdated
Show resolved
Hide resolved
…utdated_version_block.rs replace `checker` argument with `semantic` in the fn is_valid_version_info
|
@ntBre Seems one of the actions failed due to "curl: (22) The requested URL returned error: 403". Could you please rerun or skip it to merge? |
|
I got it. |
|
Oops, I guess I shouldn't have dismissed the notification until I saw it actually merge, sorry! It looks like the workflow might still be using an old version of the |
* main: [ty] Fix false positives when subscripting an object inferred as having an `Intersection` type (#18920) [`flake8-use-pathlib`] Add autofix for `PTH202` (#18763) [ty] Add relative import completion tests [ty] Clarify what "cursor" means [ty] Add a cursor test builder [ty] Enforce sort order of completions (#18917) [formatter] Fix missing blank lines before decorated classes in .pyi files (#18888) Apply fix availability and applicability when adding to `DiagnosticGuard` and remove `NoqaCode::rule` (#18834) py-fuzzer: allow relative executable paths (#18915) [ty] Change `environment.root` to accept multiple paths (#18913) [ty] Rename `src.root` setting to `environment.root` (#18760) Use file path for detecting package root (#18914) Consider virtual path for various server actions (#18910) [ty] Introduce `UnionType::try_from_elements` and `UnionType::try_map` (#18911) [ty] Support narrowing on `isinstance()`/`issubclass()` if the second argument is a dynamic, intersection, union or typevar type (#18900) [ty] Add decorator check for implicit attribute assignments (#18587) [`ruff`] Trigger `RUF037` for empty string and byte strings (#18862) [ty] Avoid duplicate diagnostic in unpacking (#18897) [`pyupgrade`] Extend version detection to include `sys.version_info.major` (`UP036`) (#18633) [`ruff`] Frozen Dataclass default should be valid (`RUF009`) (#18735)
3 participants
Summary
Resolves #18165
Added pattern
["sys", "version_info", "major"]to the existing matches forsys.version_infoto ensure consistent handling of both the base object and its major version attribute.Test Plan
cargo nextest runandcargo insta test