Skip to content

Commit

Permalink
Pass Checker by immutable reference to lint rules (#16012)
Browse files Browse the repository at this point in the history 
This very large PR changes the field `.diagnostics` in the `Checker`
from a `Vec<Diagnostic>` to a `RefCell<Vec<Diagnostic>>`, adds methods
to push new diagnostics to this cell, and then removes unnecessary
mutability throughout all of our lint rule implementations.

Consequently, the compiler may now enforce what was, till now, the
_convention_ that the only changes to the `Checker` that can happen
during a lint are the addition of diagnostics[^1].

The PR is best reviewed commit-by-commit. I have tried to keep the large
commits limited to "bulk actions that you can easily see are performing
the same find/replace on a large number of files", and separate anything
ad-hoc or with larger diffs. Please let me know if there's anything else
I can do to make this easier to review!

Many thanks to [`ast-grep`](https://github.com/ast-grep/ast-grep),
[`helix`](https://github.com/helix-editor/helix), and good ol'
fashioned`git` magic, without which this PR would have taken the rest of
my natural life.

[^1]: And randomly also the seen variables violating `flake8-bugbear`?
  • Loading branch information
@dylwil3
dylwil3 authored Feb 7, 2025
1 parent 1f7a29d commit 46fe177
Show file tree
Hide file tree
Showing 552 changed files with 1,751 additions and 2,288 deletions.
30 changes: 15 additions & 15 deletions crates/ruff_linter/src/checkers/ast/analyze/bindings.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::rules::{
};

/// Run lint rules over the [`Binding`]s.
pub(crate) fn bindings(checker: &mut Checker) {
pub(crate) fn bindings(checker: &Checker) {
if !checker.any_enabled(&[
Rule::AssignmentInAssert,
Rule::InvalidAllFormat,
Expand Down Expand Up @@ -48,22 +48,22 @@ pub(crate) fn bindings(checker: &mut Checker) {
pyflakes::fixes::remove_exception_handler_assignment(binding, checker.locator)
.map(Fix::safe_edit)
});
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::InvalidAllFormat) {
if let Some(diagnostic) = pylint::rules::invalid_all_format(binding) {
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::InvalidAllObject) {
if let Some(diagnostic) = pylint::rules::invalid_all_object(binding) {
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::NonAsciiName) {
if let Some(diagnostic) = pylint::rules::non_ascii_name(binding, checker.locator) {
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::UnconventionalImportAlias) {
Expand All @@ -72,61 +72,61 @@ pub(crate) fn bindings(checker: &mut Checker) {
binding,
&checker.settings.flake8_import_conventions.aliases,
) {
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::UnaliasedCollectionsAbcSetImport) {
if let Some(diagnostic) =
flake8_pyi::rules::unaliased_collections_abc_set_import(checker, binding)
{
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if !checker.source_type.is_stub() && checker.enabled(Rule::UnquotedTypeAlias) {
if let Some(diagnostics) =
flake8_type_checking::rules::unquoted_type_alias(checker, binding)
{
checker.diagnostics.extend(diagnostics);
checker.report_diagnostics(diagnostics);
}
}
if checker.enabled(Rule::UnsortedDunderSlots) {
if let Some(diagnostic) = ruff::rules::sort_dunder_slots(checker, binding) {
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::UsedDummyVariable) {
if let Some(diagnostic) = ruff::rules::used_dummy_variable(checker, binding, binding_id)
{
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::AssignmentInAssert) {
if let Some(diagnostic) = ruff::rules::assignment_in_assert(checker, binding) {
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::PytestUnittestRaisesAssertion) {
if let Some(diagnostic) =
flake8_pytest_style::rules::unittest_raises_assertion_binding(checker, binding)
{
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::ForLoopWrites) {
if let Some(diagnostic) = refurb::rules::for_loop_writes_binding(checker, binding) {
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::CustomTypeVarForSelf) {
if let Some(diagnostic) =
flake8_pyi::rules::custom_type_var_instead_of_self(checker, binding)
{
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::PrivateTypeParameter) {
if let Some(diagnostic) = pyupgrade::rules::private_type_parameter(checker, binding) {
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/comprehension.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::codes::Rule;
use crate::rules::{flake8_simplify, pylint, refurb};

/// Run lint rules over a [`Comprehension`] syntax nodes.
pub(crate) fn comprehension(comprehension: &Comprehension, checker: &mut Checker) {
pub(crate) fn comprehension(comprehension: &Comprehension, checker: &Checker) {
if checker.enabled(Rule::InDictKeys) {
flake8_simplify::rules::key_in_dict_comprehension(checker, comprehension);
}
Expand Down
73 changes: 27 additions & 46 deletions crates/ruff_linter/src/checkers/ast/analyze/deferred_scopes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use crate::rules::{
};

/// Run lint rules over all deferred scopes in the [`SemanticModel`].
pub(crate) fn deferred_scopes(checker: &mut Checker) {
pub(crate) fn deferred_scopes(checker: &Checker) {
if !checker.any_enabled(&[
Rule::AsyncioDanglingTask,
Rule::BadStaticmethodArgument,
Expand Down Expand Up @@ -85,12 +85,11 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
vec![]
};

let mut diagnostics: Vec<Diagnostic> = vec![];
for scope_id in checker.analyze.scopes.iter().rev().copied() {
let scope = &checker.semantic.scopes[scope_id];

if checker.enabled(Rule::UndefinedLocal) {
pyflakes::rules::undefined_local(checker, scope_id, scope, &mut diagnostics);
pyflakes::rules::undefined_local(checker, scope_id, scope);
}

if checker.enabled(Rule::GlobalVariableNotAssigned) {
Expand All @@ -112,7 +111,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
.map(|id| checker.semantic.reference(*id))
.all(ResolvedReference::is_load)
{
diagnostics.push(Diagnostic::new(
checker.report_diagnostic(Diagnostic::new(
pylint::rules::GlobalVariableNotAssigned {
name: (*name).to_string(),
},
Expand Down Expand Up @@ -146,7 +145,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
if scope.kind.is_generator() {
continue;
}
checker.diagnostics.push(Diagnostic::new(
checker.report_diagnostic(Diagnostic::new(
pylint::rules::RedefinedArgumentFromLocal {
name: name.to_string(),
},
Expand Down Expand Up @@ -186,7 +185,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
continue;
}

checker.diagnostics.push(Diagnostic::new(
checker.report_diagnostic(Diagnostic::new(
pyflakes::rules::ImportShadowedByLoopVar {
name: name.to_string(),
row: checker.compute_source_row(shadowed.start()),
Expand Down Expand Up @@ -347,7 +346,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
diagnostic.set_fix(fix.clone());
}

diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
}
Expand All @@ -356,55 +355,47 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
|| matches!(scope.kind, ScopeKind::Module | ScopeKind::Function(_))
{
if checker.enabled(Rule::UnusedPrivateTypeVar) {
flake8_pyi::rules::unused_private_type_var(checker, scope, &mut diagnostics);
flake8_pyi::rules::unused_private_type_var(checker, scope);
}
if checker.enabled(Rule::UnusedPrivateProtocol) {
flake8_pyi::rules::unused_private_protocol(checker, scope, &mut diagnostics);
flake8_pyi::rules::unused_private_protocol(checker, scope);
}
if checker.enabled(Rule::UnusedPrivateTypeAlias) {
flake8_pyi::rules::unused_private_type_alias(checker, scope, &mut diagnostics);
flake8_pyi::rules::unused_private_type_alias(checker, scope);
}
if checker.enabled(Rule::UnusedPrivateTypedDict) {
flake8_pyi::rules::unused_private_typed_dict(checker, scope, &mut diagnostics);
flake8_pyi::rules::unused_private_typed_dict(checker, scope);
}
}

if checker.enabled(Rule::AsyncioDanglingTask) {
ruff::rules::asyncio_dangling_binding(scope, &checker.semantic, &mut diagnostics);
ruff::rules::asyncio_dangling_binding(scope, checker);
}

if let Some(class_def) = scope.kind.as_class() {
if checker.enabled(Rule::BuiltinAttributeShadowing) {
flake8_builtins::rules::builtin_attribute_shadowing(
checker,
scope_id,
scope,
class_def,
&mut diagnostics,
checker, scope_id, scope, class_def,
);
}
if checker.enabled(Rule::FunctionCallInDataclassDefaultArgument) {
ruff::rules::function_call_in_dataclass_default(
checker,
class_def,
&mut diagnostics,
);
ruff::rules::function_call_in_dataclass_default(checker, class_def);
}
if checker.enabled(Rule::MutableClassDefault) {
ruff::rules::mutable_class_default(checker, class_def, &mut diagnostics);
ruff::rules::mutable_class_default(checker, class_def);
}
if checker.enabled(Rule::MutableDataclassDefault) {
ruff::rules::mutable_dataclass_default(checker, class_def, &mut diagnostics);
ruff::rules::mutable_dataclass_default(checker, class_def);
}
}

if matches!(scope.kind, ScopeKind::Function(_) | ScopeKind::Lambda(_)) {
if checker.enabled(Rule::UnusedVariable) {
pyflakes::rules::unused_variable(checker, scope, &mut diagnostics);
pyflakes::rules::unused_variable(checker, scope);
}

if checker.enabled(Rule::UnusedAnnotation) {
pyflakes::rules::unused_annotation(checker, scope, &mut diagnostics);
pyflakes::rules::unused_annotation(checker, scope);
}

if !checker.source_type.is_stub() {
Expand All @@ -415,11 +406,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
Rule::UnusedMethodArgument,
Rule::UnusedStaticMethodArgument,
]) {
flake8_unused_arguments::rules::unused_arguments(
checker,
scope,
&mut diagnostics,
);
flake8_unused_arguments::rules::unused_arguments(checker, scope);
}
}
}
Expand All @@ -428,11 +415,7 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
if !checker.source_type.is_stub()
&& checker.enabled(Rule::RuntimeImportInTypeCheckingBlock)
{
flake8_type_checking::rules::runtime_import_in_type_checking_block(
checker,
scope,
&mut diagnostics,
);
flake8_type_checking::rules::runtime_import_in_type_checking_block(checker, scope);
}
if enforce_typing_only_imports {
let runtime_imports: Vec<&Binding> = checker
Expand All @@ -447,47 +430,45 @@ pub(crate) fn deferred_scopes(checker: &mut Checker) {
checker,
scope,
&runtime_imports,
&mut diagnostics,
);
}

if checker.enabled(Rule::UnusedImport) {
pyflakes::rules::unused_import(checker, scope, &mut diagnostics);
pyflakes::rules::unused_import(checker, scope);
}

if checker.enabled(Rule::ImportPrivateName) {
pylint::rules::import_private_name(checker, scope, &mut diagnostics);
pylint::rules::import_private_name(checker, scope);
}
}

if scope.kind.is_function() {
if checker.enabled(Rule::NoSelfUse) {
pylint::rules::no_self_use(checker, scope_id, scope, &mut diagnostics);
pylint::rules::no_self_use(checker, scope_id, scope);
}

if checker.enabled(Rule::TooManyLocals) {
pylint::rules::too_many_locals(checker, scope, &mut diagnostics);
pylint::rules::too_many_locals(checker, scope);
}

if checker.enabled(Rule::SingledispatchMethod) {
pylint::rules::singledispatch_method(checker, scope, &mut diagnostics);
pylint::rules::singledispatch_method(checker, scope);
}

if checker.enabled(Rule::SingledispatchmethodFunction) {
pylint::rules::singledispatchmethod_function(checker, scope, &mut diagnostics);
pylint::rules::singledispatchmethod_function(checker, scope);
}

if checker.enabled(Rule::BadStaticmethodArgument) {
pylint::rules::bad_staticmethod_argument(checker, scope, &mut diagnostics);
pylint::rules::bad_staticmethod_argument(checker, scope);
}

if checker.any_enabled(&[
Rule::InvalidFirstArgumentNameForClassMethod,
Rule::InvalidFirstArgumentNameForMethod,
]) {
pep8_naming::rules::invalid_first_argument_name(checker, scope, &mut diagnostics);
pep8_naming::rules::invalid_first_argument_name(checker, scope);
}
}
}
checker.diagnostics.extend(diagnostics);
}
12 changes: 5 additions & 7 deletions crates/ruff_linter/src/checkers/ast/analyze/definitions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,11 @@ pub(crate) fn definitions(checker: &mut Checker) {
&checker.semantic,
)
}) {
checker
.diagnostics
.extend(flake8_annotations::rules::definition(
checker,
definition,
*visibility,
));
checker.report_diagnostics(flake8_annotations::rules::definition(
checker,
definition,
*visibility,
));
}
overloaded_name =
flake8_annotations::helpers::overloaded_name(definition, &checker.semantic);
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/checkers/ast/analyze/except_handler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::rules::{
};

/// Run lint rules over an [`ExceptHandler`] syntax node.
pub(crate) fn except_handler(except_handler: &ExceptHandler, checker: &mut Checker) {
pub(crate) fn except_handler(except_handler: &ExceptHandler, checker: &Checker) {
match except_handler {
ExceptHandler::ExceptHandler(ast::ExceptHandlerExceptHandler {
type_,
Expand All @@ -23,7 +23,7 @@ pub(crate) fn except_handler(except_handler: &ExceptHandler, checker: &mut Check
except_handler,
checker.locator,
) {
checker.diagnostics.push(diagnostic);
checker.report_diagnostic(diagnostic);
}
}
if checker.enabled(Rule::RaiseWithoutFromInsideExcept) {
Expand Down
Loading

0 comments on commit 46fe177

Please sign in to comment.