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

F811 is not raised when it is needed #11828

Open
Denis-Alexeev opened this issue Jun 10, 2024 · 4 comments
Open

F811 is not raised when it is needed #11828

Denis-Alexeev opened this issue Jun 10, 2024 · 4 comments
Labels
bug Something isn't working

Comments

@Denis-Alexeev
Copy link

Ruff does not raise F811 error when unused redefinition exists.

Keywords:
"F811", "redefinition"

Code snippet:

"""module."""
class AAA:
    """ddd."""

    def world(self: "AAA") -> None:
        """Docstring."""

    hello = world

    def hello(self: "AAA") -> None:
        """Docstring."""

Command:
ruff check --config /path/to/config /path/to/python/file.py

ruff.toml:

target-version = "py312"

[lint]
select = ["ALL"]

ruff version:
ruff 0.4.8

@charliermarsh charliermarsh added the bug Something isn't working label Jun 10, 2024
@charliermarsh
Copy link
Member

Not clear why it's not being raised here, but it should be AFAICT.

@ukyen8
Copy link
Contributor

ukyen8 commented Jun 13, 2024

@charliermarsh I think the shadowed binding can also be an Assignment. At the moment, Ruff checks shadowed binding for class, function, import, from import.

@ukyen8
Copy link
Contributor

ukyen8 commented Jun 17, 2024

@charliermarsh I like using Ruff. I'd love to contribute to ruff and would like to work on this issue. Is it okay if I take this on?

@dhruvmanila
Copy link
Member

@ukyen8 Yeah, go for it. The code for this rule logic is at

if checker.enabled(Rule::RedefinedWhileUnused) {
for (name, binding_id) in scope.bindings() {
for shadow in checker.semantic.shadowed_bindings(scope_id, binding_id) {
// If the shadowing binding is a loop variable, abort, to avoid overlap
// with F402.
let binding = &checker.semantic.bindings[shadow.binding_id()];
if binding.kind.is_loop_var() {
continue;
}
// If the shadowed binding is used, abort.
let shadowed = &checker.semantic.bindings[shadow.shadowed_id()];
if shadowed.is_used() {
continue;
}
// If the shadowing binding isn't considered a "redefinition" of the
// shadowed binding, abort.
if !binding.redefines(shadowed) {
continue;
}
if shadow.same_scope() {
// If the symbol is a dummy variable, abort, unless the shadowed
// binding is an import.
if !matches!(
shadowed.kind,
BindingKind::Import(..)
| BindingKind::FromImport(..)
| BindingKind::SubmoduleImport(..)
| BindingKind::FutureImport
) && checker.settings.dummy_variable_rgx.is_match(name)
{
continue;
}
let Some(node_id) = shadowed.source else {
continue;
};
// If this is an overloaded function, abort.
if shadowed.kind.is_function_definition() {
if checker
.semantic
.statement(node_id)
.as_function_def_stmt()
.is_some_and(|function| {
visibility::is_overload(
&function.decorator_list,
&checker.semantic,
)
})
{
continue;
}
}
} else {
// Only enforce cross-scope shadowing for imports.
if !matches!(
shadowed.kind,
BindingKind::Import(..)
| BindingKind::FromImport(..)
| BindingKind::SubmoduleImport(..)
| BindingKind::FutureImport
) {
continue;
}
}
// If the bindings are in different forks, abort.
if shadowed.source.map_or(true, |left| {
binding
.source
.map_or(true, |right| !checker.semantic.same_branch(left, right))
}) {
continue;
}
let mut diagnostic = Diagnostic::new(
pyflakes::rules::RedefinedWhileUnused {
name: (*name).to_string(),
row: checker.compute_source_row(shadowed.start()),
},
binding.range(),
);
if let Some(range) = binding.parent_range(&checker.semantic) {
diagnostic.set_parent(range.start());
}
// Remove the import if the binding and the shadowed binding are both imports,
// and both point to the same qualified name.
if let Some(shadowed_import) = shadowed.as_any_import() {
if let Some(import) = binding.as_any_import() {
if shadowed_import.qualified_name() == import.qualified_name() {
if let Some(source) = binding.source {
diagnostic.try_set_fix(|| {
let statement = checker.semantic().statement(source);
let parent = checker.semantic().parent_statement(source);
let edit = fix::edits::remove_unused_imports(
std::iter::once(import.member_name().as_ref()),
statement,
parent,
checker.locator(),
checker.stylist(),
checker.indexer(),
)?;
Ok(Fix::safe_edit(edit).isolate(Checker::isolation(
checker.semantic().parent_statement_id(source),
)))
});
}
}
}
}
diagnostics.push(diagnostic);
}
}
}

Don't hesitate to ask any questions if you're stuck either here or on Discord :)

charliermarsh pushed a commit that referenced this issue Jun 23, 2024
)

## Summary
This PR updates `F811` rule to include assignment as possible shadowed
binding. This will fix issue: #11828 .

## Test Plan

Add a test file, F811_30.py, which includes a redefinition after an
assignment and a verified snapshot file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants