diff --git a/crates/ruff/resources/test/fixtures/pylint/type_bivariance.py b/crates/ruff/resources/test/fixtures/pylint/type_bivariance.py new file mode 100644 index 0000000000000..6084d97232580 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/type_bivariance.py @@ -0,0 +1,37 @@ +from typing import ParamSpec, TypeVar + +# Errors. + +T = TypeVar("T", covariant=True, contravariant=True) +T = TypeVar(name="T", covariant=True, contravariant=True) + +T = ParamSpec("T", covariant=True, contravariant=True) +T = ParamSpec(name="T", covariant=True, contravariant=True) + +# Non-errors. + +T = TypeVar("T") +T = TypeVar("T", covariant=False) +T = TypeVar("T", contravariant=False) +T = TypeVar("T", covariant=False, contravariant=False) +T = TypeVar("T", covariant=True) +T = TypeVar("T", covariant=True, contravariant=False) +T = TypeVar(name="T", covariant=True, contravariant=False) +T = TypeVar(name="T", covariant=True) +T = TypeVar("T", contravariant=True) +T = TypeVar("T", covariant=False, contravariant=True) +T = TypeVar(name="T", covariant=False, contravariant=True) +T = TypeVar(name="T", contravariant=True) + +T = ParamSpec("T") +T = ParamSpec("T", covariant=False) +T = ParamSpec("T", contravariant=False) +T = ParamSpec("T", covariant=False, contravariant=False) +T = ParamSpec("T", covariant=True) +T = ParamSpec("T", covariant=True, contravariant=False) +T = ParamSpec(name="T", covariant=True, contravariant=False) +T = ParamSpec(name="T", covariant=True) +T = ParamSpec("T", contravariant=True) +T = ParamSpec("T", covariant=False, contravariant=True) +T = ParamSpec(name="T", covariant=False, contravariant=True) +T = ParamSpec(name="T", contravariant=True) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 1ed61170dd8ba..a321aebbc63b1 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -1658,6 +1658,9 @@ where if self.settings.rules.enabled(Rule::TypeParamNameMismatch) { pylint::rules::type_param_name_mismatch(self, value, targets); } + if self.settings.rules.enabled(Rule::TypeBivariance) { + pylint::rules::type_bivariance(self, value); + } if self.is_stub { if self.any_enabled(&[ Rule::UnprefixedTypeParam, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 5932150ab71bc..280007374b7fa 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -168,6 +168,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pyflakes, "901") => (RuleGroup::Unspecified, rules::pyflakes::rules::RaiseNotImplemented), // pylint + (Pylint, "C0131") => (RuleGroup::Unspecified, rules::pylint::rules::TypeBivariance), (Pylint, "C0132") => (RuleGroup::Unspecified, rules::pylint::rules::TypeParamNameMismatch), (Pylint, "C0205") => (RuleGroup::Unspecified, rules::pylint::rules::SingleStringSlots), (Pylint, "C0414") => (RuleGroup::Unspecified, rules::pylint::rules::UselessImportAlias), diff --git a/crates/ruff/src/rules/pylint/helpers.rs b/crates/ruff/src/rules/pylint/helpers.rs index 51d9360558ad5..02bbd982073f2 100644 --- a/crates/ruff/src/rules/pylint/helpers.rs +++ b/crates/ruff/src/rules/pylint/helpers.rs @@ -1,13 +1,37 @@ use std::fmt; use rustpython_parser::ast; -use rustpython_parser::ast::CmpOp; +use rustpython_parser::ast::{CmpOp, Constant, Expr, Keyword}; use ruff_python_semantic::analyze::function_type; use ruff_python_semantic::{ScopeKind, SemanticModel}; use crate::settings::Settings; +/// Returns the value of the `name` parameter to, e.g., a `TypeVar` constructor. +pub(super) fn type_param_name<'a>(args: &'a [Expr], keywords: &'a [Keyword]) -> Option<&'a str> { + // Handle both `TypeVar("T")` and `TypeVar(name="T")`. + let name_param = keywords + .iter() + .find(|keyword| { + keyword + .arg + .as_ref() + .map_or(false, |keyword| keyword.as_str() == "name") + }) + .map(|keyword| &keyword.value) + .or_else(|| args.get(0))?; + if let Expr::Constant(ast::ExprConstant { + value: Constant::Str(name), + .. + }) = &name_param + { + Some(name) + } else { + None + } +} + pub(super) fn in_dunder_init(semantic: &SemanticModel, settings: &Settings) -> bool { let scope = semantic.scope(); let (ScopeKind::Function(ast::StmtFunctionDef { diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index c1bb1ae591082..9129aeb506fd9 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -85,6 +85,7 @@ mod tests { Path::new("too_many_return_statements.py") )] #[test_case(Rule::TooManyStatements, Path::new("too_many_statements.py"))] + #[test_case(Rule::TypeBivariance, Path::new("type_bivariance.py"))] #[test_case(Rule::TypeParamNameMismatch, Path::new("type_param_name_mismatch.py"))] #[test_case( Rule::UnexpectedSpecialMethodSignature, diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 431c3fd170c02..ad2b6d0d6cfa7 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -37,6 +37,7 @@ pub(crate) use too_many_arguments::*; pub(crate) use too_many_branches::*; pub(crate) use too_many_return_statements::*; pub(crate) use too_many_statements::*; +pub(crate) use type_bivariance::*; pub(crate) use type_param_name_mismatch::*; pub(crate) use unexpected_special_method_signature::*; pub(crate) use unnecessary_direct_lambda_call::*; @@ -85,6 +86,7 @@ mod too_many_arguments; mod too_many_branches; mod too_many_return_statements; mod too_many_statements; +mod type_bivariance; mod type_param_name_mismatch; mod unexpected_special_method_signature; mod unnecessary_direct_lambda_call; diff --git a/crates/ruff/src/rules/pylint/rules/type_bivariance.rs b/crates/ruff/src/rules/pylint/rules/type_bivariance.rs new file mode 100644 index 0000000000000..724584a96259e --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/type_bivariance.rs @@ -0,0 +1,153 @@ +use std::fmt; + +use rustpython_parser::ast::{self, Expr, Ranged}; + +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::is_const_true; + +use crate::checkers::ast::Checker; +use crate::rules::pylint::helpers::type_param_name; + +/// ## What it does +/// Checks for `TypeVar` and `ParamSpec` definitions in which the type is +/// both covariant and contravariant. +/// +/// ## Why is this bad? +/// By default, Python's generic types are invariant, but can be marked as +/// either covariant or contravariant via the `covariant` and `contravariant` +/// keyword arguments. While the API does allow you to mark a type as both +/// covariant and contravariant, this is not supported by the type system, +/// and should be avoided. +/// +/// Instead, change the variance of the type to be either covariant, +/// contravariant, or invariant. If you want to describe both covariance and +/// contravariance, consider using two separate type parameters. +/// +/// For context: an "invariant" generic type only accepts values that exactly +/// match the type parameter; for example, `list[Dog]` accepts only `list[Dog]`, +/// not `list[Animal]` (superclass) or `list[Bulldog]` (subclass). This is +/// the default behavior for Python's generic types. +/// +/// A "covariant" generic type accepts subclasses of the type parameter; for +/// example, `Sequence[Animal]` accepts `Sequence[Dog]`. A "contravariant" +/// generic type accepts superclasses of the type parameter; for example, +/// `Callable[Dog]` accepts `Callable[Animal]`. +/// +/// ## Example +/// ```python +/// from typing import TypeVar +/// +/// T = TypeVar("T", covariant=True, contravariant=True) +/// ``` +/// +/// Use instead: +/// ```python +/// from typing import TypeVar +/// +/// T_co = TypeVar("T_co", covariant=True) +/// T_contra = TypeVar("T_contra", contravariant=True) +/// ``` +/// +/// ## References +/// - [Python documentation: `typing` — Support for type hints](https://docs.python.org/3/library/typing.html) +/// - [PEP 483 – The Theory of Type Hints: Covariance and Contravariance](https://peps.python.org/pep-0483/#covariance-and-contravariance) +/// - [PEP 484 – Type Hints: Covariance and contravariance](https://peps.python.org/pep-0484/#covariance-and-contravariance) +#[violation] +pub struct TypeBivariance { + kind: VarKind, + param_name: Option, +} + +impl Violation for TypeBivariance { + #[derive_message_formats] + fn message(&self) -> String { + let TypeBivariance { kind, param_name } = self; + match param_name { + None => format!("`{kind}` cannot be both covariant and contravariant"), + Some(param_name) => { + format!("`{kind}` \"{param_name}\" cannot be both covariant and contravariant",) + } + } + } +} + +/// PLC0131 +pub(crate) fn type_bivariance(checker: &mut Checker, value: &Expr) { + let Expr::Call(ast::ExprCall { func,args, keywords, .. }) = value else { + return; + }; + + let Some(covariant) = keywords + .iter() + .find(|keyword| { + keyword + .arg + .as_ref() + .map_or(false, |keyword| keyword.as_str() == "covariant") + }) + .map(|keyword| &keyword.value) + else { + return; + }; + + let Some(contravariant) = keywords + .iter() + .find(|keyword| { + keyword + .arg + .as_ref() + .map_or(false, |keyword| keyword.as_str() == "contravariant") + }) + .map(|keyword| &keyword.value) + else { + return; + }; + + if is_const_true(covariant) && is_const_true(contravariant) { + let Some(kind) = checker + .semantic() + .resolve_call_path(func) + .and_then(|call_path| { + if checker + .semantic() + .match_typing_call_path(&call_path, "ParamSpec") + { + Some(VarKind::ParamSpec) + } else if checker + .semantic() + .match_typing_call_path(&call_path, "TypeVar") + { + Some(VarKind::TypeVar) + } else { + None + } + }) + else { + return; + }; + + checker.diagnostics.push(Diagnostic::new( + TypeBivariance { + kind, + param_name: type_param_name(args, keywords).map(ToString::to_string), + }, + func.range(), + )); + } +} + +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +enum VarKind { + TypeVar, + ParamSpec, +} + +impl fmt::Display for VarKind { + fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result { + match self { + VarKind::TypeVar => fmt.write_str("TypeVar"), + VarKind::ParamSpec => fmt.write_str("ParamSpec"), + } + } +} diff --git a/crates/ruff/src/rules/pylint/rules/type_param_name_mismatch.rs b/crates/ruff/src/rules/pylint/rules/type_param_name_mismatch.rs index c7020475ffd59..88e058312b693 100644 --- a/crates/ruff/src/rules/pylint/rules/type_param_name_mismatch.rs +++ b/crates/ruff/src/rules/pylint/rules/type_param_name_mismatch.rs @@ -1,11 +1,12 @@ use std::fmt; -use rustpython_parser::ast::{self, Constant, Expr, Ranged}; +use rustpython_parser::ast::{self, Expr, Ranged}; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use crate::checkers::ast::Checker; +use crate::rules::pylint::helpers::type_param_name; /// ## What it does /// Checks for `TypeVar`, `TypeVarTuple`, `ParamSpec`, and `NewType` @@ -66,17 +67,17 @@ pub(crate) fn type_param_name_mismatch(checker: &mut Checker, value: &Expr, targ return; }; - let Some(param_name) = param_name(value) else { + let Expr::Call(ast::ExprCall { func, args, keywords, .. }) = value else { return; }; - if var_name == param_name { + let Some(param_name) = type_param_name(args, keywords) else { return; - } + }; - let Expr::Call(ast::ExprCall { func, .. }) = value else { + if var_name == param_name { return; - }; + } let Some(kind) = checker .semantic() @@ -138,29 +139,3 @@ impl fmt::Display for VarKind { } } } - -/// Returns the value of the `name` parameter to, e.g., a `TypeVar` constructor. -fn param_name(value: &Expr) -> Option<&str> { - // Handle both `TypeVar("T")` and `TypeVar(name="T")`. - let call = value.as_call_expr()?; - let name_param = call - .keywords - .iter() - .find(|keyword| { - keyword - .arg - .as_ref() - .map_or(false, |keyword| keyword.as_str() == "name") - }) - .map(|keyword| &keyword.value) - .or_else(|| call.args.get(0))?; - if let Expr::Constant(ast::ExprConstant { - value: Constant::Str(name), - .. - }) = &name_param - { - Some(name) - } else { - None - } -} diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC0131_type_bivariance.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC0131_type_bivariance.py.snap new file mode 100644 index 0000000000000..01107aac91aa1 --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLC0131_type_bivariance.py.snap @@ -0,0 +1,40 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +--- +type_bivariance.py:5:5: PLC0131 `TypeVar` "T" cannot be both covariant and contravariant + | +3 | # Errors. +4 | +5 | T = TypeVar("T", covariant=True, contravariant=True) + | ^^^^^^^ PLC0131 +6 | T = TypeVar(name="T", covariant=True, contravariant=True) + | + +type_bivariance.py:6:5: PLC0131 `TypeVar` "T" cannot be both covariant and contravariant + | +5 | T = TypeVar("T", covariant=True, contravariant=True) +6 | T = TypeVar(name="T", covariant=True, contravariant=True) + | ^^^^^^^ PLC0131 +7 | +8 | T = ParamSpec("T", covariant=True, contravariant=True) + | + +type_bivariance.py:8:5: PLC0131 `ParamSpec` "T" cannot be both covariant and contravariant + | +6 | T = TypeVar(name="T", covariant=True, contravariant=True) +7 | +8 | T = ParamSpec("T", covariant=True, contravariant=True) + | ^^^^^^^^^ PLC0131 +9 | T = ParamSpec(name="T", covariant=True, contravariant=True) + | + +type_bivariance.py:9:5: PLC0131 `ParamSpec` "T" cannot be both covariant and contravariant + | + 8 | T = ParamSpec("T", covariant=True, contravariant=True) + 9 | T = ParamSpec(name="T", covariant=True, contravariant=True) + | ^^^^^^^^^ PLC0131 +10 | +11 | # Non-errors. + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 3b2252d9875de..13181e83d74d0 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2123,6 +2123,7 @@ "PLC0", "PLC01", "PLC013", + "PLC0131", "PLC0132", "PLC02", "PLC020",