[flake8-pyi] Fix more complex cases (PYI019)#15821
[flake8-pyi] Fix more complex cases (PYI019)#15821AlexWaygood merged 3 commits intoastral-sh:mainfrom
flake8-pyi] Fix more complex cases (PYI019)#15821Conversation
|
Could you possibly separate this out into an isolated PR? That would make it easier to see which tests are specifically for this bugfix |
|
@AlexWaygood I submitted #15854. As it happens, this PR overlaps with #15853 as well. I'll rebase this once you give the heads-up. |
Okay, I'm done -- rebase at your leisure! Thanks :-) |
|
@AlexWaygood This PR now introduces a regression: def shadowed_type():
type = 1
class A:
@classmethod
def m[S](cls: type[S]) -> S: ... # errorThis is due to I'm not sure how to fix this. |
crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
Show resolved
Hide resolved
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 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! |
crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
Outdated
Show resolved
Hide resolved
AlexWaygood
left a comment
There was a problem hiding this comment.
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?
crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
Show resolved
Hide resolved
crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
Show resolved
Hide resolved
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 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. |
crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_return_type.rs
Outdated
Show resolved
Hide resolved
|
Sure, but users are fickle, and may only have some of the flake8-pyi rules enabled ;) |
Summary
Resolves #15798.
PYI019is 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 runandcargo insta test.