-
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
[pyupgrade] Fix false negative for TypeVar with default argument in non-pep695-generic-class (UP046)
#20660
Changes from 2 commits
6e1a204
8464d97
71c7412
8a7b495
a7175cf
60c7ba8
02be883
80fcb79
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 |
|---|---|---|
|
|
@@ -122,6 +122,10 @@ impl Display for DisplayTypeVar<'_> { | |
| } | ||
| } | ||
| } | ||
| if let Some(default) = self.type_var.default { | ||
| f.write_str(" = ")?; | ||
| f.write_str(&self.source[default.range()])?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
@@ -133,66 +137,62 @@ impl<'a> From<&'a TypeVar<'a>> for TypeParam { | |
| name, | ||
| restriction, | ||
| kind, | ||
| default: _, // TODO(brent) see below | ||
| default, | ||
| }: &'a TypeVar<'a>, | ||
| ) -> Self { | ||
| match kind { | ||
| TypeParamKind::TypeVar => { | ||
| TypeParam::TypeVar(TypeParamTypeVar { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| name: Identifier::new(*name, TextRange::default()), | ||
| bound: match restriction { | ||
| Some(TypeVarRestriction::Bound(bound)) => Some(Box::new((*bound).clone())), | ||
| Some(TypeVarRestriction::Constraint(constraints)) => { | ||
| Some(Box::new(Expr::Tuple(ast::ExprTuple { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| elts: constraints.iter().map(|expr| (*expr).clone()).collect(), | ||
| ctx: ast::ExprContext::Load, | ||
| parenthesized: true, | ||
| }))) | ||
| } | ||
| Some(TypeVarRestriction::AnyStr) => { | ||
| Some(Box::new(Expr::Tuple(ast::ExprTuple { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| elts: vec![ | ||
| Expr::Name(ExprName { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| id: Name::from("str"), | ||
| ctx: ast::ExprContext::Load, | ||
| }), | ||
| Expr::Name(ExprName { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| id: Name::from("bytes"), | ||
| ctx: ast::ExprContext::Load, | ||
| }), | ||
| ], | ||
| ctx: ast::ExprContext::Load, | ||
| parenthesized: true, | ||
| }))) | ||
| } | ||
| None => None, | ||
| }, | ||
| // We don't handle defaults here yet. Should perhaps be a different rule since | ||
| // defaults are only valid in 3.13+. | ||
| default: None, | ||
| }) | ||
| } | ||
| TypeParamKind::TypeVar => TypeParam::TypeVar(TypeParamTypeVar { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| name: Identifier::new(*name, TextRange::default()), | ||
| bound: match restriction { | ||
| Some(TypeVarRestriction::Bound(bound)) => Some(Box::new((*bound).clone())), | ||
| Some(TypeVarRestriction::Constraint(constraints)) => { | ||
| Some(Box::new(Expr::Tuple(ast::ExprTuple { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| elts: constraints.iter().map(|expr| (*expr).clone()).collect(), | ||
| ctx: ast::ExprContext::Load, | ||
| parenthesized: true, | ||
| }))) | ||
| } | ||
| Some(TypeVarRestriction::AnyStr) => { | ||
| Some(Box::new(Expr::Tuple(ast::ExprTuple { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| elts: vec![ | ||
| Expr::Name(ExprName { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| id: Name::from("str"), | ||
| ctx: ast::ExprContext::Load, | ||
| }), | ||
| Expr::Name(ExprName { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| id: Name::from("bytes"), | ||
| ctx: ast::ExprContext::Load, | ||
| }), | ||
| ], | ||
| ctx: ast::ExprContext::Load, | ||
| parenthesized: true, | ||
| }))) | ||
| } | ||
| None => None, | ||
| }, | ||
| default: default.map(|expr| Box::new((*expr).clone())), | ||
| }), | ||
| TypeParamKind::TypeVarTuple => TypeParam::TypeVarTuple(TypeParamTypeVarTuple { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| name: Identifier::new(*name, TextRange::default()), | ||
| default: None, | ||
| default: default.map(|expr| Box::new((*expr).clone())), | ||
| }), | ||
| TypeParamKind::ParamSpec => TypeParam::ParamSpec(TypeParamParamSpec { | ||
| range: TextRange::default(), | ||
| node_index: ruff_python_ast::AtomicNodeIndex::NONE, | ||
| name: Identifier::new(*name, TextRange::default()), | ||
| default: None, | ||
| default: default.map(|expr| Box::new((*expr).clone())), | ||
| }), | ||
| } | ||
| } | ||
|
|
@@ -318,8 +318,8 @@ pub(crate) fn expr_name_to_type_var<'a>( | |
| .first() | ||
| .is_some_and(Expr::is_string_literal_expr) | ||
| { | ||
| // TODO(brent) `default` was added in PEP 696 and Python 3.13 but can't be used in | ||
| // generic type parameters before that | ||
| // `default` was added in PEP 696 and Python 3.13. We now support converting | ||
| // TypeVars with defaults to PEP 695 type parameters. | ||
| // | ||
| // ```python | ||
| // T = TypeVar("T", default=Any, bound=str) | ||
|
|
@@ -373,15 +373,8 @@ fn check_type_vars(vars: Vec<TypeVar<'_>>) -> Option<Vec<TypeVar<'_>>> { | |
| } | ||
|
|
||
| // If any type variables were not unique, just bail out here. this is a runtime error and we | ||
| // can't predict what the user wanted. also bail out if any Python 3.13+ default values are | ||
| // found on the type parameters | ||
| (vars | ||
| .iter() | ||
| .unique_by(|tvar| tvar.name) | ||
| .filter(|tvar| tvar.default.is_none()) | ||
| .count() | ||
| == 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) | ||
|
Contributor
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. 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. |
||
| } | ||
|
|
||
| /// Search `class_bases` for a `typing.Generic` base class. Returns the `Generic` expression (if | ||
|
|
||
|
Contributor
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 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. |
Uh oh!
There was an error while loading. Please reload this page.