Skip to content

Commit

Permalink
Respect # noqa directives on __all__ openers (#10798)
Browse files Browse the repository at this point in the history
## Summary

Historically, given:

```python
__all__ = [  # noqa: F822
    "Bernoulli",
    "Beta",
    "Binomial",
]
```

The F822 violations would be attached to the `__all__`, so this `# noqa`
would be enforced for _all_ definitions in the list. This changed in
#10525 for the better, in that we
now use the range of each string. But these `# noqa` directives stopped
working.

This PR sets the `__all__` as a parent range in the diagnostic, so that
these directives are respected once again.

Closes #10795.

## Test Plan

`cargo test`
  • Loading branch information
charliermarsh committed Apr 6, 2024
1 parent 83db62b commit 7fb5f47
Show file tree
Hide file tree
Showing 6 changed files with 102 additions and 33 deletions.
8 changes: 8 additions & 0 deletions crates/ruff_diagnostics/src/diagnostic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,14 @@ impl Diagnostic {
}
}

/// Consumes `self` and returns a new `Diagnostic` with the given parent node.
#[inline]
#[must_use]
pub fn with_parent(mut self, parent: TextSize) -> Self {
self.set_parent(parent);
self
}

/// Set the location of the diagnostic's parent node.
#[inline]
pub fn set_parent(&mut self, parent: TextSize) {
Expand Down
13 changes: 13 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/pyflakes/F822_3.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
"""Respect `# noqa` directives on `__all__` definitions."""

__all__ = [ # noqa: F822
"Bernoulli",
"Beta",
"Binomial",
]


__all__ += [
"ContinuousBernoulli", # noqa: F822
"ExponentialFamily",
]
74 changes: 41 additions & 33 deletions crates/ruff_linter/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,14 @@ use std::path::Path;
use itertools::Itertools;
use log::debug;
use ruff_python_ast::{
self as ast, all::DunderAllName, Comprehension, ElifElseClause, ExceptHandler, Expr,
ExprContext, FStringElement, Keyword, MatchCase, Parameter, ParameterWithDefault, Parameters,
Pattern, Stmt, Suite, UnaryOp,
self as ast, Comprehension, ElifElseClause, ExceptHandler, Expr, ExprContext, FStringElement,
Keyword, MatchCase, Parameter, ParameterWithDefault, Parameters, Pattern, Stmt, Suite, UnaryOp,
};
use ruff_text_size::{Ranged, TextRange, TextSize};

use ruff_diagnostics::{Diagnostic, IsolationLevel};
use ruff_notebook::{CellOffsets, NotebookIndex};
use ruff_python_ast::all::{extract_all_names, DunderAllFlags};
use ruff_python_ast::all::{extract_all_names, DunderAllDefinition, DunderAllFlags};
use ruff_python_ast::helpers::{
collect_import_from_member, extract_handled_exceptions, is_docstring_stmt, to_module_path,
};
Expand Down Expand Up @@ -2109,45 +2108,54 @@ impl<'a> Checker<'a> {
fn visit_exports(&mut self) {
let snapshot = self.semantic.snapshot();

let exports: Vec<DunderAllName> = self
let definitions: Vec<DunderAllDefinition> = 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().copied()),
BindingKind::Export(Export { names }) => {
Some(DunderAllDefinition::new(binding.range(), names.to_vec()))
}
_ => None,
})
.flatten()
.collect();

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) {
self.diagnostics.push(Diagnostic::new(
pyflakes::rules::UndefinedLocalWithImportStarUsage {
name: name.to_string(),
},
range,
));
}
for definition in definitions {
for export in definition.names() {
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.enabled(Rule::UndefinedExport) {
if !self.path.ends_with("__init__.py") {
self.diagnostics.push(Diagnostic::new(
pyflakes::rules::UndefinedExport {
name: name.to_string(),
},
range,
));
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(),
},
range,
)
.with_parent(definition.start()),
);
}
} else {
if self.enabled(Rule::UndefinedExport) {
if !self.path.ends_with("__init__.py") {
self.diagnostics.push(
Diagnostic::new(
pyflakes::rules::UndefinedExport {
name: name.to_string(),
},
range,
)
.with_parent(definition.start()),
);
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/rules/pyflakes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ mod tests {
#[test_case(Rule::UndefinedExport, Path::new("F822_0.pyi"))]
#[test_case(Rule::UndefinedExport, Path::new("F822_1.py"))]
#[test_case(Rule::UndefinedExport, Path::new("F822_2.py"))]
#[test_case(Rule::UndefinedExport, Path::new("F822_3.py"))]
#[test_case(Rule::UndefinedLocal, Path::new("F823.py"))]
#[test_case(Rule::UnusedVariable, Path::new("F841_0.py"))]
#[test_case(Rule::UnusedVariable, Path::new("F841_1.py"))]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F822_3.py:12:5: F822 Undefined name `ExponentialFamily` in `__all__`
|
10 | __all__ += [
11 | "ContinuousBernoulli", # noqa: F822
12 | "ExponentialFamily",
| ^^^^^^^^^^^^^^^^^^^ F822
13 | ]
|
28 changes: 28 additions & 0 deletions crates/ruff_python_ast/src/all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,34 @@ impl Ranged for DunderAllName<'_> {
}
}

/// Abstraction for a collection of names inside an `__all__` definition,
/// e.g. `["foo", "bar"]` in `__all__ = ["foo", "bar"]`
#[derive(Debug, Clone)]
pub struct DunderAllDefinition<'a> {
/// The range of the `__all__` identifier.
range: TextRange,
/// The names inside the `__all__` definition.
names: Vec<DunderAllName<'a>>,
}

impl<'a> DunderAllDefinition<'a> {
/// Initialize a new [`DunderAllDefinition`] instance.
pub fn new(range: TextRange, names: Vec<DunderAllName<'a>>) -> Self {
Self { range, names }
}

/// The names inside the `__all__` definition.
pub fn names(&self) -> &[DunderAllName<'a>] {
&self.names
}
}

impl Ranged for DunderAllDefinition<'_> {
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.
Expand Down

0 comments on commit 7fb5f47

Please sign in to comment.