mirrors/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-09-30 22:01:18 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

[Internal] Use more report_diagnostic_if_enabled (#18924)

Browse source 
<!--
Thank you for contributing to Ruff/ty! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title? (Please prefix
with `[ty]` for ty pull
  requests.)
- Does this pull request include references to any relevant issues?
-->

## Summary

<!-- What's the purpose of the change? What does it do, and why? -->

From @ntBre
https://github.com/astral-sh/ruff/pull/18906#discussion_r2162843366 :
> This could be a good target for a follow-up PR, but we could fold
these `if checker.is_rule_enabled { checker.report_diagnostic` checks
into calls to `checker.report_diagnostic_if_enabled`. I didn't notice
these when adding that method.
> 
> Also, the docs on `Checker::report_diagnostic_if_enabled` and
`LintContext::report_diagnostic_if_enabled` are outdated now that the
`Rule` conversion is basically free 😅
> 
> No pressure to take on this refactor, just an idea if you're
interested!

This PR folds those calls. I also updated the doc comments by copying
from `report_diagnostic`.

Note: It seems odd to me that the doc comment for `Checker` says
`Diagnostic` while `LintContext` says `OldDiagnostic`, not sure if that
needs a bigger docs change to fix the inconsistency.

<details>
<summary>Python script to do the changes</summary>

This script assumes it is placed in the top level `ruff` directory (ie
next to `.git`/`crates`/`README.md`)

```py
import re
from copy import copy
from pathlib import Path

ruff_crates = Path(__file__).parent / "crates"

for path in ruff_crates.rglob("**/*.rs"):
    with path.open(encoding="utf-8", newline="") as f:
        original_content = f.read()
    if "is_rule_enabled" not in original_content or "report_diagnostic" not in original_content:
        continue
    original_content_position = 0
    changed_content = ""
    for match in re.finditer(r"(?m)(?:^[ \n]*|(?<=(?P<else>else )))if[ \n]+checker[ \n]*\.is_rule_enabled\([ \n]*Rule::\w+[ \n]*\)[ \n]*{[ \n]*checker\.report_diagnostic\(", original_content):
        # Content between last match and start of this one is unchanged
        changed_content += original_content[original_content_position:match.start()]
        # If this was an else if, a { needs to be added at the start
        if match.group("else"):
            changed_content += "{"
        # This will result in bad formatting, but the precommit cargo format will handle it
        changed_content += "checker.report_diagnostic_if_enabled("
        # Depth tracking would fail if a string/comment included a { or }, but unlikely given the context
        depth = 1
        position = match.end()
        while depth > 0:
            if original_content[position] == "{":
                depth += 1
            if original_content[position] == "}":
                depth -= 1
            position += 1
        # pos - 1 is the closing }
        changed_content += original_content[match.end():position - 1]
        # If this was an else if, a } needs to be added at the end
        if match.group("else"):
            changed_content += "}"
        # Skip the closing }
        original_content_position = position
        if original_content[original_content_position] == "\n":
            # If the } is followed by a \n, also skip it for better formatting
            original_content_position += 1
    # Add remaining content between last match and file end
    changed_content += original_content[original_content_position:]
    with path.open("w", encoding="utf-8", newline="") as f:
        f.write(changed_content)
```

</details>

## Test Plan

<!-- How was it tested? -->

N/A, no tests/functionality affected.
This commit is contained in:
GiGaGon 2025-06-24 18:43:22 -07:00 • committed by GitHub
parent 90f47e9b7b
commit cb152b4725
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
13 changed files with 106 additions and 178 deletions
Download patch file Download diff file Expand all files Collapse all files

44
crates/ruff_linter/src/checkers/ast/analyze/expression.rs
View file

@ -540,14 +540,12 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
match pyflakes::format::FormatSummary::try_from(string_value.to_str()) {
Err(e) => {
// F521
if checker.is_rule_enabled(Rule::StringDotFormatInvalidFormat) {
checker.report_diagnostic(
pyflakes::rules::StringDotFormatInvalidFormat {
message: pyflakes::format::error_to_string(&e),
},
location,
);
}
checker.report_diagnostic_if_enabled(
pyflakes::rules::StringDotFormatInvalidFormat {
message: pyflakes::format::error_to_string(&e),
},
location,
);
}
Ok(summary) => {
if checker
@ -1317,27 +1315,21 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
..
}) => {
// F509
if checker
.is_rule_enabled(Rule::PercentFormatUnsupportedFormatCharacter)
{
checker.report_diagnostic(
pyflakes::rules::PercentFormatUnsupportedFormatCharacter {
char: c,
},
location,
);
}
checker.report_diagnostic_if_enabled(
pyflakes::rules::PercentFormatUnsupportedFormatCharacter {
char: c,
},
location,
);
}
Err(e) => {
// F501
if checker.is_rule_enabled(Rule::PercentFormatInvalidFormat) {
checker.report_diagnostic(
pyflakes::rules::PercentFormatInvalidFormat {
message: e.to_string(),
},
location,
);
}
checker.report_diagnostic_if_enabled(
pyflakes::rules::PercentFormatInvalidFormat {
message: e.to_string(),
},
location,
);
}
Ok(summary) => {
if checker.is_rule_enabled(Rule::PercentFormatExpectedMapping) {

14
crates/ruff_linter/src/checkers/ast/analyze/statement.rs
View file

@ -840,14 +840,12 @@ pub(crate) fn statement(stmt: &Stmt, checker: &mut Checker) {
}
}
// F403
if checker.is_rule_enabled(Rule::UndefinedLocalWithImportStar) {
checker.report_diagnostic(
pyflakes::rules::UndefinedLocalWithImportStar {
name: helpers::format_import_from(level, module).to_string(),
},
stmt.range(),
);
}
checker.report_diagnostic_if_enabled(
pyflakes::rules::UndefinedLocalWithImportStar {
name: helpers::format_import_from(level, module).to_string(),
},
stmt.range(),
);
}
if checker.is_rule_enabled(Rule::RelativeImports) {
flake8_tidy_imports::rules::banned_relative_import(

14
crates/ruff_linter/src/checkers/ast/analyze/unresolved_references.rs
View file

@ -14,14 +14,12 @@ pub(crate) fn unresolved_references(checker: &Checker) {
for reference in checker.semantic.unresolved_references() {
if reference.is_wildcard_import() {
// F406
if checker.is_rule_enabled(Rule::UndefinedLocalWithImportStarUsage) {
checker.report_diagnostic(
pyflakes::rules::UndefinedLocalWithImportStarUsage {
name: reference.name(checker.source()).to_string(),
},
reference.range(),
);
}
checker.report_diagnostic_if_enabled(
pyflakes::rules::UndefinedLocalWithImportStarUsage {
name: reference.name(checker.source()).to_string(),
},
reference.range(),
);
} else {
// F821
if checker.is_rule_enabled(Rule::UndefinedName) {

12
crates/ruff_linter/src/checkers/ast/mod.rs
View file

@ -388,7 +388,7 @@ impl<'a> Checker<'a> {
/// Return a [`DiagnosticGuard`] for reporting a diagnostic.
///
/// The guard derefs to a [`Diagnostic`], so it can be used to further modify the diagnostic
/// The guard derefs to an [`OldDiagnostic`], so it can be used to further modify the diagnostic
/// before it is added to the collection in the checker on `Drop`.
pub(crate) fn report_diagnostic<'chk, T: Violation>(
&'chk self,
@ -401,8 +401,8 @@ impl<'a> Checker<'a> {
/// Return a [`DiagnosticGuard`] for reporting a diagnostic if the corresponding rule is
/// enabled.
///
/// Prefer [`Checker::report_diagnostic`] in general because the conversion from a `Diagnostic`
/// to a `Rule` is somewhat expensive.
/// The guard derefs to an [`OldDiagnostic`], so it can be used to further modify the diagnostic
/// before it is added to the collection in the checker on `Drop`.
pub(crate) fn report_diagnostic_if_enabled<'chk, T: Violation>(
&'chk self,
kind: T,
@ -3150,7 +3150,7 @@ impl<'a> LintContext<'a> {
/// Return a [`DiagnosticGuard`] for reporting a diagnostic.
///
/// The guard derefs to an [`OldDiagnostic`], so it can be used to further modify the diagnostic
/// before it is added to the collection in the collector on `Drop`.
/// before it is added to the collection in the context on `Drop`.
pub(crate) fn report_diagnostic<'chk, T: Violation>(
&'chk self,
kind: T,
@ -3166,8 +3166,8 @@ impl<'a> LintContext<'a> {
/// Return a [`DiagnosticGuard`] for reporting a diagnostic if the corresponding rule is
/// enabled.
///
/// Prefer [`DiagnosticsCollector::report_diagnostic`] in general because the conversion from an
/// `OldDiagnostic` to a `Rule` is somewhat expensive.
/// The guard derefs to an [`OldDiagnostic`], so it can be used to further modify the diagnostic
/// before it is added to the collection in the context on `Drop`.
pub(crate) fn report_diagnostic_if_enabled<'chk, T: Violation>(
&'chk self,
kind: T,

16
crates/ruff_linter/src/rules/flake8_2020/rules/compare.rs
View file

@ -273,9 +273,7 @@ pub(crate) fn compare(checker: &Checker, left: &Expr, ops: &[CmpOp], comparators
],
) = (ops, comparators)
{
if checker.is_rule_enabled(Rule::SysVersionInfo1CmpInt) {
checker.report_diagnostic(SysVersionInfo1CmpInt, left.range());
}
checker.report_diagnostic_if_enabled(SysVersionInfo1CmpInt, left.range());
}
}
}
@ -294,9 +292,7 @@ pub(crate) fn compare(checker: &Checker, left: &Expr, ops: &[CmpOp], comparators
],
) = (ops, comparators)
{
if checker.is_rule_enabled(Rule::SysVersionInfoMinorCmpInt) {
checker.report_diagnostic(SysVersionInfoMinorCmpInt, left.range());
}
checker.report_diagnostic_if_enabled(SysVersionInfoMinorCmpInt, left.range());
}
}
@ -310,11 +306,9 @@ pub(crate) fn compare(checker: &Checker, left: &Expr, ops: &[CmpOp], comparators
) = (ops, comparators)
{
if value.len() == 1 {
if checker.is_rule_enabled(Rule::SysVersionCmpStr10) {
checker.report_diagnostic(SysVersionCmpStr10, left.range());
}
} else if checker.is_rule_enabled(Rule::SysVersionCmpStr3) {
checker.report_diagnostic(SysVersionCmpStr3, left.range());
checker.report_diagnostic_if_enabled(SysVersionCmpStr10, left.range());
} else {
checker.report_diagnostic_if_enabled(SysVersionCmpStr3, left.range());
}
}
}

40
crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs
View file

@ -312,25 +312,21 @@ pub(crate) fn shell_injection(checker: &Checker, call: &ast::ExprCall) {
Some(ShellKeyword {
truthiness: truthiness @ (Truthiness::True | Truthiness::Truthy),
}) => {
if checker.is_rule_enabled(Rule::SubprocessPopenWithShellEqualsTrue) {
checker.report_diagnostic(
SubprocessPopenWithShellEqualsTrue {
safety: Safety::from(arg),
is_exact: matches!(truthiness, Truthiness::True),
},
call.func.range(),
);
}
checker.report_diagnostic_if_enabled(
SubprocessPopenWithShellEqualsTrue {
safety: Safety::from(arg),
is_exact: matches!(truthiness, Truthiness::True),
},
call.func.range(),
);
}
// S603
_ => {
if !is_trusted_input(arg) {
if checker.is_rule_enabled(Rule::SubprocessWithoutShellEqualsTrue) {
checker.report_diagnostic(
SubprocessWithoutShellEqualsTrue,
call.func.range(),
);
}
checker.report_diagnostic_if_enabled(
SubprocessWithoutShellEqualsTrue,
call.func.range(),
);
}
}
}
@ -340,14 +336,12 @@ pub(crate) fn shell_injection(checker: &Checker, call: &ast::ExprCall) {
}) = shell_keyword
{
// S604
if checker.is_rule_enabled(Rule::CallWithShellEqualsTrue) {
checker.report_diagnostic(
CallWithShellEqualsTrue {
is_exact: matches!(truthiness, Truthiness::True),
},
call.func.range(),
);
}
checker.report_diagnostic_if_enabled(
CallWithShellEqualsTrue {
is_exact: matches!(truthiness, Truthiness::True),
},
call.func.range(),
);
}
// S605

20
crates/ruff_linter/src/rules/flake8_logging_format/rules/logging_call.rs
View file

@ -47,22 +47,16 @@ fn check_msg(checker: &Checker, msg: &Expr) {
// Check for string concatenation and percent format.
Expr::BinOp(ast::ExprBinOp { op, .. }) => match op {
Operator::Add => {
if checker.is_rule_enabled(Rule::LoggingStringConcat) {
checker.report_diagnostic(LoggingStringConcat, msg.range());
}
checker.report_diagnostic_if_enabled(LoggingStringConcat, msg.range());
}
Operator::Mod => {
if checker.is_rule_enabled(Rule::LoggingPercentFormat) {
checker.report_diagnostic(LoggingPercentFormat, msg.range());
}
checker.report_diagnostic_if_enabled(LoggingPercentFormat, msg.range());
}
_ => {}
},
// Check for f-strings.
Expr::FString(_) => {
if checker.is_rule_enabled(Rule::LoggingFString) {
checker.report_diagnostic(LoggingFString, msg.range());
}
checker.report_diagnostic_if_enabled(LoggingFString, msg.range());
}
// Check for .format() calls.
Expr::Call(ast::ExprCall { func, .. }) => {
@ -209,14 +203,10 @@ pub(crate) fn logging_call(checker: &Checker, call: &ast::ExprCall) {
if let LoggingCallType::LevelCall(logging_level) = logging_call_type {
match logging_level {
LoggingLevel::Error => {
if checker.is_rule_enabled(Rule::LoggingExcInfo) {
checker.report_diagnostic(LoggingExcInfo, range);
}
checker.report_diagnostic_if_enabled(LoggingExcInfo, range);
}
LoggingLevel::Exception => {
if checker.is_rule_enabled(Rule::LoggingRedundantExcInfo) {
checker.report_diagnostic(LoggingRedundantExcInfo, exc_info.range());
}
checker.report_diagnostic_if_enabled(LoggingRedundantExcInfo, exc_info.range());
}
_ => {}
}

4
crates/ruff_linter/src/rules/flake8_pyi/rules/bad_version_info_comparison.rs
View file

@ -148,8 +148,6 @@ pub(crate) fn bad_version_info_comparison(checker: &Checker, test: &Expr, has_el
}
}
} else {
if checker.is_rule_enabled(Rule::BadVersionInfoComparison) {
checker.report_diagnostic(BadVersionInfoComparison, test.range());
}
checker.report_diagnostic_if_enabled(BadVersionInfoComparison, test.range());
}
}

8
crates/ruff_linter/src/rules/flake8_pyi/rules/unrecognized_platform.rs
View file

@ -114,9 +114,7 @@ pub(crate) fn unrecognized_platform(checker: &Checker, test: &Expr) {
// "in" might also make sense but we don't currently have one.
if !matches!(op, CmpOp::Eq | CmpOp::NotEq) {
if checker.is_rule_enabled(Rule::UnrecognizedPlatformCheck) {
checker.report_diagnostic(UnrecognizedPlatformCheck, test.range());
}
checker.report_diagnostic_if_enabled(UnrecognizedPlatformCheck, test.range());
return;
}
@ -134,8 +132,6 @@ pub(crate) fn unrecognized_platform(checker: &Checker, test: &Expr) {
}
}
} else {
if checker.is_rule_enabled(Rule::UnrecognizedPlatformCheck) {
checker.report_diagnostic(UnrecognizedPlatformCheck, test.range());
}
checker.report_diagnostic_if_enabled(UnrecognizedPlatformCheck, test.range());
}
}

20
crates/ruff_linter/src/rules/flake8_pyi/rules/unrecognized_version_info.rs
View file

@ -147,9 +147,7 @@ pub(crate) fn unrecognized_version_info(checker: &Checker, test: &Expr) {
if let Some(expected) = ExpectedComparator::try_from(left) {
version_check(checker, expected, test, *op, comparator);
} else {
if checker.is_rule_enabled(Rule::UnrecognizedVersionInfoCheck) {
checker.report_diagnostic(UnrecognizedVersionInfoCheck, test.range());
}
checker.report_diagnostic_if_enabled(UnrecognizedVersionInfoCheck, test.range());
}
}
@ -163,33 +161,25 @@ fn version_check(
// Single digit comparison, e.g., `sys.version_info[0] == 2`.
if expected == ExpectedComparator::MajorDigit {
if !is_int_constant(comparator) {
if checker.is_rule_enabled(Rule::UnrecognizedVersionInfoCheck) {
checker.report_diagnostic(UnrecognizedVersionInfoCheck, test.range());
}
checker.report_diagnostic_if_enabled(UnrecognizedVersionInfoCheck, test.range());
}
return;
}
// Tuple comparison, e.g., `sys.version_info == (3, 4)`.
let Expr::Tuple(tuple) = comparator else {
if checker.is_rule_enabled(Rule::UnrecognizedVersionInfoCheck) {
checker.report_diagnostic(UnrecognizedVersionInfoCheck, test.range());
}
checker.report_diagnostic_if_enabled(UnrecognizedVersionInfoCheck, test.range());
return;
};
if !tuple.iter().all(is_int_constant) {
// All tuple elements must be integers, e.g., `sys.version_info == (3, 4)` instead of
// `sys.version_info == (3.0, 4)`.
if checker.is_rule_enabled(Rule::UnrecognizedVersionInfoCheck) {
checker.report_diagnostic(UnrecognizedVersionInfoCheck, test.range());
}
checker.report_diagnostic_if_enabled(UnrecognizedVersionInfoCheck, test.range());
} else if tuple.len() > 2 {
// Must compare against major and minor version only, e.g., `sys.version_info == (3, 4)`
// instead of `sys.version_info == (3, 4, 0)`.
if checker.is_rule_enabled(Rule::PatchVersionComparison) {
checker.report_diagnostic(PatchVersionComparison, test.range());
}
checker.report_diagnostic_if_enabled(PatchVersionComparison, test.range());
}
if checker.is_rule_enabled(Rule::WrongTupleLengthVersionComparison) {

6
crates/ruff_linter/src/rules/pydocstyle/rules/not_empty.rs
View file

@ -4,7 +4,6 @@ use ruff_text_size::Ranged;
use crate::Violation;
use crate::checkers::ast::Checker;
use crate::docstrings::Docstring;
use crate::registry::Rule;
/// ## What it does
/// Checks for empty docstrings.
@ -44,9 +43,6 @@ pub(crate) fn not_empty(checker: &Checker, docstring: &Docstring) -> bool {
if !docstring.body().trim().is_empty() {
return true;
}
if checker.is_rule_enabled(Rule::EmptyDocstring) {
checker.report_diagnostic(EmptyDocstring, docstring.range());
}
checker.report_diagnostic_if_enabled(EmptyDocstring, docstring.range());
false
}

44
crates/ruff_linter/src/rules/pydocstyle/rules/not_missing.rs
View file

@ -8,7 +8,6 @@ use ruff_text_size::TextRange;
use crate::Violation;
use crate::checkers::ast::Checker;
use crate::registry::Rule;
/// ## What it does
/// Checks for undocumented public module definitions.
@ -551,36 +550,29 @@ pub(crate) fn not_missing(
if checker.source_type.is_ipynb() {
return true;
}
if checker.is_rule_enabled(Rule::UndocumentedPublicModule) {
checker.report_diagnostic(UndocumentedPublicModule, TextRange::default());
}
checker.report_diagnostic_if_enabled(UndocumentedPublicModule, TextRange::default());
false
}
Definition::Module(Module {
kind: ModuleKind::Package,
..
}) => {
if checker.is_rule_enabled(Rule::UndocumentedPublicPackage) {
checker.report_diagnostic(UndocumentedPublicPackage, TextRange::default());
}
checker.report_diagnostic_if_enabled(UndocumentedPublicPackage, TextRange::default());
false
}
Definition::Member(Member {
kind: MemberKind::Class(class),
..
}) => {
if checker.is_rule_enabled(Rule::UndocumentedPublicClass) {
checker.report_diagnostic(UndocumentedPublicClass, class.identifier());
}
checker.report_diagnostic_if_enabled(UndocumentedPublicClass, class.identifier());
false
}
Definition::Member(Member {
kind: MemberKind::NestedClass(function),
..
}) => {
if checker.is_rule_enabled(Rule::UndocumentedPublicNestedClass) {
checker.report_diagnostic(UndocumentedPublicNestedClass, function.identifier());
}
checker
.report_diagnostic_if_enabled(UndocumentedPublicNestedClass, function.identifier());
false
}
Definition::Member(Member {
@ -590,9 +582,10 @@ pub(crate) fn not_missing(
if is_overload(&function.decorator_list, checker.semantic()) {
true
} else {
if checker.is_rule_enabled(Rule::UndocumentedPublicFunction) {
checker.report_diagnostic(UndocumentedPublicFunction, function.identifier());
}
checker.report_diagnostic_if_enabled(
UndocumentedPublicFunction,
function.identifier(),
);
false
}
}
@ -605,24 +598,19 @@ pub(crate) fn not_missing(
{
true
} else if is_init(&function.name) {
if checker.is_rule_enabled(Rule::UndocumentedPublicInit) {
checker.report_diagnostic(UndocumentedPublicInit, function.identifier());
}
checker.report_diagnostic_if_enabled(UndocumentedPublicInit, function.identifier());
true
} else if is_new(&function.name) || is_call(&function.name) {
if checker.is_rule_enabled(Rule::UndocumentedPublicMethod) {
checker.report_diagnostic(UndocumentedPublicMethod, function.identifier());
}
checker
.report_diagnostic_if_enabled(UndocumentedPublicMethod, function.identifier());
true
} else if is_magic(&function.name) {
if checker.is_rule_enabled(Rule::UndocumentedMagicMethod) {
checker.report_diagnostic(UndocumentedMagicMethod, function.identifier());
}
checker
.report_diagnostic_if_enabled(UndocumentedMagicMethod, function.identifier());
true
} else {
if checker.is_rule_enabled(Rule::UndocumentedPublicMethod) {
checker.report_diagnostic(UndocumentedPublicMethod, function.identifier());
}
checker
.report_diagnostic_if_enabled(UndocumentedPublicMethod, function.identifier());
true
}
}

42
crates/ruff_linter/src/rules/pydocstyle/rules/sections.rs
View file

@ -1434,14 +1434,12 @@ fn blanks_and_section_underline(
}
if following_lines.peek().is_none() {
if checker.is_rule_enabled(Rule::EmptyDocstringSection) {
checker.report_diagnostic(
EmptyDocstringSection {
name: context.section_name().to_string(),
},
context.section_name_range(),
);
}
checker.report_diagnostic_if_enabled(
EmptyDocstringSection {
name: context.section_name().to_string(),
},
context.section_name_range(),
);
} else if checker.is_rule_enabled(Rule::BlankLinesBetweenHeaderAndContent) {
// If the section is followed by exactly one line, and then a
// reStructuredText directive, the blank lines should be preserved, as in:
@ -1495,14 +1493,12 @@ fn blanks_and_section_underline(
}
}
} else {
if checker.is_rule_enabled(Rule::EmptyDocstringSection) {
checker.report_diagnostic(
EmptyDocstringSection {
name: context.section_name().to_string(),
},
context.section_name_range(),
);
}
checker.report_diagnostic_if_enabled(
EmptyDocstringSection {
name: context.section_name().to_string(),
},
context.section_name_range(),
);
}
} else {
if style.is_numpy() && checker.is_rule_enabled(Rule::MissingDashedUnderlineAfterSection)
@ -1618,14 +1614,12 @@ fn blanks_and_section_underline(
context.summary_range().end(),
)));
}
if checker.is_rule_enabled(Rule::EmptyDocstringSection) {
checker.report_diagnostic(
EmptyDocstringSection {
name: context.section_name().to_string(),
},
context.section_name_range(),
);
}
checker.report_diagnostic_if_enabled(
EmptyDocstringSection {
name: context.section_name().to_string(),
},
context.section_name_range(),
);
}
}