Skip to content
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

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

Merged
merged 6 commits into from
Jul 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.

Loading