[`pylint`] Implement Pylint `typevar-double-variance` (`C0131`) (#5517)

## Summary

Implement Pylint `typevar-double-variance` (`C0131`) as
`type-bivariance` (`PLC0131`). Includes documentation. Related to #970.
Renamed the rule to be more clear (it's not immediately obvious what
'double' means, IMO).

The Pylint implementation checks only `TypeVar`, but this PR checks
`ParamSpec` as well.

## Test Plan

Added tests.

`cargo test`
This commit is contained in:
Tom Kuson 2023-07-05 19:53:41 +01:00 committed by GitHub
parent 9a8e5f7877
commit 9478454b96
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 270 additions and 33 deletions

View File

@ -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)

View File

@ -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,

View File

@ -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),

View File

@ -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 {

View File

@ -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,

View File

@ -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;

View File

@ -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<String>,
}
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"),
}
}
}

View File

@ -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,7 +67,11 @@ 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;
};
let Some(param_name) = type_param_name(args, keywords) else {
return;
};
@ -74,10 +79,6 @@ pub(crate) fn type_param_name_mismatch(checker: &mut Checker, value: &Expr, targ
return;
}
let Expr::Call(ast::ExprCall { func, .. }) = value else {
return;
};
let Some(kind) = checker
.semantic()
.resolve_call_path(func)
@ -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
}
}

View File

@ -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.
|

1
ruff.schema.json generated
View File

@ -2123,6 +2123,7 @@
"PLC0",
"PLC01",
"PLC013",
"PLC0131",
"PLC0132",
"PLC02",
"PLC020",