From 1e79e8d008df760ee8a8c834881d24d89bc08cb1 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 2 Aug 2023 20:30:35 -0400 Subject: [PATCH] Tweaks --- .../src/checkers/ast/analyze/statement.rs | 6 +- .../rules/custom_type_var_return_type.rs | 212 ++++++++++++++++++ .../flake8_pyi/rules/custom_typevar_return.rs | 203 ----------------- crates/ruff/src/rules/flake8_pyi/rules/mod.rs | 4 +- ..._flake8_pyi__tests__PYI019_PYI019.pyi.snap | 10 +- 5 files changed, 222 insertions(+), 213 deletions(-) create mode 100644 crates/ruff/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs delete mode 100644 crates/ruff/src/rules/flake8_pyi/rules/custom_typevar_return.rs diff --git a/crates/ruff/src/checkers/ast/analyze/statement.rs b/crates/ruff/src/checkers/ast/analyze/statement.rs index 12fdf21ad5ecd..e697d6a476e2d 100644 --- a/crates/ruff/src/checkers/ast/analyze/statement.rs +++ b/crates/ruff/src/checkers/ast/analyze/statement.rs @@ -158,13 +158,13 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) { ); } if checker.enabled(Rule::CustomTypeVarReturnType) { - flake8_pyi::rules::custom_typevar_return_type( + flake8_pyi::rules::custom_type_var_return_type( checker, name, decorator_list, returns.as_ref().map(AsRef::as_ref), - args, - type_params, + parameters, + type_params.as_ref(), ); } if checker.enabled(Rule::StrOrReprDefinedInStub) { diff --git a/crates/ruff/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs b/crates/ruff/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs new file mode 100644 index 0000000000000..4ba24634a64ec --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs @@ -0,0 +1,212 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast as ast; +use ruff_python_ast::helpers::map_subscript; +use ruff_python_ast::{ + Decorator, Expr, ParameterWithDefault, Parameters, Ranged, TypeParam, TypeParams, +}; +use ruff_python_semantic::analyze::visibility::{ + is_abstract, is_classmethod, is_new, is_overload, is_staticmethod, +}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for methods that define a custom `TypeVar` for their return type +/// annotation instead of using `typing_extensions.Self`. +/// +/// ## Why is this bad? +/// If certain methods are annotated with a custom `TypeVar` type, and the +/// class is subclassed, type checkers will not be able to infer the correct +/// return type. +/// +/// This check currently applies to instance methods that return `self`, class +/// methods that return an instance of `cls`, and `__new__` methods. +/// +/// ## Example +/// ```python +/// class Foo: +/// def __new__(cls: type[_S], *args: str, **kwargs: int) -> _S: +/// ... +/// +/// def foo(self: _S, arg: bytes) -> _S: +/// ... +/// +/// @classmethod +/// def bar(cls: type[_S], arg: int) -> _S: +/// ... +/// ``` +/// +/// Use instead: +/// ```python +/// from typing import Self +/// +/// +/// class Foo: +/// def __new__(cls: type[Self], *args: str, **kwargs: int) -> Self: +/// ... +/// +/// def foo(self: Self, arg: bytes) -> Self: +/// ... +/// +/// @classmethod +/// def bar(cls: type[Self], arg: int) -> Self: +/// ... +/// ``` +#[violation] +pub struct CustomTypeVarReturnType { + method_name: String, +} + +impl Violation for CustomTypeVarReturnType { + #[derive_message_formats] + fn message(&self) -> String { + let CustomTypeVarReturnType { method_name } = self; + format!( + "Methods like `{method_name}` should return `typing.Self` instead of a custom `TypeVar`" + ) + } +} + +/// PYI019 +pub(crate) fn custom_type_var_return_type( + checker: &mut Checker, + name: &str, + decorator_list: &[Decorator], + returns: Option<&Expr>, + args: &Parameters, + type_params: Option<&TypeParams>, +) { + if args.args.is_empty() && args.posonlyargs.is_empty() { + return; + } + + let Some(returns) = returns else { + return; + }; + + if !checker.semantic().scope().kind.is_class() { + return; + }; + + // Skip any abstract, static, and overloaded methods. + if is_abstract(decorator_list, checker.semantic()) + || is_overload(decorator_list, checker.semantic()) + || is_staticmethod(decorator_list, checker.semantic()) + { + return; + } + + let returns = map_subscript(returns); + + let uses_custom_var: bool = + if is_classmethod(decorator_list, checker.semantic()) || is_new(name) { + class_method(args, returns, type_params) + } else { + // If not static, or a class method or __new__ we know it is an instance method + instance_method(args, returns, type_params) + }; + + if uses_custom_var { + checker.diagnostics.push(Diagnostic::new( + CustomTypeVarReturnType { + method_name: name.to_string(), + }, + returns.range(), + )); + } +} + +/// Returns `true` if the class method is annotated with a custom `TypeVar` that is likely +/// private. +fn class_method( + args: &Parameters, + return_annotation: &Expr, + type_params: Option<&TypeParams>, +) -> bool { + let ParameterWithDefault { parameter, .. } = &args.args[0]; + + let Some(annotation) = ¶meter.annotation else { + return false; + }; + + let Expr::Subscript(ast::ExprSubscript { slice, value, .. }) = annotation.as_ref() else { + return false; + }; + + let Expr::Name(value) = value.as_ref() else { + return false; + }; + + // Don't error if the first argument is annotated with typing.Type[T]. + // These are edge cases, and it's hard to give good error messages for them. + if value.id != "type" { + return false; + }; + + let Expr::Name(slice) = slice.as_ref() else { + return false; + }; + + let Expr::Name(return_annotation) = return_annotation else { + return false; + }; + + if slice.id != return_annotation.id { + return false; + } + + is_likely_private_typevar(&slice.id, type_params) +} + +/// Returns `true` if the instance method is annotated with a custom `TypeVar` that is likely +/// private. +fn instance_method( + args: &Parameters, + return_annotation: &Expr, + type_params: Option<&TypeParams>, +) -> bool { + let ParameterWithDefault { parameter, .. } = &args.args[0]; + + let Some(annotation) = ¶meter.annotation else { + return false; + }; + + let Expr::Name(ast::ExprName { + id: first_arg_type, .. + }) = annotation.as_ref() + else { + return false; + }; + + let Expr::Name(ast::ExprName { + id: return_type, .. + }) = return_annotation + else { + return false; + }; + + if first_arg_type != return_type { + return false; + } + + is_likely_private_typevar(first_arg_type, type_params) +} + +/// Returns `true` if the type variable is likely private. +fn is_likely_private_typevar(type_var_name: &str, type_params: Option<&TypeParams>) -> bool { + // Ex) `_T` + if type_var_name.starts_with('_') { + return true; + } + // Ex) `class Foo[T]: ...` + type_params.is_some_and(|type_params| { + type_params.iter().any(|type_param| { + if let TypeParam::TypeVar(ast::TypeParamTypeVar { name, .. }) = type_param { + name == type_var_name + } else { + false + } + }) + }) +} diff --git a/crates/ruff/src/rules/flake8_pyi/rules/custom_typevar_return.rs b/crates/ruff/src/rules/flake8_pyi/rules/custom_typevar_return.rs deleted file mode 100644 index 8b5ebda1343c3..0000000000000 --- a/crates/ruff/src/rules/flake8_pyi/rules/custom_typevar_return.rs +++ /dev/null @@ -1,203 +0,0 @@ -use crate::checkers::ast::Checker; -use crate::settings::types::PythonVersion; -use ruff_diagnostics::{Diagnostic, Violation}; -use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast as ast; -use ruff_python_ast::{ArgWithDefault, Arguments, Decorator, Expr, Ranged, TypeParam}; -use ruff_python_semantic::analyze::visibility::{ - is_abstract, is_classmethod, is_overload, is_staticmethod, -}; -use ruff_python_semantic::ScopeKind; - -/// ## What it does -/// Checks if certain methods define a custom `TypeVar`s for their return annotation instead of -/// using `typing_extensions.Self`. This check currently applies for instance methods that return -/// `self`, class methods that return an instance of `cls`, and `__new__` methods. -/// -/// ## Why is this bad? -/// If certain methods are annotated with a custom `TypeVar` type, and the class is subclassed, -/// type checkers will not be able to infer the correct return type. -/// -/// ## Example -/// ```python -/// class Foo: -/// def __new__(cls: type[_S], *args: str, **kwargs: int) -> _S: -/// ... -/// -/// def foo(self: _S, arg: bytes) -> _S: -/// ... -/// -/// @classmethod -/// def bar(cls: type[_S], arg: int) -> _S: -/// ... -/// ``` -/// -/// Use instead: -/// ```python -/// from typing import Self -/// -/// -/// class Foo: -/// def __new__(cls: type[Self], *args: str, **kwargs: int) -> Self: -/// ... -/// -/// def foo(self: Self, arg: bytes) -> Self: -/// ... -/// -/// @classmethod -/// def bar(cls: type[Self], arg: int) -> Self: -/// ... -/// ``` -#[violation] -pub struct CustomTypeVarReturnType { - method_name: String, -} - -impl Violation for CustomTypeVarReturnType { - #[derive_message_formats] - fn message(&self) -> String { - let CustomTypeVarReturnType { method_name } = self; - format!( - "Methods like {method_name} should return `typing.Self` instead of a custom TypeVar" - ) - } -} - -/// PYI019 -pub(crate) fn custom_typevar_return_type( - checker: &mut Checker, - name: &str, - decorator_list: &[Decorator], - returns: Option<&Expr>, - args: &Arguments, - type_params: &[TypeParam], -) { - let ScopeKind::Class(_) = checker.semantic().scope().kind else { - return; - }; - - if args.args.is_empty() && args.posonlyargs.is_empty() { - return; - } - - let Some(returns) = returns else { - return; - }; - - let return_annotation = if let Expr::Subscript(ast::ExprSubscript { value, .. }) = returns { - // Ex) `Type[T]` - value - } else { - // Ex) `Type`, `typing.Type` - returns - }; - - // Skip any abstract, static and overloaded methods. - if is_abstract(decorator_list, checker.semantic()) - || is_overload(decorator_list, checker.semantic()) - || is_staticmethod(decorator_list, checker.semantic()) - { - return; - } - - let is_violation: bool = - if is_classmethod(decorator_list, checker.semantic()) || name == "__new__" { - check_class_method_for_bad_typevars(checker, args, return_annotation, type_params) - } else { - // If not static, or a class method or __new__ we know it is an instance method - check_instance_method_for_bad_typevars(checker, args, return_annotation, type_params) - }; - - if is_violation { - checker.diagnostics.push(Diagnostic::new( - CustomTypeVarReturnType { - method_name: name.to_string(), - }, - return_annotation.range(), - )); - } -} - -fn check_class_method_for_bad_typevars( - checker: &Checker, - args: &Arguments, - return_annotation: &Expr, - type_params: &[TypeParam], -) -> bool { - let ArgWithDefault { def, .. } = &args.args[0]; - - let Some(annotation) = &def.annotation else { - return false - }; - - let Expr::Subscript(ast::ExprSubscript{slice, value, ..}) = annotation.as_ref() else { - return false - }; - - let Expr::Name(ast::ExprName{ id: id_value, .. }) = value.as_ref() else { - return false - }; - - // Don't error if the first argument is annotated with typing.Type[T] - // These are edge cases, and it's hard to give good error messages for them. - if id_value != "type" { - return false; - }; - - let Expr::Name(ast::ExprName { id: id_slice, .. }) = slice.as_ref() else { - return false - }; - - let Expr::Name(ast::ExprName { id: return_type, .. }) = return_annotation else { - return false - }; - - return_type == id_slice && is_likely_private_typevar(checker, id_slice, type_params) -} - -fn check_instance_method_for_bad_typevars( - checker: &Checker, - args: &Arguments, - return_annotation: &Expr, - type_params: &[TypeParam], -) -> bool { - let ArgWithDefault { def, .. } = &args.args[0]; - - let Some(annotation) = &def.annotation else { - return false - }; - - let Expr::Name(ast::ExprName{id: first_arg_type,..}) = annotation.as_ref() else { - return false - }; - - let Expr::Name(ast::ExprName { id: return_type, .. }) = return_annotation else { - return false - }; - - if first_arg_type != return_type { - return false; - } - - is_likely_private_typevar(checker, first_arg_type, type_params) -} - -fn is_likely_private_typevar( - checker: &Checker, - tvar_name: &str, - type_params: &[TypeParam], -) -> bool { - if tvar_name.starts_with('_') { - return true; - } - if checker.settings.target_version < PythonVersion::Py312 { - return false; - } - - for param in type_params { - if let TypeParam::TypeVar(ast::TypeParamTypeVar { name, .. }) = param { - return name == tvar_name; - } - } - false -} diff --git a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs index 8e2fd9d7157d0..e340427d0499f 100644 --- a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs @@ -3,7 +3,7 @@ pub(crate) use bad_version_info_comparison::*; pub(crate) use collections_named_tuple::*; pub(crate) use complex_assignment_in_stub::*; pub(crate) use complex_if_statement_in_stub::*; -pub(crate) use custom_typevar_return::*; +pub(crate) use custom_type_var_return_type::*; pub(crate) use docstring_in_stubs::*; pub(crate) use duplicate_union_member::*; pub(crate) use ellipsis_in_non_empty_class_body::*; @@ -38,7 +38,7 @@ mod bad_version_info_comparison; mod collections_named_tuple; mod complex_assignment_in_stub; mod complex_if_statement_in_stub; -mod custom_typevar_return; +mod custom_type_var_return_type; mod docstring_in_stubs; mod duplicate_union_member; mod ellipsis_in_non_empty_class_body; diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI019_PYI019.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI019_PYI019.pyi.snap index a87f86e3a5f1e..121ee656304d4 100644 --- a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI019_PYI019.pyi.snap +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI019_PYI019.pyi.snap @@ -1,27 +1,27 @@ --- source: crates/ruff/src/rules/flake8_pyi/mod.rs --- -PYI019.pyi:7:62: PYI019 Methods like __new__ should return `typing.Self` instead of a custom TypeVar +PYI019.pyi:7:62: PYI019 Methods like `__new__` should return `typing.Self` instead of a custom `TypeVar` | 6 | class BadClass: 7 | def __new__(cls: type[_S], *args: str, **kwargs: int) -> _S: ... # PYI019 | ^^ PYI019 | -PYI019.pyi:10:54: PYI019 Methods like bad_instance_method should return `typing.Self` instead of a custom TypeVar +PYI019.pyi:10:54: PYI019 Methods like `bad_instance_method` should return `typing.Self` instead of a custom `TypeVar` | 10 | def bad_instance_method(self: _S, arg: bytes) -> _S: ... # PYI019 | ^^ PYI019 | -PYI019.pyi:14:54: PYI019 Methods like bad_class_method should return `typing.Self` instead of a custom TypeVar +PYI019.pyi:14:54: PYI019 Methods like `bad_class_method` should return `typing.Self` instead of a custom `TypeVar` | 13 | @classmethod 14 | def bad_class_method(cls: type[_S], arg: int) -> _S: ... # PYI019 | ^^ PYI019 | -PYI019.pyi:35:63: PYI019 Methods like __new__ should return `typing.Self` instead of a custom TypeVar +PYI019.pyi:35:63: PYI019 Methods like `__new__` should return `typing.Self` instead of a custom `TypeVar` | 33 | # Python > 3.12 34 | class PEP695BadDunderNew[T]: @@ -29,7 +29,7 @@ PYI019.pyi:35:63: PYI019 Methods like __new__ should return `typing.Self` instea | ^ PYI019 | -PYI019.pyi:38:46: PYI019 Methods like generic_instance_method should return `typing.Self` instead of a custom TypeVar +PYI019.pyi:38:46: PYI019 Methods like `generic_instance_method` should return `typing.Self` instead of a custom `TypeVar` | 38 | def generic_instance_method[S](self: S) -> S: ... # PYI019 | ^ PYI019