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

[flake8-pyi] Fix more complex cases (PYI019) #15821

Merged
merged 3 commits into from
Feb 2, 2025

Conversation

InSyncWithFoo
Copy link
Contributor

@InSyncWithFoo InSyncWithFoo commented Jan 30, 2025

Summary

Resolves #15798.

PYI019 is now a binding-based rule. All references to the type variable will now be replaced correctly. As a result, the fix is now safe in most cases; when that is not possible, its applicability remains display-only. Additionally, for a safe fix, comments within the fix ranges will cause it to be marked as unsafe.

Test Plan

cargo nextest run and cargo insta test.

Copy link
Contributor

github-actions bot commented Jan 30, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@dylwil3 dylwil3 added the fixes Related to suggested fixes for violations label Jan 30, 2025
@AlexWaygood AlexWaygood self-assigned this Jan 31, 2025
@AlexWaygood
Copy link
Member

AlexWaygood commented Jan 31, 2025

I'd like for us to fix existing bugs such as #15849 first before extending this rule further (edit: see #15851, and #15853)

@AlexWaygood
Copy link
Member

Previously, there was also a small bug that would cause a PEP 695 type variable not to be removed correctly were it to be placed at the very end of the type parameter list. This has been fixed as well.

Could you possibly separate this out into an isolated PR? That would make it easier to see which tests are specifically for this bugfix

@InSyncWithFoo
Copy link
Contributor Author

@AlexWaygood I submitted #15854. As it happens, this PR overlaps with #15853 as well. I'll rebase this once you give the heads-up.

@AlexWaygood
Copy link
Member

I'll rebase this once you give the heads-up.

Okay, I'm done -- rebase at your leisure! Thanks :-)

@InSyncWithFoo
Copy link
Contributor Author

@AlexWaygood This PR now introduces a regression:

def shadowed_type():
    type = 1
    class A:
        @classmethod
        def m[S](cls: type[S]) -> S: ...  # error

This is due to semantic.match_builtin_expr() searching for type in the "current" scope (via self.scope_id in .lookup_symbol()) rather than the scope of the method.

I'm not sure how to fix this. .simulate_runtime_load_at_location_in_scope() returns a binding, but a ::Builtin binding does not have any information as to what symbol it represents. Not to mention, that method expects a string, so builtins.type can't be detected correctly. On the other hand, .resolve_qualified_name() does not accept a scope.

@AlexWaygood
Copy link
Member

AlexWaygood commented Feb 1, 2025

@AlexWaygood This PR now introduces a regression:

I think you can do something like this:

diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
index 4d30fd3eb..7128fddb1 100644
--- a/crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
+++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
@@ -8,7 +8,7 @@ use ruff_python_ast::{
 };
 use ruff_python_semantic::analyze::function_type::{self, FunctionType};
 use ruff_python_semantic::analyze::visibility::{is_abstract, is_overload};
-use ruff_python_semantic::{Binding, BindingKind, SemanticModel};
+use ruff_python_semantic::{Binding, BindingKind, ScopeId, SemanticModel};
 use ruff_text_size::{Ranged, TextRange, TextSize};
 
 use crate::checkers::ast::Checker;
@@ -138,7 +138,7 @@ pub(crate) fn custom_type_var_return_type(
         }),
     };
 
-    if !method.uses_custom_var(semantic) {
+    if !method.uses_custom_var(semantic, binding.scope) {
         return None;
     }
 
@@ -162,9 +162,9 @@ enum Method<'a> {
 }
 
 impl Method<'_> {
-    fn uses_custom_var(&self, semantic: &SemanticModel) -> bool {
+    fn uses_custom_var(&self, semantic: &SemanticModel, scope: ScopeId) -> bool {
         match self {
-            Self::Class(class_method) => class_method.uses_custom_var(semantic),
+            Self::Class(class_method) => class_method.uses_custom_var(semantic, scope),
             Self::Instance(instance_method) => instance_method.uses_custom_var(),
         }
     }
@@ -180,7 +180,7 @@ struct ClassMethod<'a> {
 impl ClassMethod<'_> {
     /// Returns `true` if the class method is annotated with
     /// a custom `TypeVar` that is likely private.
-    fn uses_custom_var(&self, semantic: &SemanticModel) -> bool {
+    fn uses_custom_var(&self, semantic: &SemanticModel, scope: ScopeId) -> bool {
         let Expr::Subscript(ast::ExprSubscript {
             value: cls_annotation_value,
             slice: cls_annotation_typevar,
@@ -196,7 +196,15 @@ impl ClassMethod<'_> {
 
         let cls_annotation_typevar = &cls_annotation_typevar.id;
 
-        if !semantic.match_builtin_expr(cls_annotation_value, "type") {
+        let Expr::Name(ExprName { id, ..}) = &**cls_annotation_value else {
+            return false;
+        };
+
+        if id != "type" {
+            return false;
+        }
+
+        if !semantic.has_builtin_binding_in_scope("type", scope) {
             return false;
         }
 
@@ -206,7 +214,10 @@ impl ClassMethod<'_> {
                 let Expr::Name(return_annotation_typevar) = &**slice else {
                     return false;
                 };
-                if !semantic.match_builtin_expr(value, "type") {
+                let Expr::Name(ExprName { id, ..}) = &**value else {
+                    return false;
+                };
+                if id != "type" {
                     return false;
                 }
                 &return_annotation_typevar.id
diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs
index 4ef240e23..825ee3256 100644
--- a/crates/ruff_python_semantic/src/model.rs
+++ b/crates/ruff_python_semantic/src/model.rs
@@ -265,13 +265,22 @@ impl<'a> SemanticModel<'a> {
         self.shadowed_bindings.get(&binding_id).copied()
     }
 
-    /// Return `true` if `member` is bound as a builtin.
+    /// Return `true` if `member` is bound as a builtin *in the scope we are currently visiting*.
     ///
     /// Note that a "builtin binding" does *not* include explicit lookups via the `builtins`
     /// module, e.g. `import builtins; builtins.open`. It *only* includes the bindings
     /// that are pre-populated in Python's global scope before any imports have taken place.
     pub fn has_builtin_binding(&self, member: &str) -> bool {
-        self.lookup_symbol(member)
+        self.has_builtin_binding_in_scope(member, self.scope_id)
+    }
+
+    /// Return `true` if `member` is bound as a builtin *in a given scope*.
+    ///
+    /// Note that a "builtin binding" does *not* include explicit lookups via the `builtins`
+    /// module, e.g. `import builtins; builtins.open`. It *only* includes the bindings
+    /// that are pre-populated in Python's global scope before any imports have taken place.
+    pub fn has_builtin_binding_in_scope(&self, member: &str, scope_id: ScopeId) -> bool {
+        self.lookup_symbol_in_scope(member, scope_id, false)
             .map(|binding_id| &self.bindings[binding_id])
             .is_some_and(|binding| binding.kind.is_builtin())
     }

It will still mean that the PR introduces a small regression. We will no longer emit the diagnostic on code like this, where the fully qualified builtins.type is used in the annotation of the first parameter of a classmethod:

import builtins

class Foo:
    @classmethod
    def m[S](cls: builtins.type[S]) -> builtins.type[S]: ...

But false negatives are much better than false positives. And I think the small regression of some edge-case false negatives is hugely outweighed by the improvements you're introducing in this PR!

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Do the changes in this PR mean that we could also offer the fix for .py files as well as stub files? We could potentially iterate over the references to the TypeVar in the body of the function and replace them with Self, right?

@InSyncWithFoo
Copy link
Contributor Author

Do the changes in this PR mean that we could also offer the fix for .py files as well as stub files?

Very unfortunately, no. This would have problems similar to that of #15862.

@AlexWaygood
Copy link
Member

AlexWaygood commented Feb 2, 2025

Very unfortunately, no. This would have problems similar to that of #15862.

You're worried about references inside stringized type expressions? That doesn't seem like a great reason to restrict the rule to stub files, since stringized type expressions are legal in stub files as well as .py files (they're just not encouraged). It would be better to detect if any of the references are inside stringized type expressions, and make the fix display-only if so, but to run the fix on both stub files and .py files.

But anyway, let's land this as-is for now. It's a big improvement as it is, and the PR is big enough as it is. I have some followup work locally that I can file as PRs to build on this.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thank you!

@AlexWaygood AlexWaygood added the preview Related to preview mode features label Feb 2, 2025
@AlexWaygood AlexWaygood enabled auto-merge (squash) February 2, 2025 18:34
@AlexWaygood AlexWaygood merged commit 3c09100 into astral-sh:main Feb 2, 2025
21 checks passed
@InSyncWithFoo InSyncWithFoo deleted the PYI019 branch February 2, 2025 19:04
@InSyncWithFoo
Copy link
Contributor Author

[...] stringized type expressions are legal in stub files as well as .py files [...]

True, but we also have PYI010 for non-empty bodies and PYI020 for stringified types, both of which are only reported for stubs and always safely fixable.

@AlexWaygood
Copy link
Member

Sure, but users are fickle, and may only have some of the flake8-pyi rules enabled ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixes Related to suggested fixes for violations preview Related to preview mode features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Request: More unsafe fixes for custom-type-var-return-type/PYI019
3 participants