Avoid generating diagnostics with per-file ignores by ntBre · Pull Request #18801 · astral-sh/ruff

ntBre · 2025-06-19T19:30:37Z

Summary

This PR avoids one of the three calls to NoqaCode::rule from #18391 by applying per-file ignores in the LintContext. To help with this, it also replaces all direct uses of LinterSettings.rules.enabled with a LintContext::enabled (or Checker::enabled, which defers to its context) method. There are still some direct accesses to settings.rules, but as far as I can tell these are not in a part of the code where we can really access a LintContext. I believe all of the code reachable from check_path, where the replaced per-file ignore code was, should be converted to the new methods.

Test Plan

Existing tests, with a single snapshot updated for RUF100, which I think actually shows a more accurate diagnostic message now.

github-actions · 2025-06-19T19:39:37Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+8 -1 violations, +0 -0 fixes in 2 projects; 53 projects unchanged)

apache/airflow (+7 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --no-preview --select ALL

+ airflow-core/tests/unit/models/test_renderedtifields.py:205:9: PLC0415 `import` should be at the top-level of a file
+ airflow-core/tests/unit/serialization/serializers/test_serializers.py:249:9: PLC0415 `import` should be at the top-level of a file
+ providers/google/tests/unit/google/cloud/hooks/test_bigquery.py:157:9: PLC0415 `import` should be at the top-level of a file
+ providers/google/tests/unit/google/cloud/hooks/test_bigquery.py:158:9: PLC0415 `import` should be at the top-level of a file
+ providers/google/tests/unit/google/cloud/hooks/test_bigquery_system.py:40:13: PLC0415 `import` should be at the top-level of a file
+ providers/google/tests/unit/google/cloud/hooks/test_bigquery_system.py:44:13: PLC0415 `import` should be at the top-level of a file
+ providers/weaviate/tests/unit/weaviate/hooks/test_weaviate.py:393:9: PLC0415 `import` should be at the top-level of a file

ibis-project/ibis (+1 -1 violations, +0 -0 fixes)

+ ibis/backends/tests/signature/typecheck.py:8:1: RUF100 Unused `noqa` directive (non-enabled: `D205`, `D415`, `D400`)
- ibis/backends/tests/signature/typecheck.py:8:1: RUF100 Unused `noqa` directive (unused: `D205`, `D415`, `D400`)

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PLC0415	7	7	0	0	0
RUF100	2	1	1	0	0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+8 -1 violations, +0 -0 fixes in 2 projects; 53 projects unchanged)

apache/airflow (+7 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview --select ALL

+ airflow-core/tests/unit/models/test_renderedtifields.py:205:9: PLC0415 `import` should be at the top-level of a file
+ airflow-core/tests/unit/serialization/serializers/test_serializers.py:249:9: PLC0415 `import` should be at the top-level of a file
+ providers/google/tests/unit/google/cloud/hooks/test_bigquery.py:157:9: PLC0415 `import` should be at the top-level of a file
+ providers/google/tests/unit/google/cloud/hooks/test_bigquery.py:158:9: PLC0415 `import` should be at the top-level of a file
+ providers/google/tests/unit/google/cloud/hooks/test_bigquery_system.py:40:13: PLC0415 `import` should be at the top-level of a file
+ providers/google/tests/unit/google/cloud/hooks/test_bigquery_system.py:44:13: PLC0415 `import` should be at the top-level of a file
+ providers/weaviate/tests/unit/weaviate/hooks/test_weaviate.py:393:9: PLC0415 `import` should be at the top-level of a file

ibis-project/ibis (+1 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --preview

+ ibis/backends/tests/signature/typecheck.py:8:1: RUF100 Unused `noqa` directive (non-enabled: `D205`, `D415`, `D400`)
- ibis/backends/tests/signature/typecheck.py:8:1: RUF100 Unused `noqa` directive (unused: `D205`, `D415`, `D400`)

Changes by rule (2 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PLC0415	7	7	0	0	0
RUF100	2	1	1	0	0

codspeed-hq · 2025-06-19T19:39:49Z

CodSpeed Instrumentation Performance Report

Merging #18801 will not alter performance

_{Comparing brent/earlier-per-file-ignores (0e7769f) with main (073a71c)}

Summary

✅ 37 untouched benchmarks

ntBre · 2025-06-19T19:55:12Z

I'm a bit confused by both the ecosystem and benchmark results. It's at least plausible that I broke per-file ignores, but I don't see PLC0415 in Airflow's main pyproject.toml, at least. And I'm really surprised that these changes could be slower since they should be skipping the creation of diagnostics that previously would have been created and then discarded. We clone the RuleTable if there are any per-file ignores, but I didn't expect that to be too expensive.

crates/ruff_linter/src/checkers/ast/mod.rs

MichaReiser · 2025-06-19T20:04:07Z

crates/ruff_linter/src/checkers/noqa.rs

-        && !per_file_ignores.contains(Rule::RedirectedNOQA)
-        && !exemption.includes(Rule::RedirectedNOQA)
-    {
+    if context.enabled(Rule::RedirectedNOQA) && !exemption.includes(Rule::RedirectedNOQA) {


Hmm. The behavior here might be different? It explicitly checked if the rule is disabled in the per file ignores (not sure why?)

I cloned the airflow repo and I'm not getting any hits for PLC0415 with this branch locally. Hopefully it will go away when I push another commit. I thought enabled should have that folded in now, but maybe there is a subtle difference.

MichaReiser · 2025-06-19T20:43:25Z

We override the rule selection for airflow to all, see

ruff/python/ruff-ecosystem/ruff_ecosystem/defaults.py

Lines 23 to 26 in 10a78fb

    
           Project( 
        
               repo=Repository(owner="apache", name="airflow", ref="main"), 
        
               check_options=CheckOptions(select="ALL"), 
        
           ),

Not sure why we see this difference.

You can also see the command in the ecosystem results:

ruff check --no-cache --exit-zero --ignore RUF9 --no-fix --output-format concise --no-preview --select ALL

MichaReiser · 2025-06-19T20:47:55Z

crates/ruff_linter/src/linter.rs

        let use_imports = !directives.isort.skip_file
-            && settings
-                .rules
+            && diagnostics


Nit: This reads a bit strange. It gives the impression that we iterate over diagnostics (the enabled diagnostics).

I'm inclined to rename diagnostics to context, and rename all methods to is_rule_enabled, iter_enabled_rules...

I agree, I'm not sure why I called this diagnostics in the first place...

Should I rename Checker::enabled and Checker::any_enabled too? I was trying to be consistent with those for the method names.

I think so. It's not checker that's enabled, it's whether a rule is enabled.

ntBre · 2025-06-19T20:48:39Z

Thanks, I can reproduce this locally now. There are 2579 hits for PLC0415 on 0.12, so I'll try to see if there's anything different about the 7 new ones.

ntBre · 2025-06-19T21:08:47Z

I think the Airflow hits have to do with PLC0415 checking if TID253 is enabled:

ruff/crates/ruff_linter/src/rules/pylint/rules/import_outside_top_level.rs

Lines 65 to 69 in beff9c7

    
           // Check if any of the non-top-level imports are banned by TID253 
        
           // before emitting the diagnostic to avoid conflicts. 
        
           if checker.enabled(Rule::BannedModuleLevelImports) { 
        
               let mut all_aliases_banned = true; 
        
               let mut has_alias = false;

Airflow does have per-file ignores for TID253 that cover all of the new hits. It seems like --select ALL was taking precedence before, but now per-file-ignores is taking precedence, disabling TID253 and causing these diagnostics. Does that sound right to you, or was the old behavior correct?

That also explains why I wasn't reproducing this with just --select PLC0415, you have to select both rules, or ALL obviously.

MichaReiser · 2025-06-19T21:11:33Z

I think the Airflow hits have to do with PLC0415 checking if TID253 is enabled:

This does sounds plossible and, judging by the comment, the new behavior seems more correct

crates/ruff_linter/src/checkers/ast/mod.rs

MichaReiser

Nice!

MichaReiser · 2025-06-20T07:43:54Z

crates/ruff_linter/src/checkers/ast/mod.rs

-    source_file: &'a SourceFile,
-    settings: &'a LinterSettings,
+    source_file: SourceFile,
+    rules: RuleTable,


I found it somewhat convenient that LintContext stores the settings, as I think it will allow us to pass LintContext in most places where we currently pass Checker.

But I guess it makes sense not to store the settings if we also store them on Checker, although I think we should just remove it from Checker. Either way. This seems unrelated to this PR and not urgent but maybe something for a contributor issue.

That makes sense. I just thought it might be confusing to have access to a RulesTable through either self.settings.rules or just self.rules. These fields are all private anyway, though.

If we want to keep the settings on the context, it might be convenient to partially revert the removal here, otherwise someone will have to plumb all the lifetimes back through. The rest sounds like a good follow-up like you said.

Okay I restored the settings field with an expect(unused) and a TODO, just to preserve all of the lifetimes for a follow-up. I can open an issue too and possibly tag it help-wanted.

* main: (21 commits) [`flake8-logging`] Avoid false positive for `exc_info=True` outside `logger.exception` (`LOG014`) (#18737) [`flake8-pie`] Small docs fix to `PIE794` (#18829) [`pylint`] Ignore __init__.py files in (PLC0414) (#18400) Avoid generating diagnostics with per-file ignores (#18801) [`flake8-simplify`] Fix false negatives for shadowed bindings (`SIM910`, `SIM911`) (#18794) [ty] Fix panics when pulling types for `ClassVar` or `Final` parameterized with >1 argument (#18824) [`pylint`] add fix safety section (`PLR1714`) (#18415) [Perflint] Small docs improvement to `PERF401` (#18786) [`pylint`] Avoid flattening nested `min`/`max` when outer call has single argument (`PLW3301`) (#16885) [`ruff`] Added `cls.__dict__.get('__annotations__')` check (`RUF063`) (#18233) [ty] Use `HashTable` in `PlaceTable` (#18819) docs: Correct collections-named-tuple example to use PascalCase assignment (#16884) [ty] ecosystem-analyzer workflow (#18719) [ty] Add support for `@staticmethod`s (#18809) unnecessary_dict_kwargs doc - a note on type checking benefits (#18666) [`flake8-pytest-style`] Mark autofix for `PT001` and `PT023` as unsafe if there's comments in the decorator (#18792) [ty] Surface matched overload diagnostic directly (#18452) [ty] Report when a dataclass contains more than one `KW_ONLY` field (#18731) [`flake8-pie`] Add fix safety section to `PIE794` (#18802) [`pycodestyle`] Add fix safety section to `W291` and `W293` (#18800) ...

ntBre added internal An internal refactor or improvement diagnostics Related to reporting of diagnostics. labels Jun 19, 2025

MichaReiser reviewed Jun 19, 2025

View reviewed changes

crates/ruff_linter/src/checkers/ast/mod.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Jun 19, 2025

View reviewed changes

crates/ruff_linter/src/checkers/ast/mod.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Jun 19, 2025

View reviewed changes

ntBre marked this pull request as ready for review June 19, 2025 21:59

ntBre requested a review from AlexWaygood as a code owner June 19, 2025 21:59

ntBre removed the request for review from AlexWaygood June 19, 2025 21:59

MichaReiser added bug Something isn't working and removed internal An internal refactor or improvement labels Jun 20, 2025

MichaReiser reviewed Jun 20, 2025

View reviewed changes

crates/ruff_linter/src/checkers/ast/mod.rs Outdated Show resolved Hide resolved

MichaReiser approved these changes Jun 20, 2025

View reviewed changes

ntBre added 10 commits June 20, 2025 10:30

use Checker::enabled where possible

e372a2c

defer to LintContext::enabled in Checker

6bbd70b

store just a RuleTable, not the whole Settings

42f5315

prefer LintContext methods to accessing rules directly

fd774da

construct LintContext's SourceFile internally

c3af9ce

apply per-file ignores in LintContext

ffbab4b

always clone RuleTable, restore const

697781b

diagnostics -> context in check_path

534e2a6

update LintContext method names

4a8ecbe

update Checker method names too

a1d9b9e

move empty ignores check to ignores_from_path

deb304e

ntBre force-pushed the brent/earlier-per-file-ignores branch from a0eddd8 to deb304e Compare June 20, 2025 14:36

restore (unused) LinterSettings on LintContext

0e7769f

ntBre merged commit 8cff77c into main Jun 20, 2025
35 checks passed

ntBre deleted the brent/earlier-per-file-ignores branch June 20, 2025 17:33

ntBre mentioned this pull request Jun 20, 2025

Use the LinterSettings on LintContext instead of Checker #18828

Closed

BrewTestBot mentioned this pull request Jun 26, 2025

ruff 0.12.1 Homebrew/homebrew-core#228300

Merged

Comments

Conversation

ntBre commented Jun 19, 2025

Summary

Test Plan

Uh oh!

github-actions bot commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

codspeed-hq bot commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #18801 will not alter performance

Summary

Uh oh!

ntBre commented Jun 19, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser commented Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ntBre commented Jun 19, 2025

Uh oh!

ntBre commented Jun 19, 2025

Uh oh!

MichaReiser commented Jun 19, 2025

Uh oh!

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Jun 19, 2025 •

edited

Loading

`ruff-ecosystem` results

codspeed-hq bot commented Jun 19, 2025 •

edited

Loading

MichaReiser commented Jun 19, 2025 •

edited

Loading