-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[pyupgrade] Fix false negative for TypeVar with default argument in non-pep695-generic-class (UP046)
#20660
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
Conversation
|
ntBre
left a comment
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.
Thank you, this is looking good. I guess I expected this to be more complicated to have left the TODO around for so long 😆 I thought we would need an explicit Python 3.13 check somewhere, but it looks like the default argument to the typing.TypeVar constructors was only added in 3.13 too. So it seems safe to assume we're on 3.13+ if we find one.
As I noted in the inline comments, I think we need to make this a preview change and also check on UP040 test cases. I think all three of UP040, UP046, and UP047 share this same infrastructure.
| == vars.len()) | ||
| .then_some(vars) | ||
| // can't predict what the user wanted. | ||
| (vars.iter().unique_by(|tvar| tvar.name).count() == vars.len()).then_some(vars) |
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.
We need to preview gate this as I mentioned on the issue. I think this might be the easiest place to do that, assuming every usage goes through this code path.
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.
As demonstrated here, this change also affects UP047. I think it should also affect UP040, so we should make sure we have test coverage for that as well.
ntBre
left a comment
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.
The code changes look good, but now that this is a preview change, we also need to run the test cases in preview mode.
ntBre
left a comment
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.
Thanks!
I just pushed a few commits adding more of the test cases to the preview tests and using the new assert_diagnostics_diff macro. I also cleaned up some of the resolved TODOs in the tests.
…rable * origin/main: Don't use codspeed or depot runners in CI jobs on forks (#20894) [ty] cache Type::is_redundant_with (#20477) Fix run-away for mutually referential instance attributes (#20645) [ty] Limit shown import paths to at most 5 unless ty runs with `-v` (#20912) [ty] Use field-specifier return type as the default type for the field (#20915) [ty] Do not assume that `field`s have a default value (#20914) [ty] Fix match pattern value narrowing to use equality semantics (#20882) Update setup instructions for Zed 0.208.0+ (#20902) Move TOML indent size config (#20905) [syntax-errors]: implement F702 as semantic syntax error (#20869) [ty] Heterogeneous unpacking support for unions (#20377) [ty] refactor `Place` (#20871) Auto-accept snapshot changes as part of typeshed-sync PRs (#20892) [`airflow`] Add warning to `airflow.datasets.DatasetEvent` usage (`AIR301`) (#20551) [`flake8-pyi`] Fix operator precedence by adding parentheses when needed (`PYI061`) (#20508) [`pyupgrade`] Fix false negative for `TypeVar` with default argument in `non-pep695-generic-class` (`UP046`) (#20660) Update parser snapshots (#20893) Fix syntax error false positives for escapes and quotes in f-strings (#20867)
…nt-sets * dcreager/non-non-inferable: Don't use codspeed or depot runners in CI jobs on forks (#20894) [ty] cache Type::is_redundant_with (#20477) Fix run-away for mutually referential instance attributes (#20645) [ty] Limit shown import paths to at most 5 unless ty runs with `-v` (#20912) [ty] Use field-specifier return type as the default type for the field (#20915) [ty] Do not assume that `field`s have a default value (#20914) [ty] Fix match pattern value narrowing to use equality semantics (#20882) Update setup instructions for Zed 0.208.0+ (#20902) Move TOML indent size config (#20905) [syntax-errors]: implement F702 as semantic syntax error (#20869) [ty] Heterogeneous unpacking support for unions (#20377) [ty] refactor `Place` (#20871) Auto-accept snapshot changes as part of typeshed-sync PRs (#20892) [`airflow`] Add warning to `airflow.datasets.DatasetEvent` usage (`AIR301`) (#20551) [`flake8-pyi`] Fix operator precedence by adding parentheses when needed (`PYI061`) (#20508) [`pyupgrade`] Fix false negative for `TypeVar` with default argument in `non-pep695-generic-class` (`UP046`) (#20660) Update parser snapshots (#20893) Fix syntax error false positives for escapes and quotes in f-strings (#20867)
Summary
Fixes #20656