From 6086cb7d0edca8c1e36df780498fb1d58aa7c8d5 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 22 Mar 2024 13:42:15 +0000 Subject: [PATCH 1/7] Track ranges of names inside `__all__` definitions --- .../src/checkers/ast/analyze/definitions.rs | 4 +-- crates/ruff_linter/src/checkers/ast/mod.rs | 15 ++++---- ..._rules__pyflakes__tests__F405_F405.py.snap | 6 ++-- ...ules__pyflakes__tests__F822_F822_0.py.snap | 6 ++-- ...les__pyflakes__tests__F822_F822_0.pyi.snap | 4 +-- ...ules__pyflakes__tests__F822_F822_1.py.snap | 6 ++-- crates/ruff_python_ast/src/all.rs | 35 ++++++++++++++++--- crates/ruff_python_semantic/src/binding.rs | 3 +- crates/ruff_python_semantic/src/definition.rs | 12 ++++--- 9 files changed, 56 insertions(+), 35 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs b/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs index 8b5ba3700d511..aaac685e2980f 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs @@ -1,4 +1,4 @@ -use ruff_python_ast::str::raw_contents_range; +use ruff_python_ast::{all::DunderAllName, str::raw_contents_range}; use ruff_text_size::{Ranged, TextRange}; use ruff_python_semantic::{ @@ -93,7 +93,7 @@ pub(crate) fn definitions(checker: &mut Checker) { } // Compute visibility of all definitions. - let exports: Option> = { + let exports: Option> = { checker .semantic .global_scope() diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index d6fb29191662b..ae0d18da0d79f 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -31,8 +31,9 @@ use std::path::Path; use itertools::Itertools; use log::debug; use ruff_python_ast::{ - self as ast, Comprehension, ElifElseClause, ExceptHandler, Expr, ExprContext, Keyword, - MatchCase, Parameter, ParameterWithDefault, Parameters, Pattern, Stmt, Suite, UnaryOp, + self as ast, all::DunderAllName, Comprehension, ElifElseClause, ExceptHandler, Expr, + ExprContext, Keyword, MatchCase, Parameter, ParameterWithDefault, Parameters, Pattern, Stmt, + Suite, UnaryOp, }; use ruff_text_size::{Ranged, TextRange, TextSize}; @@ -2097,25 +2098,21 @@ impl<'a> Checker<'a> { fn visit_exports(&mut self) { let snapshot = self.semantic.snapshot(); - let exports: Vec<(&str, TextRange)> = self + let exports: Vec = self .semantic .global_scope() .get_all("__all__") .map(|binding_id| &self.semantic.bindings[binding_id]) .filter_map(|binding| match &binding.kind { - BindingKind::Export(Export { names }) => { - Some(names.iter().map(|name| (*name, binding.range()))) - } + BindingKind::Export(Export { names }) => Some(names.iter().copied()), _ => None, }) .flatten() .collect(); - for (name, range) in exports { + for DunderAllName { name, range } in exports { if let Some(binding_id) = self.semantic.global_scope().get(name) { // Mark anything referenced in `__all__` as used. - // TODO(charlie): `range` here should be the range of the name in `__all__`, not - // the range of `__all__` itself. self.semantic .add_global_reference(binding_id, ExprContext::Load, range); } else { diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F405_F405.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F405_F405.py.snap index 3e3c43d3b9f1e..5c73ac384d32c 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F405_F405.py.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F405_F405.py.snap @@ -8,12 +8,10 @@ F405.py:5:11: F405 `name` may be undefined, or defined from star imports | ^^^^ F405 | -F405.py:11:1: F405 `a` may be undefined, or defined from star imports +F405.py:11:12: F405 `a` may be undefined, or defined from star imports | 9 | print(name) 10 | 11 | __all__ = ['a'] - | ^^^^^^^ F405 + | ^^^ F405 | - - diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_0.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_0.py.snap index e4ced191b50f6..78dfd63657048 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_0.py.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_0.py.snap @@ -1,12 +1,10 @@ --- source: crates/ruff_linter/src/rules/pyflakes/mod.rs --- -F822_0.py:3:1: F822 Undefined name `b` in `__all__` +F822_0.py:3:17: F822 Undefined name `b` in `__all__` | 1 | a = 1 2 | 3 | __all__ = ["a", "b"] - | ^^^^^^^ F822 + | ^^^ F822 | - - diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_0.pyi.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_0.pyi.snap index 320ac6c37fd72..d74b6e8936d56 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_0.pyi.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_0.pyi.snap @@ -1,10 +1,10 @@ --- source: crates/ruff_linter/src/rules/pyflakes/mod.rs --- -F822_0.pyi:4:1: F822 Undefined name `c` in `__all__` +F822_0.pyi:4:22: F822 Undefined name `c` in `__all__` | 2 | b: int # Considered a binding in a `.pyi` stub file, not in a `.py` runtime file 3 | 4 | __all__ = ["a", "b", "c"] # c is flagged as missing; b is not - | ^^^^^^^ F822 + | ^^^ F822 | diff --git a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_1.py.snap b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_1.py.snap index 0e28a36803498..d65deed9e78a3 100644 --- a/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_1.py.snap +++ b/crates/ruff_linter/src/rules/pyflakes/snapshots/ruff_linter__rules__pyflakes__tests__F822_F822_1.py.snap @@ -1,12 +1,10 @@ --- source: crates/ruff_linter/src/rules/pyflakes/mod.rs --- -F822_1.py:3:1: F822 Undefined name `b` in `__all__` +F822_1.py:3:22: F822 Undefined name `b` in `__all__` | 1 | a = 1 2 | 3 | __all__ = list(["a", "b"]) - | ^^^^^^^ F822 + | ^^^ F822 | - - diff --git a/crates/ruff_python_ast/src/all.rs b/crates/ruff_python_ast/src/all.rs index 2b35ee58c5c4c..a792a8742552a 100644 --- a/crates/ruff_python_ast/src/all.rs +++ b/crates/ruff_python_ast/src/all.rs @@ -1,4 +1,5 @@ use bitflags::bitflags; +use ruff_text_size::{Ranged, TextRange}; use crate::helpers::map_subscript; use crate::{self as ast, Expr, Stmt}; @@ -15,17 +16,41 @@ bitflags! { } } +/// Abstraction for a string inside an `__all__` definition, +/// e.g. `"foo"` in `__all__ = ["foo"]` +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct DunderAllName<'a> { + /// The value of the string inside the `__all__` definition + pub name: &'a str, + + /// The range of the string inside the `__all__` definition + pub range: TextRange, +} + +impl Ranged for DunderAllName<'_> { + fn range(&self) -> TextRange { + self.range + } +} + /// Extract the names bound to a given __all__ assignment. /// /// Accepts a closure that determines whether a given name (e.g., `"list"`) is a Python builtin. -pub fn extract_all_names(stmt: &Stmt, is_builtin: F) -> (Vec<&str>, DunderAllFlags) +pub fn extract_all_names(stmt: &Stmt, is_builtin: F) -> (Vec, DunderAllFlags) where F: Fn(&str) -> bool, { - fn add_to_names<'a>(elts: &'a [Expr], names: &mut Vec<&'a str>, flags: &mut DunderAllFlags) { + fn add_to_names<'a>( + elts: &'a [Expr], + names: &mut Vec>, + flags: &mut DunderAllFlags, + ) { for elt in elts { - if let Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) = elt { - names.push(value.to_str()); + if let Expr::StringLiteral(ast::ExprStringLiteral { value, range }) = elt { + names.push(DunderAllName { + name: value.to_str(), + range: *range, + }); } else { *flags |= DunderAllFlags::INVALID_OBJECT; } @@ -96,7 +121,7 @@ where (None, DunderAllFlags::INVALID_FORMAT) } - let mut names: Vec<&str> = vec![]; + let mut names: Vec = vec![]; let mut flags = DunderAllFlags::empty(); if let Some(value) = match stmt { diff --git a/crates/ruff_python_semantic/src/binding.rs b/crates/ruff_python_semantic/src/binding.rs index fff0d852bbe38..9b791653ff940 100644 --- a/crates/ruff_python_semantic/src/binding.rs +++ b/crates/ruff_python_semantic/src/binding.rs @@ -4,6 +4,7 @@ use std::ops::{Deref, DerefMut}; use bitflags::bitflags; use ruff_index::{newtype_index, IndexSlice, IndexVec}; +use ruff_python_ast::all::DunderAllName; use ruff_python_ast::name::QualifiedName; use ruff_python_ast::Stmt; use ruff_source_file::Locator; @@ -370,7 +371,7 @@ impl<'a> FromIterator> for Bindings<'a> { #[derive(Debug, Clone)] pub struct Export<'a> { /// The names of the bindings exported via `__all__`. - pub names: Box<[&'a str]>, + pub names: Box<[DunderAllName<'a>]>, } /// A binding for an `import`, keyed on the name to which the import is bound. diff --git a/crates/ruff_python_semantic/src/definition.rs b/crates/ruff_python_semantic/src/definition.rs index 19f7bb890494b..963b130faff3b 100644 --- a/crates/ruff_python_semantic/src/definition.rs +++ b/crates/ruff_python_semantic/src/definition.rs @@ -5,7 +5,7 @@ use std::fmt::Debug; use std::ops::Deref; use ruff_index::{newtype_index, IndexSlice, IndexVec}; -use ruff_python_ast::{self as ast, Stmt}; +use ruff_python_ast::{self as ast, all::DunderAllName, Stmt}; use ruff_text_size::{Ranged, TextRange}; use crate::analyze::visibility::{ @@ -187,7 +187,7 @@ impl<'a> Definitions<'a> { } /// Resolve the visibility of each definition in the collection. - pub fn resolve(self, exports: Option<&[&str]>) -> ContextualizedDefinitions<'a> { + pub fn resolve(self, exports: Option<&[DunderAllName]>) -> ContextualizedDefinitions<'a> { let mut definitions: IndexVec> = IndexVec::with_capacity(self.len()); @@ -201,7 +201,9 @@ impl<'a> Definitions<'a> { MemberKind::Class(class) => { let parent = &definitions[member.parent]; if parent.visibility.is_private() - || exports.is_some_and(|exports| !exports.contains(&member.name())) + || exports.is_some_and(|exports| { + !exports.iter().any(|export| export.name == member.name()) + }) { Visibility::Private } else { @@ -221,7 +223,9 @@ impl<'a> Definitions<'a> { MemberKind::Function(function) => { let parent = &definitions[member.parent]; if parent.visibility.is_private() - || exports.is_some_and(|exports| !exports.contains(&member.name())) + || exports.is_some_and(|exports| { + !exports.iter().any(|export| export.name == member.name()) + }) { Visibility::Private } else { From 29f6065dacc3fafbf40b11638a03777d28878f6b Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 22 Mar 2024 16:41:36 +0000 Subject: [PATCH 2/7] Apply Micha's suggestion --- .../src/checkers/ast/analyze/definitions.rs | 5 +++-- crates/ruff_python_semantic/src/definition.rs | 10 +++------- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs b/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs index aaac685e2980f..980696e7a1a91 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs @@ -93,7 +93,7 @@ pub(crate) fn definitions(checker: &mut Checker) { } // Compute visibility of all definitions. - let exports: Option> = { + let exports: Vec = { checker .semantic .global_scope() @@ -106,6 +106,7 @@ pub(crate) fn definitions(checker: &mut Checker) { .fold(None, |acc, names| { Some(acc.into_iter().flatten().chain(names).collect()) }) + .unwrap_or_default() }; let definitions = std::mem::take(&mut checker.semantic.definitions); @@ -113,7 +114,7 @@ pub(crate) fn definitions(checker: &mut Checker) { for ContextualizedDefinition { definition, visibility, - } in definitions.resolve(exports.as_deref()).iter() + } in definitions.resolve(&exports).iter() { let docstring = docstrings::extraction::extract_docstring(definition); diff --git a/crates/ruff_python_semantic/src/definition.rs b/crates/ruff_python_semantic/src/definition.rs index 963b130faff3b..6c706ef1d7d45 100644 --- a/crates/ruff_python_semantic/src/definition.rs +++ b/crates/ruff_python_semantic/src/definition.rs @@ -187,7 +187,7 @@ impl<'a> Definitions<'a> { } /// Resolve the visibility of each definition in the collection. - pub fn resolve(self, exports: Option<&[DunderAllName]>) -> ContextualizedDefinitions<'a> { + pub fn resolve(self, exports: &[DunderAllName]) -> ContextualizedDefinitions<'a> { let mut definitions: IndexVec> = IndexVec::with_capacity(self.len()); @@ -201,9 +201,7 @@ impl<'a> Definitions<'a> { MemberKind::Class(class) => { let parent = &definitions[member.parent]; if parent.visibility.is_private() - || exports.is_some_and(|exports| { - !exports.iter().any(|export| export.name == member.name()) - }) + || !exports.iter().any(|export| export.name == member.name()) { Visibility::Private } else { @@ -223,9 +221,7 @@ impl<'a> Definitions<'a> { MemberKind::Function(function) => { let parent = &definitions[member.parent]; if parent.visibility.is_private() - || exports.is_some_and(|exports| { - !exports.iter().any(|export| export.name == member.name()) - }) + || !exports.iter().any(|export| export.name == member.name()) { Visibility::Private } else { From 56b28d2548b4e60f9013de287d010e1ead8b9880 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 22 Mar 2024 16:52:17 +0000 Subject: [PATCH 3/7] Revert "Apply Micha's suggestion" This reverts commit 29f6065dacc3fafbf40b11638a03777d28878f6b. --- .../src/checkers/ast/analyze/definitions.rs | 5 ++--- crates/ruff_python_semantic/src/definition.rs | 10 +++++++--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs b/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs index 980696e7a1a91..aaac685e2980f 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/definitions.rs @@ -93,7 +93,7 @@ pub(crate) fn definitions(checker: &mut Checker) { } // Compute visibility of all definitions. - let exports: Vec = { + let exports: Option> = { checker .semantic .global_scope() @@ -106,7 +106,6 @@ pub(crate) fn definitions(checker: &mut Checker) { .fold(None, |acc, names| { Some(acc.into_iter().flatten().chain(names).collect()) }) - .unwrap_or_default() }; let definitions = std::mem::take(&mut checker.semantic.definitions); @@ -114,7 +113,7 @@ pub(crate) fn definitions(checker: &mut Checker) { for ContextualizedDefinition { definition, visibility, - } in definitions.resolve(&exports).iter() + } in definitions.resolve(exports.as_deref()).iter() { let docstring = docstrings::extraction::extract_docstring(definition); diff --git a/crates/ruff_python_semantic/src/definition.rs b/crates/ruff_python_semantic/src/definition.rs index 6c706ef1d7d45..963b130faff3b 100644 --- a/crates/ruff_python_semantic/src/definition.rs +++ b/crates/ruff_python_semantic/src/definition.rs @@ -187,7 +187,7 @@ impl<'a> Definitions<'a> { } /// Resolve the visibility of each definition in the collection. - pub fn resolve(self, exports: &[DunderAllName]) -> ContextualizedDefinitions<'a> { + pub fn resolve(self, exports: Option<&[DunderAllName]>) -> ContextualizedDefinitions<'a> { let mut definitions: IndexVec> = IndexVec::with_capacity(self.len()); @@ -201,7 +201,9 @@ impl<'a> Definitions<'a> { MemberKind::Class(class) => { let parent = &definitions[member.parent]; if parent.visibility.is_private() - || !exports.iter().any(|export| export.name == member.name()) + || exports.is_some_and(|exports| { + !exports.iter().any(|export| export.name == member.name()) + }) { Visibility::Private } else { @@ -221,7 +223,9 @@ impl<'a> Definitions<'a> { MemberKind::Function(function) => { let parent = &definitions[member.parent]; if parent.visibility.is_private() - || !exports.iter().any(|export| export.name == member.name()) + || exports.is_some_and(|exports| { + !exports.iter().any(|export| export.name == member.name()) + }) { Visibility::Private } else { From a6fc5107836950fb4fbeb8fc68f2852175dbaa5e Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 22 Mar 2024 18:06:10 +0000 Subject: [PATCH 4/7] make fields private --- crates/ruff_linter/src/checkers/ast/mod.rs | 7 ++++--- crates/ruff_python_ast/src/all.rs | 10 ++++++++-- crates/ruff_python_semantic/src/definition.rs | 4 ++-- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index ae0d18da0d79f..718930c364a7f 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -2110,7 +2110,8 @@ impl<'a> Checker<'a> { .flatten() .collect(); - for DunderAllName { name, range } in exports { + for export in exports { + let (name, range) = (export.name(), export.range()); if let Some(binding_id) = self.semantic.global_scope().get(name) { // Mark anything referenced in `__all__` as used. self.semantic @@ -2120,7 +2121,7 @@ impl<'a> Checker<'a> { if self.enabled(Rule::UndefinedLocalWithImportStarUsage) { self.diagnostics.push(Diagnostic::new( pyflakes::rules::UndefinedLocalWithImportStarUsage { - name: (*name).to_string(), + name: name.to_string(), }, range, )); @@ -2130,7 +2131,7 @@ impl<'a> Checker<'a> { if !self.path.ends_with("__init__.py") { self.diagnostics.push(Diagnostic::new( pyflakes::rules::UndefinedExport { - name: (*name).to_string(), + name: name.to_string(), }, range, )); diff --git a/crates/ruff_python_ast/src/all.rs b/crates/ruff_python_ast/src/all.rs index a792a8742552a..71348cdc105eb 100644 --- a/crates/ruff_python_ast/src/all.rs +++ b/crates/ruff_python_ast/src/all.rs @@ -21,10 +21,16 @@ bitflags! { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct DunderAllName<'a> { /// The value of the string inside the `__all__` definition - pub name: &'a str, + name: &'a str, /// The range of the string inside the `__all__` definition - pub range: TextRange, + range: TextRange, +} + +impl DunderAllName<'_> { + pub fn name(&self) -> &str { + self.name + } } impl Ranged for DunderAllName<'_> { diff --git a/crates/ruff_python_semantic/src/definition.rs b/crates/ruff_python_semantic/src/definition.rs index 963b130faff3b..6b8f1d5d865da 100644 --- a/crates/ruff_python_semantic/src/definition.rs +++ b/crates/ruff_python_semantic/src/definition.rs @@ -202,7 +202,7 @@ impl<'a> Definitions<'a> { let parent = &definitions[member.parent]; if parent.visibility.is_private() || exports.is_some_and(|exports| { - !exports.iter().any(|export| export.name == member.name()) + !exports.iter().any(|export| export.name() == member.name()) }) { Visibility::Private @@ -224,7 +224,7 @@ impl<'a> Definitions<'a> { let parent = &definitions[member.parent]; if parent.visibility.is_private() || exports.is_some_and(|exports| { - !exports.iter().any(|export| export.name == member.name()) + !exports.iter().any(|export| export.name() == member.name()) }) { Visibility::Private From 1cc06c8154bd2730b7297d56dcda54e56117a145 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 22 Mar 2024 15:57:57 +0000 Subject: [PATCH 5/7] Add a failing test --- .../fixtures/flake8_pyi/{PYI025.py => PYI025_1.py} | 0 .../fixtures/flake8_pyi/{PYI025.pyi => PYI025_1.pyi} | 0 .../resources/test/fixtures/flake8_pyi/PYI025_2.py | 3 +++ .../resources/test/fixtures/flake8_pyi/PYI025_2.pyi | 3 +++ crates/ruff_linter/src/rules/flake8_pyi/mod.rs | 6 ++++-- ...ules__flake8_pyi__tests__PYI025_PYI025_1.py.snap} | 6 ++---- ...les__flake8_pyi__tests__PYI025_PYI025_1.pyi.snap} | 12 +++++------- 7 files changed, 17 insertions(+), 13 deletions(-) rename crates/ruff_linter/resources/test/fixtures/flake8_pyi/{PYI025.py => PYI025_1.py} (100%) rename crates/ruff_linter/resources/test/fixtures/flake8_pyi/{PYI025.pyi => PYI025_1.pyi} (100%) create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py create mode 100644 crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi rename crates/ruff_linter/src/rules/flake8_pyi/snapshots/{ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025.py.snap => ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.py.snap} (81%) rename crates/ruff_linter/src/rules/flake8_pyi/snapshots/{ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025.pyi.snap => ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.pyi.snap} (85%) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.py similarity index 100% rename from crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025.py rename to crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.py diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi similarity index 100% rename from crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025.pyi rename to crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_1.pyi diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py new file mode 100644 index 0000000000000..170719eedbc7c --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py @@ -0,0 +1,3 @@ +from collections.abc import Set + +__all__ = ["Set"] diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi new file mode 100644 index 0000000000000..170719eedbc7c --- /dev/null +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi @@ -0,0 +1,3 @@ +from collections.abc import Set + +__all__ = ["Set"] diff --git a/crates/ruff_linter/src/rules/flake8_pyi/mod.rs b/crates/ruff_linter/src/rules/flake8_pyi/mod.rs index 79b5e05afe5d3..6f4166339e54c 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/mod.rs @@ -79,8 +79,10 @@ mod tests { #[test_case(Rule::TypeCommentInStub, Path::new("PYI033.pyi"))] #[test_case(Rule::TypedArgumentDefaultInStub, Path::new("PYI011.py"))] #[test_case(Rule::TypedArgumentDefaultInStub, Path::new("PYI011.pyi"))] - #[test_case(Rule::UnaliasedCollectionsAbcSetImport, Path::new("PYI025.py"))] - #[test_case(Rule::UnaliasedCollectionsAbcSetImport, Path::new("PYI025.pyi"))] + #[test_case(Rule::UnaliasedCollectionsAbcSetImport, Path::new("PYI025_1.py"))] + #[test_case(Rule::UnaliasedCollectionsAbcSetImport, Path::new("PYI025_1.pyi"))] + #[test_case(Rule::UnaliasedCollectionsAbcSetImport, Path::new("PYI025_2.py"))] + #[test_case(Rule::UnaliasedCollectionsAbcSetImport, Path::new("PYI025_2.pyi"))] #[test_case(Rule::UnannotatedAssignmentInStub, Path::new("PYI052.py"))] #[test_case(Rule::UnannotatedAssignmentInStub, Path::new("PYI052.pyi"))] #[test_case(Rule::UnassignedSpecialVariableInStub, Path::new("PYI035.py"))] diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.py.snap similarity index 81% rename from crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025.py.snap rename to crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.py.snap index ebbbebba5ce05..3fd40be9ec54c 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025.py.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.py.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs --- -PYI025.py:10:33: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin +PYI025_1.py:10:33: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin | 9 | def f(): 10 | from collections.abc import Set # PYI025 @@ -19,7 +19,7 @@ PYI025.py:10:33: PYI025 [*] Use `from collections.abc import Set as AbstractSet` 12 12 | 13 13 | def f(): -PYI025.py:14:51: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin +PYI025_1.py:14:51: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin | 13 | def f(): 14 | from collections.abc import Container, Sized, Set, ValuesView # PYI025 @@ -42,5 +42,3 @@ PYI025.py:14:51: PYI025 [*] Use `from collections.abc import Set as AbstractSet` 18 18 | class Class: 19 |- member: Set[int] 19 |+ member: AbstractSet[int] - - diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.pyi.snap similarity index 85% rename from crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025.pyi.snap rename to crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.pyi.snap index 7ae80bb5f2b85..53bb3f37dc5c0 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025.pyi.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_1.pyi.snap @@ -1,7 +1,7 @@ --- source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs --- -PYI025.pyi:8:33: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin +PYI025_1.pyi:8:33: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin | 7 | def f(): 8 | from collections.abc import Set # PYI025 @@ -21,7 +21,7 @@ PYI025.pyi:8:33: PYI025 [*] Use `from collections.abc import Set as AbstractSet` 10 10 | def f(): 11 11 | from collections.abc import Container, Sized, Set, ValuesView # PYI025 -PYI025.pyi:11:51: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin +PYI025_1.pyi:11:51: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin | 10 | def f(): 11 | from collections.abc import Container, Sized, Set, ValuesView # PYI025 @@ -41,7 +41,7 @@ PYI025.pyi:11:51: PYI025 [*] Use `from collections.abc import Set as AbstractSet 13 13 | def f(): 14 14 | """Test: local symbol renaming.""" -PYI025.pyi:16:37: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin +PYI025_1.pyi:16:37: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin | 14 | """Test: local symbol renaming.""" 15 | if True: @@ -76,7 +76,7 @@ PYI025.pyi:16:37: PYI025 [*] Use `from collections.abc import Set as AbstractSet 29 29 | def Set(): 30 30 | pass -PYI025.pyi:33:29: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin +PYI025_1.pyi:33:29: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin | 31 | print(Set) 32 | @@ -119,7 +119,7 @@ PYI025.pyi:33:29: PYI025 [*] Use `from collections.abc import Set as AbstractSet 42 42 | def f(): 43 43 | """Test: nonlocal symbol renaming.""" -PYI025.pyi:44:33: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin +PYI025_1.pyi:44:33: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin | 42 | def f(): 43 | """Test: nonlocal symbol renaming.""" @@ -145,5 +145,3 @@ PYI025.pyi:44:33: PYI025 [*] Use `from collections.abc import Set as AbstractSet 50 |- print(Set) 49 |+ AbstractSet = 1 50 |+ print(AbstractSet) - - From d08be5d7f9b6ef21f40f894f1a926cf8f14cdf1a Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 22 Mar 2024 17:46:06 +0000 Subject: [PATCH 6/7] Correctly handle references in `__all__` definitions when renaming symbols in autofixes --- .../test/fixtures/flake8_pyi/PYI025_2.py | 8 +++++ .../test/fixtures/flake8_pyi/PYI025_2.pyi | 8 +++++ crates/ruff_linter/src/checkers/ast/mod.rs | 2 ++ crates/ruff_linter/src/renamer.rs | 28 +++++++++++++++-- .../rules/unconventional_import_alias.rs | 5 ++-- .../unaliased_collections_abc_set_import.rs | 6 ++-- ...flake8_pyi__tests__PYI025_PYI025_2.py.snap | 30 +++++++++++++++++++ ...lake8_pyi__tests__PYI025_PYI025_2.pyi.snap | 30 +++++++++++++++++++ .../rules/invalid_first_argument_name.rs | 4 +++ crates/ruff_python_semantic/src/model.rs | 19 ++++++++++++ crates/ruff_python_semantic/src/reference.rs | 6 ++++ 11 files changed, 139 insertions(+), 7 deletions(-) create mode 100644 crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.py.snap create mode 100644 crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.pyi.snap diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py index 170719eedbc7c..02336afbd7e13 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py @@ -1,3 +1,11 @@ +"""Tests to ensure we correctly rename references inside `__all__`""" + from collections.abc import Set __all__ = ["Set"] + +if True: + __all__ += [r'''Set'''] + +if 1: + __all__ += ["S" "e" "t"] diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi index 170719eedbc7c..02336afbd7e13 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi @@ -1,3 +1,11 @@ +"""Tests to ensure we correctly rename references inside `__all__`""" + from collections.abc import Set __all__ = ["Set"] + +if True: + __all__ += [r'''Set'''] + +if 1: + __all__ += ["S" "e" "t"] diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 718930c364a7f..e771f0c7b6573 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -2113,9 +2113,11 @@ impl<'a> Checker<'a> { for export in exports { let (name, range) = (export.name(), export.range()); if let Some(binding_id) = self.semantic.global_scope().get(name) { + self.semantic.flags |= SemanticModelFlags::DUNDER_ALL_DEFINITION; // Mark anything referenced in `__all__` as used. self.semantic .add_global_reference(binding_id, ExprContext::Load, range); + self.semantic.flags -= SemanticModelFlags::DUNDER_ALL_DEFINITION; } else { if self.semantic.global_scope().uses_star_imports() { if self.enabled(Rule::UndefinedLocalWithImportStarUsage) { diff --git a/crates/ruff_linter/src/renamer.rs b/crates/ruff_linter/src/renamer.rs index 8571d7f53b067..aa7c12abffb86 100644 --- a/crates/ruff_linter/src/renamer.rs +++ b/crates/ruff_linter/src/renamer.rs @@ -4,7 +4,9 @@ use anyhow::{anyhow, Result}; use itertools::Itertools; use ruff_diagnostics::Edit; +use ruff_python_ast::str::raw_contents_range; use ruff_python_semantic::{Binding, BindingKind, Scope, ScopeId, SemanticModel}; +use ruff_source_file::Locator; use ruff_text_size::Ranged; pub(crate) struct Renamer; @@ -104,6 +106,7 @@ impl Renamer { target: &str, scope: &Scope, semantic: &SemanticModel, + locator: &Locator, ) -> Result<(Edit, Vec)> { let mut edits = vec![]; @@ -130,7 +133,9 @@ impl Renamer { }); let scope = scope_id.map_or(scope, |scope_id| &semantic.scopes[scope_id]); - edits.extend(Renamer::rename_in_scope(name, target, scope, semantic)); + edits.extend(Renamer::rename_in_scope( + name, target, scope, semantic, locator, + )); // Find any scopes in which the symbol is referenced as `nonlocal` or `global`. For example, // given: @@ -160,7 +165,9 @@ impl Renamer { .copied() { let scope = &semantic.scopes[scope_id]; - edits.extend(Renamer::rename_in_scope(name, target, scope, semantic)); + edits.extend(Renamer::rename_in_scope( + name, target, scope, semantic, locator, + )); } // Deduplicate any edits. @@ -180,6 +187,7 @@ impl Renamer { target: &str, scope: &Scope, semantic: &SemanticModel, + locator: &Locator, ) -> Vec { let mut edits = vec![]; @@ -202,7 +210,21 @@ impl Renamer { // Rename the references to the binding. edits.extend(binding.references().map(|reference_id| { let reference = semantic.reference(reference_id); - Edit::range_replacement(target.to_string(), reference.range()) + let range_to_replace = { + if reference.in_dunder_all_definition() { + let reference_source = locator.slice(reference.range()); + let relative_range = raw_contents_range(reference_source) + .expect( + "Expected all references on the r.h.s. of an `__all__` definition to be strings" + ); + debug_assert!(!relative_range.is_empty()); + relative_range + reference.start() + } else { + debug_assert!(locator.slice(reference.range()).contains(name)); + reference.range() + } + }; + Edit::range_replacement(target.to_string(), range_to_replace) })); } } diff --git a/crates/ruff_linter/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs b/crates/ruff_linter/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs index e0a2d2f9631f7..3afeee14499fa 100644 --- a/crates/ruff_linter/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs +++ b/crates/ruff_linter/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs @@ -64,8 +64,9 @@ pub(crate) fn unconventional_import_alias( let import = binding.as_any_import()?; let qualified_name = import.qualified_name().to_string(); let expected_alias = conventions.get(qualified_name.as_str())?; + let locator = checker.locator(); - let name = binding.name(checker.locator()); + let name = binding.name(locator); if binding.is_alias() && name == expected_alias { return None; } @@ -82,7 +83,7 @@ pub(crate) fn unconventional_import_alias( diagnostic.try_set_fix(|| { let scope = &checker.semantic().scopes[binding.scope]; let (edit, rest) = - Renamer::rename(name, expected_alias, scope, checker.semantic())?; + Renamer::rename(name, expected_alias, scope, checker.semantic(), locator)?; Ok(Fix::unsafe_edits(edit, rest)) }); } diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs index 531649c305542..2fc0a6761403c 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs @@ -68,7 +68,8 @@ pub(crate) fn unaliased_collections_abc_set_import( return None; } - let name = binding.name(checker.locator()); + let locator = checker.locator(); + let name = binding.name(locator); if name == "AbstractSet" { return None; } @@ -77,7 +78,8 @@ pub(crate) fn unaliased_collections_abc_set_import( if checker.semantic().is_available("AbstractSet") { diagnostic.try_set_fix(|| { let scope = &checker.semantic().scopes[binding.scope]; - let (edit, rest) = Renamer::rename(name, "AbstractSet", scope, checker.semantic())?; + let (edit, rest) = + Renamer::rename(name, "AbstractSet", scope, checker.semantic(), locator)?; Ok(Fix::applicable_edits( edit, rest, diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.py.snap new file mode 100644 index 0000000000000..bafb03a053172 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.py.snap @@ -0,0 +1,30 @@ +--- +source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs +--- +PYI025_2.py:3:29: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin + | +1 | """Tests to ensure we correctly rename references inside `__all__`""" +2 | +3 | from collections.abc import Set + | ^^^ PYI025 +4 | +5 | __all__ = ["Set"] + | + = help: Alias `Set` to `AbstractSet` + +ℹ Unsafe fix +1 1 | """Tests to ensure we correctly rename references inside `__all__`""" +2 2 | +3 |-from collections.abc import Set + 3 |+from collections.abc import Set as AbstractSet +4 4 | +5 |-__all__ = ["Set"] + 5 |+__all__ = ["AbstractSet"] +6 6 | +7 7 | if True: +8 |- __all__ += [r'''Set'''] + 8 |+ __all__ += [r'''AbstractSet'''] +9 9 | +10 10 | if 1: +11 |- __all__ += ["S" "e" "t"] + 11 |+ __all__ += ["AbstractSet"] diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.pyi.snap new file mode 100644 index 0000000000000..e79829304343d --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.pyi.snap @@ -0,0 +1,30 @@ +--- +source: crates/ruff_linter/src/rules/flake8_pyi/mod.rs +--- +PYI025_2.pyi:3:29: PYI025 [*] Use `from collections.abc import Set as AbstractSet` to avoid confusion with the `set` builtin + | +1 | """Tests to ensure we correctly rename references inside `__all__`""" +2 | +3 | from collections.abc import Set + | ^^^ PYI025 +4 | +5 | __all__ = ["Set"] + | + = help: Alias `Set` to `AbstractSet` + +ℹ Unsafe fix +1 1 | """Tests to ensure we correctly rename references inside `__all__`""" +2 2 | +3 |-from collections.abc import Set + 3 |+from collections.abc import Set as AbstractSet +4 4 | +5 |-__all__ = ["Set"] + 5 |+__all__ = ["AbstractSet"] +6 6 | +7 7 | if True: +8 |- __all__ += [r'''Set'''] + 8 |+ __all__ += [r'''AbstractSet'''] +9 9 | +10 10 | if 1: +11 |- __all__ += ["S" "e" "t"] + 11 |+ __all__ += ["AbstractSet"] diff --git a/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs b/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs index a560fab4c1a8c..473c8a0a3d11a 100644 --- a/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs +++ b/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs @@ -6,6 +6,7 @@ use ruff_python_ast as ast; use ruff_python_ast::ParameterWithDefault; use ruff_python_semantic::analyze::function_type; use ruff_python_semantic::{Scope, ScopeKind, SemanticModel}; +use ruff_source_file::Locator; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -239,6 +240,7 @@ pub(crate) fn invalid_first_argument_name( parameters, checker.semantic(), function_type, + checker.locator(), ) }); diagnostics.push(diagnostic); @@ -251,6 +253,7 @@ fn rename_parameter( parameters: &ast::Parameters, semantic: &SemanticModel<'_>, function_type: FunctionType, + locator: &Locator, ) -> Result> { // Don't fix if another parameter has the valid name. if parameters @@ -272,6 +275,7 @@ fn rename_parameter( function_type.valid_first_argument_name(), scope, semantic, + locator, )?; Ok(Some(Fix::unsafe_edits(edit, rest))) } diff --git a/crates/ruff_python_semantic/src/model.rs b/crates/ruff_python_semantic/src/model.rs index 93ec108911bd5..7745426ec9ebc 100644 --- a/crates/ruff_python_semantic/src/model.rs +++ b/crates/ruff_python_semantic/src/model.rs @@ -1590,6 +1590,13 @@ impl<'a> SemanticModel<'a> { .intersects(SemanticModelFlags::COMPREHENSION_ASSIGNMENT) } + /// Return `true` if the model is visiting the r.h.s. of an `__all__` definition + /// (e.g. `"foo"` in `__all__ = ["foo"]`) + pub const fn in_dunder_all_definition(&self) -> bool { + self.flags + .intersects(SemanticModelFlags::DUNDER_ALL_DEFINITION) + } + /// Return an iterator over all bindings shadowed by the given [`BindingId`], within the /// containing scope, and across scopes. pub fn shadowed_bindings( @@ -1941,6 +1948,18 @@ bitflags! { /// ``` const DOCSTRING = 1 << 21; + /// The model is visiting the r.h.s. of a module-level `__all__` definition. + /// + /// This could be any module-level statement that assigns or alters `__all__`, + /// for example: + /// ```python + /// __all__ = ["foo"] + /// __all__: str = ["foo"] + /// __all__ = ("bar",) + /// __all__ += ("baz,") + /// ``` + const DUNDER_ALL_DEFINITION = 1 << 22; + /// The context is in any type annotation. const ANNOTATION = Self::TYPING_ONLY_ANNOTATION.bits() | Self::RUNTIME_EVALUATED_ANNOTATION.bits() | Self::RUNTIME_REQUIRED_ANNOTATION.bits(); diff --git a/crates/ruff_python_semantic/src/reference.rs b/crates/ruff_python_semantic/src/reference.rs index d6735f4232dc8..1b79347684bea 100644 --- a/crates/ruff_python_semantic/src/reference.rs +++ b/crates/ruff_python_semantic/src/reference.rs @@ -93,6 +93,12 @@ impl ResolvedReference { self.flags .intersects(SemanticModelFlags::TYPE_CHECKING_BLOCK) } + + /// Return `true` if the context is in the r.h.s. of an `__all__` definition. + pub const fn in_dunder_all_definition(&self) -> bool { + self.flags + .intersects(SemanticModelFlags::DUNDER_ALL_DEFINITION) + } } impl Ranged for ResolvedReference { From 7e7e8011238136ee4e9cc8cdf0230971fe941371 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 22 Mar 2024 18:39:53 +0000 Subject: [PATCH 7/7] Correctly handle references in `__all__` definitions when renaming symbols in autofixes --- .../test/fixtures/flake8_pyi/PYI025_2.py | 3 ++ .../test/fixtures/flake8_pyi/PYI025_2.pyi | 3 ++ crates/ruff_linter/src/renamer.rs | 31 ++++++++----------- .../rules/unconventional_import_alias.rs | 12 ++++--- .../unaliased_collections_abc_set_import.rs | 12 ++++--- ...flake8_pyi__tests__PYI025_PYI025_2.py.snap | 6 +++- ...lake8_pyi__tests__PYI025_PYI025_2.pyi.snap | 6 +++- .../rules/invalid_first_argument_name.rs | 8 ++--- 8 files changed, 49 insertions(+), 32 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py index 02336afbd7e13..d3bbb2727051a 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.py @@ -9,3 +9,6 @@ if 1: __all__ += ["S" "e" "t"] + +if not False: + __all__ += ["Se" 't'] diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi index 02336afbd7e13..d3bbb2727051a 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi +++ b/crates/ruff_linter/resources/test/fixtures/flake8_pyi/PYI025_2.pyi @@ -9,3 +9,6 @@ if True: if 1: __all__ += ["S" "e" "t"] + +if not False: + __all__ += ["Se" 't'] diff --git a/crates/ruff_linter/src/renamer.rs b/crates/ruff_linter/src/renamer.rs index aa7c12abffb86..1c7c8b9baa7cd 100644 --- a/crates/ruff_linter/src/renamer.rs +++ b/crates/ruff_linter/src/renamer.rs @@ -4,10 +4,9 @@ use anyhow::{anyhow, Result}; use itertools::Itertools; use ruff_diagnostics::Edit; -use ruff_python_ast::str::raw_contents_range; +use ruff_python_codegen::Stylist; use ruff_python_semantic::{Binding, BindingKind, Scope, ScopeId, SemanticModel}; -use ruff_source_file::Locator; -use ruff_text_size::Ranged; +use ruff_text_size::{Ranged, TextSize}; pub(crate) struct Renamer; @@ -106,7 +105,7 @@ impl Renamer { target: &str, scope: &Scope, semantic: &SemanticModel, - locator: &Locator, + stylist: &Stylist, ) -> Result<(Edit, Vec)> { let mut edits = vec![]; @@ -134,7 +133,7 @@ impl Renamer { let scope = scope_id.map_or(scope, |scope_id| &semantic.scopes[scope_id]); edits.extend(Renamer::rename_in_scope( - name, target, scope, semantic, locator, + name, target, scope, semantic, stylist, )); // Find any scopes in which the symbol is referenced as `nonlocal` or `global`. For example, @@ -166,7 +165,7 @@ impl Renamer { { let scope = &semantic.scopes[scope_id]; edits.extend(Renamer::rename_in_scope( - name, target, scope, semantic, locator, + name, target, scope, semantic, stylist, )); } @@ -187,7 +186,7 @@ impl Renamer { target: &str, scope: &Scope, semantic: &SemanticModel, - locator: &Locator, + stylist: &Stylist, ) -> Vec { let mut edits = vec![]; @@ -210,21 +209,17 @@ impl Renamer { // Rename the references to the binding. edits.extend(binding.references().map(|reference_id| { let reference = semantic.reference(reference_id); - let range_to_replace = { + let replacement = { if reference.in_dunder_all_definition() { - let reference_source = locator.slice(reference.range()); - let relative_range = raw_contents_range(reference_source) - .expect( - "Expected all references on the r.h.s. of an `__all__` definition to be strings" - ); - debug_assert!(!relative_range.is_empty()); - relative_range + reference.start() + debug_assert!(!reference.range().is_empty()); + let quote = stylist.quote(); + format!("{quote}{target}{quote}") } else { - debug_assert!(locator.slice(reference.range()).contains(name)); - reference.range() + debug_assert_eq!(TextSize::of(name), reference.range().len()); + target.to_string() } }; - Edit::range_replacement(target.to_string(), range_to_replace) + Edit::range_replacement(replacement, reference.range()) })); } } diff --git a/crates/ruff_linter/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs b/crates/ruff_linter/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs index 3afeee14499fa..4db3974c965cf 100644 --- a/crates/ruff_linter/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs +++ b/crates/ruff_linter/src/rules/flake8_import_conventions/rules/unconventional_import_alias.rs @@ -64,9 +64,8 @@ pub(crate) fn unconventional_import_alias( let import = binding.as_any_import()?; let qualified_name = import.qualified_name().to_string(); let expected_alias = conventions.get(qualified_name.as_str())?; - let locator = checker.locator(); - let name = binding.name(locator); + let name = binding.name(checker.locator()); if binding.is_alias() && name == expected_alias { return None; } @@ -82,8 +81,13 @@ pub(crate) fn unconventional_import_alias( if checker.semantic().is_available(expected_alias) { diagnostic.try_set_fix(|| { let scope = &checker.semantic().scopes[binding.scope]; - let (edit, rest) = - Renamer::rename(name, expected_alias, scope, checker.semantic(), locator)?; + let (edit, rest) = Renamer::rename( + name, + expected_alias, + scope, + checker.semantic(), + checker.stylist(), + )?; Ok(Fix::unsafe_edits(edit, rest)) }); } diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs index 2fc0a6761403c..873df56106db7 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/unaliased_collections_abc_set_import.rs @@ -68,8 +68,7 @@ pub(crate) fn unaliased_collections_abc_set_import( return None; } - let locator = checker.locator(); - let name = binding.name(locator); + let name = binding.name(checker.locator()); if name == "AbstractSet" { return None; } @@ -78,8 +77,13 @@ pub(crate) fn unaliased_collections_abc_set_import( if checker.semantic().is_available("AbstractSet") { diagnostic.try_set_fix(|| { let scope = &checker.semantic().scopes[binding.scope]; - let (edit, rest) = - Renamer::rename(name, "AbstractSet", scope, checker.semantic(), locator)?; + let (edit, rest) = Renamer::rename( + name, + "AbstractSet", + scope, + checker.semantic(), + checker.stylist(), + )?; Ok(Fix::applicable_edits( edit, rest, diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.py.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.py.snap index bafb03a053172..1177dc90ecf8e 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.py.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.py.snap @@ -23,8 +23,12 @@ PYI025_2.py:3:29: PYI025 [*] Use `from collections.abc import Set as AbstractSet 6 6 | 7 7 | if True: 8 |- __all__ += [r'''Set'''] - 8 |+ __all__ += [r'''AbstractSet'''] + 8 |+ __all__ += ["AbstractSet"] 9 9 | 10 10 | if 1: 11 |- __all__ += ["S" "e" "t"] 11 |+ __all__ += ["AbstractSet"] +12 12 | +13 13 | if not False: +14 |- __all__ += ["Se" 't'] + 14 |+ __all__ += ["AbstractSet"] diff --git a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.pyi.snap b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.pyi.snap index e79829304343d..41feeabda6cf0 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.pyi.snap +++ b/crates/ruff_linter/src/rules/flake8_pyi/snapshots/ruff_linter__rules__flake8_pyi__tests__PYI025_PYI025_2.pyi.snap @@ -23,8 +23,12 @@ PYI025_2.pyi:3:29: PYI025 [*] Use `from collections.abc import Set as AbstractSe 6 6 | 7 7 | if True: 8 |- __all__ += [r'''Set'''] - 8 |+ __all__ += [r'''AbstractSet'''] + 8 |+ __all__ += ["AbstractSet"] 9 9 | 10 10 | if 1: 11 |- __all__ += ["S" "e" "t"] 11 |+ __all__ += ["AbstractSet"] +12 12 | +13 13 | if not False: +14 |- __all__ += ["Se" 't'] + 14 |+ __all__ += ["AbstractSet"] diff --git a/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs b/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs index 473c8a0a3d11a..aa049a5e05176 100644 --- a/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs +++ b/crates/ruff_linter/src/rules/pep8_naming/rules/invalid_first_argument_name.rs @@ -4,9 +4,9 @@ use ruff_diagnostics::{Diagnostic, DiagnosticKind, Fix, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast as ast; use ruff_python_ast::ParameterWithDefault; +use ruff_python_codegen::Stylist; use ruff_python_semantic::analyze::function_type; use ruff_python_semantic::{Scope, ScopeKind, SemanticModel}; -use ruff_source_file::Locator; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -240,7 +240,7 @@ pub(crate) fn invalid_first_argument_name( parameters, checker.semantic(), function_type, - checker.locator(), + checker.stylist(), ) }); diagnostics.push(diagnostic); @@ -253,7 +253,7 @@ fn rename_parameter( parameters: &ast::Parameters, semantic: &SemanticModel<'_>, function_type: FunctionType, - locator: &Locator, + stylist: &Stylist, ) -> Result> { // Don't fix if another parameter has the valid name. if parameters @@ -275,7 +275,7 @@ fn rename_parameter( function_type.valid_first_argument_name(), scope, semantic, - locator, + stylist, )?; Ok(Some(Fix::unsafe_edits(edit, rest))) }