Skip to content

[ruff-0.10] [flake8-pyi] Stabilize preview-mode behaviours for custom-type-var-for-self(PYI019) #16607

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 11, 2025
Merged

[ruff-0.10] [flake8-pyi] Stabilize preview-mode behaviours for custom-type-var-for-self(PYI019) #16607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 0 additions & 24 deletions crates/ruff_linter/src/rules/flake8_pyi/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,30 +156,6 @@ mod tests {
Ok(())
}

#[test_case(Rule::CustomTypeVarForSelf, Path::new("PYI019_0.py"))]
#[test_case(Rule::CustomTypeVarForSelf, Path::new("PYI019_0.pyi"))]
#[test_case(Rule::CustomTypeVarForSelf, Path::new("PYI019_1.pyi"))]
fn custom_classmethod_rules_preview(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview_{}_{}",
rule_code.noqa_code(),
path.to_string_lossy()
);
let diagnostics = test_path(
Path::new("flake8_pyi").join(path).as_path(),
&settings::LinterSettings {
pep8_naming: pep8_naming::settings::Settings {
classmethod_decorators: vec!["foo_classmethod".to_string()],
..pep8_naming::settings::Settings::default()
},
preview: PreviewMode::Enabled,
..settings::LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(Rule::TypeAliasWithoutAnnotation, Path::new("PYI026.py"))]
#[test_case(Rule::TypeAliasWithoutAnnotation, Path::new("PYI026.pyi"))]
#[test_case(Rule::RedundantNoneLiteral, Path::new("PYI061.py"))]
Expand Down
238 changes: 26 additions & 212 deletions crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_for_self.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,20 +71,8 @@ use ruff_python_ast::PythonVersion;
/// The fix is only marked as unsafe if there is the possibility that it might delete a comment
/// from your code.
///
/// ## Preview-mode behaviour
/// This rule's behaviour has several differences when [`preview`] mode is enabled:
/// 1. The fix for this rule is currently only available if `preview` mode is enabled.
/// 2. By default, this rule is only applied to methods that have return-type annotations,
/// and the range of the diagnostic is the range of the return-type annotation.
/// In preview mode, this rule is also applied to some methods that do not have
/// return-type annotations. The range of the diagnostic is the range of the function
/// header (from the end of the function name to the end of the parameters).
/// 3. In `preview` mode, the rule uses different logic to determine whether an annotation
/// refers to a type variable. The `preview`-mode logic is more accurate, but may lead
/// to more methods being flagged than if `preview` mode is disabled.
///
/// [PEP 673]: https://peps.python.org/pep-0673/#motivation
/// [PEP 695]: https://peps.python.org/pep-0695/
/// [PEP-695]: https://peps.python.org/pep-0695/
Copy link
Member

@MichaReiser MichaReiser Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update both PEP names. It feels somewhat inconsistent to use a - here but not for PEP 673 (I don't think we have a standard for this but I suspect that no dash is more commonly used?)

Copy link
Contributor

@ntBre ntBre Mar 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this PEP-695 is only used in a hyphenated expression ([PEP-695]-style), which probably explains the extra dash in this one.

Copy link
Member Author

@AlexWaygood AlexWaygood Mar 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, Brent has it exactly right. The unhyphenated form is usually preferred, so that's why "PEP 673" should remain unhyphenated in the docs above this. But PEP-695-style is a hyphenated expression, so it makes sense for the hyphen to remain there between "PEP" and "695"

/// [PYI018]: https://docs.astral.sh/ruff/rules/unused-private-type-var/
/// [type parameter list]: https://docs.python.org/3/reference/compound_stmts.html#type-params
/// [Self]: https://docs.python.org/3/library/typing.html#typing.Self
Expand Down Expand Up @@ -162,73 +150,33 @@ pub(crate) fn custom_type_var_instead_of_self(
&checker.settings.pep8_naming.staticmethod_decorators,
);

let function_header_end = returns
.as_deref()
.map(Ranged::end)
.unwrap_or_else(|| parameters.end());

// In stable mode, we only emit the diagnostic on methods that have a return type annotation.
// In preview mode, we have a more principled approach to determine if an annotation refers
// to a type variable, and we emit the diagnostic on some methods that do not have return
// annotations.
let (method, diagnostic_range) = match function_kind {
FunctionType::ClassMethod | FunctionType::NewMethod => {
if checker.settings.preview.is_enabled() {
(
Method::PreviewClass(PreviewClassMethod {
cls_annotation: self_or_cls_annotation,
type_params,
}),
TextRange::new(function_name.end(), function_header_end),
)
} else {
returns.as_deref().map(|returns| {
(
Method::Class(ClassMethod {
cls_annotation: self_or_cls_annotation,
returns,
type_params,
}),
returns.range(),
)
})?
}
}
FunctionType::Method => {
if checker.settings.preview.is_enabled() {
(
Method::PreviewInstance(PreviewInstanceMethod {
self_annotation: self_or_cls_annotation,
type_params,
}),
TextRange::new(function_name.end(), function_header_end),
)
} else {
returns.as_deref().map(|returns| {
(
Method::Instance(InstanceMethod {
self_annotation: self_or_cls_annotation,
returns,
type_params,
}),
returns.range(),
)
})?
}
}
let method = match function_kind {
FunctionType::ClassMethod | FunctionType::NewMethod => Method::Class(ClassMethod {
cls_annotation: self_or_cls_annotation,
type_params,
}),
FunctionType::Method => Method::Instance(InstanceMethod {
self_annotation: self_or_cls_annotation,
type_params,
}),
FunctionType::Function | FunctionType::StaticMethod => return None,
};

let custom_typevar = method.custom_typevar(semantic, binding.scope)?;

let function_header_end = returns
.as_deref()
.map(Ranged::end)
.unwrap_or_else(|| parameters.end());

let mut diagnostic = Diagnostic::new(
CustomTypeVarForSelf {
typevar_name: custom_typevar.name(checker.source()).to_string(),
},
diagnostic_range,
TextRange::new(function_name.end(), function_header_end),
);

diagnostic.try_set_optional_fix(|| {
diagnostic.try_set_fix(|| {
replace_custom_typevar_with_self(
checker,
function_def,
Expand All @@ -244,9 +192,7 @@ pub(crate) fn custom_type_var_instead_of_self(
#[derive(Debug)]
enum Method<'a> {
Class(ClassMethod<'a>),
PreviewClass(PreviewClassMethod<'a>),
Instance(InstanceMethod<'a>),
PreviewInstance(PreviewInstanceMethod<'a>),
}

impl Method<'_> {
Expand All @@ -257,86 +203,18 @@ impl Method<'_> {
) -> Option<TypeVar<'a>> {
match self {
Self::Class(class_method) => class_method.custom_typevar(semantic, scope),
Self::PreviewClass(class_method) => class_method.custom_typevar(semantic, scope),
Self::Instance(instance_method) => instance_method.custom_typevar(semantic),
Self::PreviewInstance(instance_method) => instance_method.custom_typevar(semantic),
}
}
}

#[derive(Debug)]
struct ClassMethod<'a> {
cls_annotation: &'a ast::Expr,
returns: &'a ast::Expr,
type_params: Option<&'a ast::TypeParams>,
}

impl ClassMethod<'_> {
/// Returns `Some(typevar)` if the class method is annotated with
/// a custom `TypeVar` that is likely private.
fn custom_typevar<'a>(
&'a self,
semantic: &'a SemanticModel<'a>,
scope: ScopeId,
) -> Option<TypeVar<'a>> {
let ast::ExprSubscript {
value: cls_annotation_value,
slice: cls_annotation_typevar,
..
} = self.cls_annotation.as_subscript_expr()?;

let cls_annotation_typevar = cls_annotation_typevar.as_name_expr()?;
let cls_annotation_typevar_name = &cls_annotation_typevar.id;
let ast::ExprName { id, .. } = cls_annotation_value.as_name_expr()?;

if id != "type" {
return None;
}

if !semantic.has_builtin_binding_in_scope("type", scope) {
return None;
}

let return_annotation_typevar = match self.returns {
ast::Expr::Name(ast::ExprName { id, .. }) => id,
ast::Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => {
let return_annotation_typevar = slice.as_name_expr()?;
let ast::ExprName { id, .. } = value.as_name_expr()?;
if id != "type" {
return None;
}
&return_annotation_typevar.id
}
_ => return None,
};

if cls_annotation_typevar_name != return_annotation_typevar {
return None;
}

if !is_likely_private_typevar(cls_annotation_typevar_name, self.type_params) {
return None;
}

semantic
.resolve_name(cls_annotation_typevar)
.map(|binding_id| TypeVar(semantic.binding(binding_id)))
}
}

/// Struct for implementing this rule as applied to classmethods in preview mode.
///
/// In stable mode, we only emit this diagnostic on methods that have return annotations,
/// so the stable-mode version of this struct has a `returns: &ast::Expr` field. In preview
/// mode, we also emit this diagnostic on methods that do not have return annotations, so
/// the preview-mode version of this struct does not have a `returns` field.
#[derive(Debug)]
struct PreviewClassMethod<'a> {
cls_annotation: &'a ast::Expr,
type_params: Option<&'a ast::TypeParams>,
}

impl PreviewClassMethod<'_> {
/// Returns `Some(typevar)` if the class method is annotated with
/// a custom `TypeVar` for the `cls` parameter
fn custom_typevar<'a>(
Expand All @@ -360,90 +238,30 @@ impl PreviewClassMethod<'_> {
return None;
}

custom_typevar_preview(cls_annotation_typevar, self.type_params, semantic)
custom_typevar(cls_annotation_typevar, self.type_params, semantic)
}
}

#[derive(Debug)]
struct InstanceMethod<'a> {
self_annotation: &'a ast::Expr,
returns: &'a ast::Expr,
type_params: Option<&'a ast::TypeParams>,
}

impl InstanceMethod<'_> {
/// Returns `Some(typevar)` if the instance method is annotated with
/// a custom `TypeVar` that is likely private.
fn custom_typevar<'a>(&'a self, semantic: &'a SemanticModel<'a>) -> Option<TypeVar<'a>> {
let self_annotation = self.self_annotation.as_name_expr()?;
let first_arg_type = &self_annotation.id;

let ast::ExprName {
id: return_type, ..
} = self.returns.as_name_expr()?;

if first_arg_type != return_type {
return None;
}

if !is_likely_private_typevar(first_arg_type, self.type_params) {
return None;
}

semantic
.resolve_name(self_annotation)
.map(|binding_id| TypeVar(semantic.binding(binding_id)))
}
}

/// Struct for implementing this rule as applied to instance methods in preview mode.
///
/// In stable mode, we only emit this diagnostic on methods that have return annotations,
/// so the stable-mode version of this struct has a `returns: &ast::Expr` field. In preview
/// mode, we also emit this diagnostic on methods that do not have return annotations, so
/// the preview-mode version of this struct does not have a `returns` field.
#[derive(Debug)]
struct PreviewInstanceMethod<'a> {
self_annotation: &'a ast::Expr,
type_params: Option<&'a ast::TypeParams>,
}

impl PreviewInstanceMethod<'_> {
/// Returns `Some(typevar)` if the instance method is annotated with
/// a custom `TypeVar` for the `self` parameter
fn custom_typevar<'a>(&'a self, semantic: &'a SemanticModel<'a>) -> Option<TypeVar<'a>> {
custom_typevar_preview(
custom_typevar(
self.self_annotation.as_name_expr()?,
self.type_params,
semantic,
)
}
}

/// Returns `true` if the type variable is likely private.
///
/// This routine is only used if `--preview` is not enabled,
/// as it uses heuristics to determine if an annotation uses a type variable.
/// In preview mode, we apply a more principled approach.
fn is_likely_private_typevar(type_var_name: &str, type_params: Option<&ast::TypeParams>) -> bool {
// Ex) `_T`
if type_var_name.starts_with('_') {
return true;
}
// Ex) `class Foo[T]: ...`
type_params.is_some_and(|type_params| {
type_params.iter().any(|type_param| {
if let ast::TypeParam::TypeVar(ast::TypeParamTypeVar { name, .. }) = type_param {
name == type_var_name
} else {
false
}
})
})
}

/// Returns `Some(TypeVar)` if `typevar_expr` refers to a `TypeVar` binding
fn custom_typevar_preview<'a>(
fn custom_typevar<'a>(
typevar_expr: &'a ast::ExprName,
type_params: Option<&ast::TypeParams>,
semantic: &'a SemanticModel<'a>,
Expand Down Expand Up @@ -497,11 +315,7 @@ fn replace_custom_typevar_with_self(
custom_typevar: TypeVar,
self_or_cls_parameter: &ast::ParameterWithDefault,
self_or_cls_annotation: &ast::Expr,
) -> anyhow::Result<Option<Fix>> {
if checker.settings.preview.is_disabled() {
return Ok(None);
}

) -> anyhow::Result<Fix> {
// (1) Import `Self` (if necessary)
let (import_edit, self_symbol_binding) = import_self(checker, function_def.start())?;

Expand All @@ -513,9 +327,9 @@ fn replace_custom_typevar_with_self(

// (3) If it was a PEP-695 type variable, remove that `TypeVar` from the PEP-695 type-parameter list
if custom_typevar.is_pep695_typevar() {
let Some(type_params) = function_def.type_params.as_deref() else {
bail!("Should not be possible to have a type parameter without a type parameter list");
};
let type_params = function_def.type_params.as_deref().context(
"Should not be possible to have a type parameter without a type parameter list",
)?;
let deletion_edit = remove_pep695_typevar_declaration(type_params, custom_typevar)
.context("Failed to find a `TypeVar` in the type params that matches the binding")?;
other_edits.push(deletion_edit);
Expand Down Expand Up @@ -546,11 +360,11 @@ fn replace_custom_typevar_with_self(
Applicability::Safe
};

Ok(Some(Fix::applicable_edits(
Ok(Fix::applicable_edits(
import_edit,
other_edits,
applicability,
)))
))
}

/// Attempt to create an [`Edit`] that imports `Self`.
Expand Down
Loading
Loading