Skip to content

Commit

Permalink
[pyupgrade] Rename private type parameters in PEP 695 generics (`UP…
Browse files Browse the repository at this point in the history
…049`) (#15862)

## Summary

This is a new rule to implement the renaming of PEP 695 type parameters
with leading underscores after they have (presumably) been converted
from standalone type variables by either UP046 or UP047. Part of #15642.

I'm not 100% sure the fix is always safe, but I haven't come up with any
counterexamples yet. `Renamer` seems pretty precise, so I don't think
the usual issues with comments apply.

I initially tried writing this as a rule that receives a `Stmt` rather
than a `Binding`, but in that case the
`checker.semantic().current_scope()` was the global scope, rather than
the scope of the type parameters as I needed. Most of the other rules
using `Renamer` also used `Binding`s, but it does have the downside of
offering separate diagnostics for each parameter to rename.

## Test Plan

New snapshot tests for UP049 alone and the combination of UP046, UP049,
and PYI018.

---------

Co-authored-by: Alex Waygood <[email protected]>
  • Loading branch information
ntBre and AlexWaygood authored Feb 4, 2025
1 parent cb71393 commit 6bb3235
Show file tree
Hide file tree
Showing 13 changed files with 736 additions and 68 deletions.
51 changes: 51 additions & 0 deletions crates/ruff/tests/lint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2174,3 +2174,54 @@ fn flake8_import_convention_unused_aliased_import() {
.arg("-")
.pass_stdin("1"));
}

/// Test that private, old-style `TypeVar` generics
/// 1. Get replaced with PEP 695 type parameters (UP046, UP047)
/// 2. Get renamed to remove leading underscores (UP049)
/// 3. Emit a warning that the standalone type variable is now unused (PYI018)
/// 4. Remove the now-unused `Generic` import
#[test]
fn pep695_generic_rename() {
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(["--select", "F401,PYI018,UP046,UP047,UP049"])
.args(["--stdin-filename", "test.py"])
.arg("--unsafe-fixes")
.arg("--fix")
.arg("--preview")
.arg("--target-version=py312")
.arg("-")
.pass_stdin(
r#"
from typing import Generic, TypeVar
_T = TypeVar("_T")
class OldStyle(Generic[_T]):
var: _T
def func(t: _T) -> _T:
x: _T
return x
"#
),
@r#"
success: false
exit_code: 1
----- stdout -----
from typing import TypeVar
_T = TypeVar("_T")
class OldStyle[T]:
var: T
def func[T](t: T) -> T:
x: T
return x
----- stderr -----
test.py:3:1: PYI018 Private TypeVar `_T` is never used
Found 6 errors (5 fixed, 1 remaining).
"#
);
}
30 changes: 30 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pyupgrade/UP049_0.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# simple case, replace _T in signature and body
class Generic[_T]:
buf: list[_T]

def append(self, t: _T):
self.buf.append(t)


# simple case, replace _T in signature and body
def second[_T](var: tuple[_T]) -> _T:
y: _T = var[1]
return y


# one diagnostic for each variable, comments are preserved
def many_generics[
_T, # first generic
_U, # second generic
](args):
return args


# neither of these are currently renamed
from typing import Literal, cast


def f[_T](v):
cast("_T", v)
cast("Literal['_T']")
cast("list[_T]", v)
56 changes: 56 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pyupgrade/UP049_1.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# bound
class Foo[_T: str]:
var: _T


# constraint
class Foo[_T: (str, bytes)]:
var: _T


# python 3.13+ default
class Foo[_T = int]:
var: _T


# tuple
class Foo[*_Ts]:
var: tuple[*_Ts]


# paramspec
class C[**_P]:
var: _P


from typing import Callable


# each of these will get a separate diagnostic, but at least they'll all get
# fixed
class Everything[_T, _U: str, _V: (int, float), *_W, **_X]:
@staticmethod
def transform(t: _T, u: _U, v: _V) -> tuple[*_W] | Callable[_X, _T] | None:
return None


# this should not be fixed because the new name is a keyword, but we still
# offer a diagnostic
class F[_async]: ...


# and this should not be fixed because of the conflict with the outer X, but it
# also gets a diagnostic
def f():
type X = int

class ScopeConflict[_X]:
var: _X
x: X


# these cases should be skipped entirely
def f[_](x: _) -> _: ...
def g[__](x: __) -> __: ...
def h[_T_](x: _T_) -> _T_: ...
def i[__T__](x: __T__) -> __T__: ...
8 changes: 7 additions & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/bindings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::checkers::ast::Checker;
use crate::codes::Rule;
use crate::rules::{
flake8_import_conventions, flake8_pyi, flake8_pytest_style, flake8_type_checking, pyflakes,
pylint, refurb, ruff,
pylint, pyupgrade, refurb, ruff,
};

/// Run lint rules over the [`Binding`]s.
Expand All @@ -24,6 +24,7 @@ pub(crate) fn bindings(checker: &mut Checker) {
Rule::PytestUnittestRaisesAssertion,
Rule::ForLoopWrites,
Rule::CustomTypeVarForSelf,
Rule::PrivateTypeParameter,
]) {
return;
}
Expand Down Expand Up @@ -123,5 +124,10 @@ pub(crate) fn bindings(checker: &mut Checker) {
checker.diagnostics.push(diagnostic);
}
}
if checker.enabled(Rule::PrivateTypeParameter) {
if let Some(diagnostic) = pyupgrade::rules::private_type_parameter(checker, binding) {
checker.diagnostics.push(diagnostic);
}
}
}
}
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Pyupgrade, "045") => (RuleGroup::Preview, rules::pyupgrade::rules::NonPEP604AnnotationOptional),
(Pyupgrade, "046") => (RuleGroup::Preview, rules::pyupgrade::rules::NonPEP695GenericClass),
(Pyupgrade, "047") => (RuleGroup::Preview, rules::pyupgrade::rules::NonPEP695GenericFunction),
(Pyupgrade, "049") => (RuleGroup::Preview, rules::pyupgrade::rules::PrivateTypeParameter),

// pydocstyle
(Pydocstyle, "100") => (RuleGroup::Stable, rules::pydocstyle::rules::UndocumentedPublicModule),
Expand Down
52 changes: 52 additions & 0 deletions crates/ruff_linter/src/renamer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@ use ruff_diagnostics::Edit;
use ruff_python_ast as ast;
use ruff_python_codegen::Stylist;
use ruff_python_semantic::{Binding, BindingKind, Scope, ScopeId, SemanticModel};
use ruff_python_stdlib::{builtins::is_python_builtin, keyword::is_keyword};
use ruff_text_size::Ranged;

use crate::checkers::ast::Checker;

pub(crate) struct Renamer;

impl Renamer {
Expand Down Expand Up @@ -369,3 +372,52 @@ impl Renamer {
}
}
}

/// Enumeration of various ways in which a binding can shadow other variables
#[derive(Debug, PartialEq, Eq, Copy, Clone)]
pub(crate) enum ShadowedKind {
/// The variable shadows a global, nonlocal or local symbol
Some,
/// The variable shadows a builtin symbol
BuiltIn,
/// The variable shadows a keyword
Keyword,
/// The variable does not shadow any other symbols
None,
}

impl ShadowedKind {
/// Determines the kind of shadowing or conflict for a given variable name.
///
/// This function is useful for checking whether or not the `target` of a [`Rename::rename`]
/// will shadow another binding.
pub(crate) fn new(name: &str, checker: &Checker, scope_id: ScopeId) -> ShadowedKind {
// Check the kind in order of precedence
if is_keyword(name) {
return ShadowedKind::Keyword;
}

if is_python_builtin(
name,
checker.settings.target_version.minor(),
checker.source_type.is_ipynb(),
) {
return ShadowedKind::BuiltIn;
}

if !checker.semantic().is_available_in_scope(name, scope_id) {
return ShadowedKind::Some;
}

// Default to no shadowing
ShadowedKind::None
}

/// Returns `true` if `self` shadows any global, nonlocal, or local symbol, keyword, or builtin.
pub(crate) const fn shadows_any(self) -> bool {
matches!(
self,
ShadowedKind::Some | ShadowedKind::BuiltIn | ShadowedKind::Keyword
)
}
}
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pyupgrade/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ mod tests {
#[test_case(Rule::NonPEP695GenericClass, Path::new("UP046_0.py"))]
#[test_case(Rule::NonPEP695GenericClass, Path::new("UP046_1.py"))]
#[test_case(Rule::NonPEP695GenericFunction, Path::new("UP047.py"))]
#[test_case(Rule::PrivateTypeParameter, Path::new("UP049_0.py"))]
#[test_case(Rule::PrivateTypeParameter, Path::new("UP049_1.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = path.to_string_lossy().to_string();
let diagnostics = test_path(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/pyupgrade/rules/pep695/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@ use ruff_text_size::{Ranged, TextRange};
pub(crate) use non_pep695_generic_class::*;
pub(crate) use non_pep695_generic_function::*;
pub(crate) use non_pep695_type_alias::*;
pub(crate) use private_type_parameter::*;

use crate::checkers::ast::Checker;

mod non_pep695_generic_class;
mod non_pep695_generic_function;
mod non_pep695_type_alias;
mod private_type_parameter;

#[derive(Debug)]
enum TypeVarRestriction<'a> {
Expand Down
Loading

0 comments on commit 6bb3235

Please sign in to comment.