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

## Summary

This PR stabilizes several preview-only behaviours for
`custom-typevar-for-self` (`PYI019`). Namely:
- A new, more accurate technique is now employed for detecting custom
TypeVars that are replaceable with `Self`. See
#15888 for details.
- The range of the diagnostic is now the full function header rather
than just the return annotation. (Previously, the rule only applied to
methods with return annotations, but this is no longer true due to the
changes in the first bullet point.)
- The fix is now available even when preview mode is not enabled.

## Test Plan

- Existing snapshots that do not have preview mode enabled are updated
- Preview-specific snapshots are removed
- I'll check the ecosystem report on this PR to verify everything's as
expected

Author: AlexWaygood

crates/ruff_linter/src/rules/flake8_pyi/mod.rs

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

crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_for_self.rs

@@ -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/
 /// [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
@@ -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,
@@ -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<'_> {
@@ -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>(
@@ -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>,
@@ -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())?;
 
@@ -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);
@@ -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`.