Skip to content

Commit

Permalink
[pylint] Implement Pylint typevar-double-variance (C0131) (#5517)
Browse files Browse the repository at this point in the history
## 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`
  • Loading branch information
tjkuson authored Jul 5, 2023
1 parent 9a8e5f7 commit 9478454
Show file tree
Hide file tree
Showing 10 changed files with 270 additions and 33 deletions.
37 changes: 37 additions & 0 deletions crates/ruff/resources/test/fixtures/pylint/type_bivariance.py
Original file line number Diff line number Diff line change
@@ -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)
3 changes: 3 additions & 0 deletions crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
26 changes: 25 additions & 1 deletion crates/ruff/src/rules/pylint/helpers.rs
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down
1 change: 1 addition & 0 deletions crates/ruff/src/rules/pylint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/src/rules/pylint/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::*;
Expand Down Expand Up @@ -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;
Expand Down
153 changes: 153 additions & 0 deletions crates/ruff/src/rules/pylint/rules/type_bivariance.rs
Original file line number Diff line number Diff line change
@@ -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"),
}
}
}
39 changes: 7 additions & 32 deletions crates/ruff/src/rules/pylint/rules/type_param_name_mismatch.rs
Original file line number Diff line number Diff line change
@@ -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`
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
}
}
Original file line number Diff line number Diff line change
@@ -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 change: 1 addition & 0 deletions ruff.schema.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 9478454

Please sign in to comment.