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

Fix NFKC normalization bug when removing unused imports #12571

Merged
merged 3 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/ruff_linter/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ toml = { workspace = true }
typed-arena = { workspace = true }
unicode-width = { workspace = true }
unicode_names2 = { workspace = true }
unicode-normalization = { workspace = true }
url = { workspace = true }

[dev-dependencies]
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
"""
Test: ensure we're able to correctly remove unused imports
even if they have characters in them that undergo NFKC normalization
"""

from .main import MaµToMan
64 changes: 37 additions & 27 deletions crates/ruff_linter/src/fix/codemods.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
//! Interface for editing code snippets. These functions take statements or expressions as input,
//! and return the modified code snippet as output.
use std::borrow::Cow;

use anyhow::{bail, Result};
use libcst_native::{
Codegen, CodegenState, Expression, ImportNames, NameOrAttribute, ParenthesizableWhitespace,
SmallStatement, Statement,
};
use ruff_python_ast::name::UnqualifiedName;
use smallvec::{smallvec, SmallVec};
use unicode_normalization::UnicodeNormalization;

use ruff_python_ast::name::UnqualifiedName;
use ruff_python_ast::Stmt;
use ruff_python_codegen::Stylist;
use ruff_source_file::Locator;
Expand Down Expand Up @@ -167,39 +170,46 @@ pub(crate) fn retain_imports(
Ok(tree.codegen_stylist(stylist))
}

fn collect_segments<'a>(expr: &'a Expression, parts: &mut SmallVec<[&'a str; 8]>) {
match expr {
Expression::Call(expr) => {
collect_segments(&expr.func, parts);
}
Expression::Attribute(expr) => {
collect_segments(&expr.value, parts);
parts.push(expr.attr.value);
}
Expression::Name(expr) => {
parts.push(expr.value);
fn qualified_name_from_name_or_attribute(module: &NameOrAttribute) -> String {
fn collect_segments<'a>(expr: &'a Expression, parts: &mut SmallVec<[&'a str; 8]>) {
match expr {
Expression::Call(expr) => {
collect_segments(&expr.func, parts);
}
Expression::Attribute(expr) => {
collect_segments(&expr.value, parts);
parts.push(expr.attr.value);
}
Expression::Name(expr) => {
parts.push(expr.value);
}
_ => {}
}
_ => {}
}
}

fn unqualified_name_from_expression<'a>(expr: &'a Expression<'a>) -> Option<UnqualifiedName<'a>> {
let mut segments = smallvec![];
collect_segments(expr, &mut segments);
if segments.is_empty() {
None
} else {
Some(segments.into_iter().collect())
fn unqualified_name_from_expression<'a>(
expr: &'a Expression<'a>,
) -> Option<UnqualifiedName<'a>> {
let mut segments = smallvec![];
collect_segments(expr, &mut segments);
if segments.is_empty() {
None
} else {
Some(segments.into_iter().collect())
}
}
}

MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
fn qualified_name_from_name_or_attribute(module: &NameOrAttribute) -> String {
match module {
NameOrAttribute::N(name) => name.value.to_string(),
let unnormalized = match module {
NameOrAttribute::N(name) => Cow::Borrowed(name.value),
NameOrAttribute::A(attr) => {
let name = attr.attr.value;
let prefix = unqualified_name_from_expression(&attr.value);
prefix.map_or_else(|| name.to_string(), |prefix| format!("{prefix}.{name}"))
prefix.map_or_else(
|| Cow::Borrowed(name),
|prefix| Cow::Owned(format!("{prefix}.{name}")),
)
}
}
};

unnormalized.nfkc().collect()
}
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 @@ -258,6 +258,7 @@ mod tests {
#[test_case(Rule::UnusedImport, Path::new("F401_27__all_mistyped/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_28__all_multiple/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_29__all_conditional/__init__.py"))]
#[test_case(Rule::UnusedImport, Path::new("F401_30.py"))]
fn f401_deprecated_option(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"{}_deprecated_option_{}",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
---
source: crates/ruff_linter/src/rules/pyflakes/mod.rs
---
F401_30.py:6:19: F401 [*] `.main.MaμToMan` imported but unused
|
4 | """
5 |
6 | from .main import MaµToMan
| ^^^^^^^^ F401
|
= help: Remove unused import: `.main.MaμToMan`

ℹ Safe fix
3 3 | even if they have characters in them that undergo NFKC normalization
4 4 | """
5 5 |
6 |-from .main import MaµToMan
Loading