Skip to content

Comments

[Internal] Use more report_diagnostic_if_enabled#18924

Merged
ntBre merged 11 commits intoastral-sh:mainfrom
MeGaGiGaGon:use-more-report_diagnostic_if_enabled
Jun 25, 2025
Merged

[Internal] Use more report_diagnostic_if_enabled#18924
ntBre merged 11 commits intoastral-sh:mainfrom
MeGaGiGaGon:use-more-report_diagnostic_if_enabled

Conversation

@MeGaGiGaGon
Copy link
Contributor

@MeGaGiGaGon MeGaGiGaGon commented Jun 24, 2025 •
edited
Loading

Summary

From @ntBre #18906 (comment) :

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.

Python script to do the changes

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

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)

Test Plan

N/A, no tests/functionality affected.

@MeGaGiGaGon MeGaGiGaGon requested a review from AlexWaygood as a code owner June 24, 2025 20:27
@AlexWaygood AlexWaygood requested review from ntBre and removed request for AlexWaygood June 24, 2025 20:29
@github-actions
Copy link
Contributor

github-actions bot commented Jun 24, 2025 •
edited
Loading

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@ntBre ntBre added the internal An internal refactor or improvement label Jun 24, 2025
ntBre
ntBre approved these changes Jun 24, 2025
View reviewed changes
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thank you! I just had a couple of small suggestions on the docs.

Also, I probably should have mentioned this earlier (sorry!), and it looks like your script did a good job, but this is a nice use case for ast-grep. I just started using it recently, and it's really handy. Something like this would probably work too:

ast-grep \
-p 'if checker.is_rule_enabled($RULE) { checker.report_diagnostic($KIND, $RANGE); }' \
-r 'checker.report_diagnostic_if_enabled($KIND, $RANGE);'

It only found two additional cases, and one of them I had to clean up manually since it was in an else-if.

Diff 
diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs
index 21e8aa0d11..8b094b346f 100644
--- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs
+++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs
@@ -1311,16 +1311,12 @@ pub(crate) fn expression(expr: &Expr, checker: &Checker) {
                             typ: CFormatErrorType::UnsupportedFormatChar(c),
                             ..
                         }) => {
-                            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) => {
                             checker.report_diagnostic_if_enabled(
diff --git a/crates/ruff_linter/src/rules/flake8_2020/rules/compare.rs b/crates/ruff_linter/src/rules/flake8_2020/rules/compare.rs
index 01751361da..12c18a2dd7 100644
--- a/crates/ruff_linter/src/rules/flake8_2020/rules/compare.rs
+++ b/crates/ruff_linter/src/rules/flake8_2020/rules/compare.rs
@@ -307,8 +307,8 @@ pub(crate) fn compare(checker: &Checker, left: &Expr, ops: &[CmpOp], comparators
         {
             if value.len() == 1 {
                 checker.report_diagnostic_if_enabled(SysVersionCmpStr10, left.range());
-            } else if checker.is_rule_enabled(Rule::SysVersionCmpStr3) {
-                checker.report_diagnostic(SysVersionCmpStr3, left.range());
+            } else {
+                checker.report_diagnostic_if_enabled(SysVersionCmpStr3, left.range());
             }
         }
     }

@MeGaGiGaGon
Copy link
Contributor Author

MeGaGiGaGon commented Jun 24, 2025

Docs updated to fix the inconsistencies, and I updated the script to handle those cases. ast-grep looks like a really cool alternative to my normal regex based shenanigans, I'll have to try it out if I do something like this again.

ntBre
ntBre approved these changes Jun 25, 2025
View reviewed changes
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you!

I think I should have merged this before your rule code changes 😬 I hope the merge conflicts aren't too much of a pain. I'm happy to resolve them if you want.

@MeGaGiGaGon
Copy link
Contributor Author

MeGaGiGaGon commented Jun 25, 2025

I'm surprised there were any merge conflicts, I thought git would have been smarter. There weren't too many and they were all easy to resolve (assuming/hoping I did it right)

@ntBre ntBre merged commit cb152b4 into astral-sh:main Jun 25, 2025
35 checks passed
@MeGaGiGaGon MeGaGiGaGon deleted the use-more-report_diagnostic_if_enabled branch June 25, 2025 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ntBre ntBre ntBre approved these changes

Assignees

No one assigned

Labels

internal An internal refactor or improvement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MeGaGiGaGon @ntBre