Track ranges of names inside __all__ definitions (#10525)

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<Vec<&str>> = {
+    let exports: Option<Vec<DunderAllName>> = {
         checker
             .semantic
             .global_scope()

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};
 
@@ -2098,33 +2099,30 @@ impl<'a> Checker<'a> {
     fn visit_exports(&mut self) {
         let snapshot = self.semantic.snapshot();
 
-        let exports: Vec<(&str, TextRange)> = self
+        let exports: Vec<DunderAllName> = 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 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.
-                // 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 {
                 if self.semantic.global_scope().uses_star_imports() {
                     if self.enabled(Rule::UndefinedLocalWithImportStarUsage) {
                         self.diagnostics.push(Diagnostic::new(
                             pyflakes::rules::UndefinedLocalWithImportStarUsage {
-                                name: (*name).to_string(),
+                                name: name.to_string(),
                             },
                             range,
                         ));
@@ -2134,7 +2132,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,
                             ));

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
    |
-
-

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
   |
-
-

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
   |

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
   |
-
-

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,47 @@ 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
+    name: &'a str,
+
+    /// The range of the string inside the `__all__` definition
+    range: TextRange,
+}
+
+impl DunderAllName<'_> {
+    pub fn name(&self) -> &str {
+        self.name
+    }
+}
+
+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<F>(stmt: &Stmt, is_builtin: F) -> (Vec<&str>, DunderAllFlags)
+pub fn extract_all_names<F>(stmt: &Stmt, is_builtin: F) -> (Vec<DunderAllName>, 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<DunderAllName<'a>>,
+        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 +127,7 @@ where
         (None, DunderAllFlags::INVALID_FORMAT)
     }
 
-    let mut names: Vec<&str> = vec![];
+    let mut names: Vec<DunderAllName> = vec![];
     let mut flags = DunderAllFlags::empty();
 
     if let Some(value) = match stmt {

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<Binding<'a>> 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.

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<DefinitionId, ContextualizedDefinition<'a>> =
             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 {