Emit warnings for invalid `# noqa` directives #5571

charliermarsh · 2023-07-06T20:11:08Z

Summary

This PR adds a ParseError type to the noqa parsing system to enable us to render useful warnings instead of silently failing when parsing noqa codes.

For example, given foo.py:

# ruff: noqa: x

# ruff: noqa foo

# flake8: noqa: F401
import os  # noqa: foo-bar

We would now output:

warning: Invalid `# noqa` directive on line 2: expected a comma-separated list of codes (e.g., `# noqa: F401, F841`).
warning: Invalid `# noqa` directive on line 4: expected `:` followed by a comma-separated list of codes (e.g., `# noqa: F401, F841`).
warning: Invalid `# noqa` directive on line 6: Flake8's blanket exemption does not support exempting specific codes. To exempt specific codes, use, e.g., `# ruff: noqa: F401, F841` instead.
warning: Invalid `# noqa` directive on line 7: expected a comma-separated list of codes (e.g., `# noqa: F401, F841`).

There's one important behavior change here too. Right now, with Flake8, if you do # flake8: noqa: F401, Flake8 treats that as equivalent to # flake8: noqa -- it turns off all diagnostics in the file, not just F401. Historically, we respected this... but, I think it's confusing. So we now raise a warning, and don't respect it at all. This will lead to errors in some projects, but I'd argue that right now, those directives are almost certainly behaving in an unintended way for users anyway.

Closes #3339.

charliermarsh · 2023-07-06T20:14:09Z

crates/ruff/src/noqa.rs

@@ -39,7 +40,7 @@ pub(crate) enum Directive<'a> {

 impl<'a> Directive<'a> {
    /// Extract the noqa `Directive` from a line of Python source code.
-    pub(crate) fn try_extract(text: &'a str, offset: TextSize) -> Option<Self> {
+    pub(crate) fn try_extract(text: &'a str, offset: TextSize) -> Result<Option<Self>, ParseError> {


I find Result<Option> somewhat non-ergonomic, but...

charliermarsh · 2023-07-06T20:20:50Z

crates/ruff/src/noqa.rs

+            let line = ParsedFileExemption::lex_whitespace(line);
+            let Some(line) = ParsedFileExemption::lex_char(line, ':') else {
+                return Ok(None);
+            };


@MichaReiser - Is there any more succinct or idiomatic way to express this? I.e., propagate the None?

Not that I know. I find it readable.

github-actions · 2023-07-06T20:28:18Z

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+21, -0, 0 error(s))

airflow (+21, -0)

+ airflow/__init__.py:129:1: I001 [*] Import block is un-sorted or un-formatted
+ airflow/__init__.py:131:36: F401 [*] `airflow.exceptions.AirflowException` imported but unused
+ airflow/__init__.py:132:40: F401 [*] `airflow.models.dataset.Dataset` imported but unused
+ airflow/__init__.py:18:1: D212 [*] Multi-line docstring summary should start at the first line
+ airflow/__init__.py:52:1: I001 [*] Import block is un-sorted or un-formatted
+ airflow/__init__.py:52:21: F401 [*] `airflow.configuration` imported but unused
+ airflow/__init__.py:60:67: PGH003 Use specific rule codes when ignoring type issues
+ airflow/__init__.py:90:5: ANN202 Missing return type annotation for private function `__getattr__`
+ airflow/__init__.py:94:15: TRY003 Avoid specifying long messages outside the exception class
+ airflow/__init__.py:94:30: EM102 [*] Exception must not use an f-string literal, assign to variable first
+ airflow/__init__.py:99:5: SIM108 [*] Use ternary operator `val = getattr(mod, attr_name) if attr_name else mod` instead of `if`-`else`-block
+ scripts/ci/pre_commit/pre_commit_insert_extras.py:1:1: INP001 File `scripts/ci/pre_commit/pre_commit_insert_extras.py` is part of an implicit namespace package. Add an `__init__.py`.
+ scripts/ci/pre_commit/pre_commit_insert_extras.py:33:1: E402 Module level import not at top of file
+ scripts/ci/pre_commit/pre_commit_insert_extras.py:34:1: E402 Module level import not at top of file
+ scripts/ci/pre_commit/pre_commit_insert_extras.py:62:41: SIM118 [*] Use `extra in EXTRAS_DEPENDENCIES` instead of `extra in EXTRAS_DEPENDENCIES.keys()`
+ scripts/ci/pre_commit/pre_commit_local_yml_mounts.py:1:1: INP001 File `scripts/ci/pre_commit/pre_commit_local_yml_mounts.py` is part of an implicit namespace package. Add an `__init__.py`.
+ scripts/ci/pre_commit/pre_commit_local_yml_mounts.py:25:1: E402 Module level import not at top of file
+ scripts/ci/pre_commit/pre_commit_local_yml_mounts.py:29:65: COM812 [*] Trailing comma missing
+ scripts/ci/pre_commit/pre_commit_local_yml_mounts.py:32:1: E402 Module level import not at top of file
+ scripts/ci/pre_commit/pre_commit_local_yml_mounts.py:34:1: E402 Module level import not at top of file
+ scripts/ci/pre_commit/pre_commit_local_yml_mounts.py:57:14: COM812 [*] Trailing comma missing

Rules changed: 12

Rule	Changes	Additions
E402	5	5
F401	3	3
I001	2	2
INP001	2	2
COM812	2	2
D212	1	1
PGH003	1	1
ANN202	1	1
TRY003	1	1
EM102	1	1
SIM108	1	1
SIM118	1	1

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      8.3±0.02ms     4.9 MB/sec    1.00      8.3±0.02ms     4.9 MB/sec
formatter/numpy/ctypeslib.py               1.00   1782.7±3.21µs     9.3 MB/sec    1.00   1778.5±4.77µs     9.4 MB/sec
formatter/numpy/globals.py                 1.00    196.3±0.48µs    15.0 MB/sec    1.00    195.8±1.19µs    15.1 MB/sec
formatter/pydantic/types.py                1.01      4.1±0.01ms     6.3 MB/sec    1.00      4.0±0.00ms     6.4 MB/sec
linter/all-rules/large/dataset.py          1.00     13.9±0.02ms     2.9 MB/sec    1.01     14.0±0.02ms     2.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.01ms     4.7 MB/sec    1.00      3.5±0.01ms     4.7 MB/sec
linter/all-rules/numpy/globals.py          1.00    363.2±1.80µs     8.1 MB/sec    1.00    364.3±1.13µs     8.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.1±0.01ms     4.1 MB/sec    1.00      6.2±0.01ms     4.1 MB/sec
linter/default-rules/large/dataset.py      1.00      7.1±0.01ms     5.7 MB/sec    1.00      7.1±0.01ms     5.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1464.8±3.69µs    11.4 MB/sec    1.00   1460.3±7.63µs    11.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    157.0±0.77µs    18.8 MB/sec    1.00    157.1±0.74µs    18.8 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.01ms     8.0 MB/sec    1.00      3.2±0.01ms     8.0 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.05     10.4±0.10ms     3.9 MB/sec    1.00      9.9±0.09ms     4.1 MB/sec
formatter/numpy/ctypeslib.py               1.03      2.2±0.03ms     7.6 MB/sec    1.00      2.1±0.04ms     7.8 MB/sec
formatter/numpy/globals.py                 1.02    243.2±6.71µs    12.1 MB/sec    1.00    239.4±7.33µs    12.3 MB/sec
formatter/pydantic/types.py                1.04      4.9±0.05ms     5.2 MB/sec    1.00      4.7±0.06ms     5.4 MB/sec
linter/all-rules/large/dataset.py          1.00     15.6±0.12ms     2.6 MB/sec    1.00     15.7±0.13ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.1±0.04ms     4.1 MB/sec    1.01      4.1±0.05ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00    498.8±5.87µs     5.9 MB/sec    1.01    503.1±7.18µs     5.9 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.9±0.06ms     3.7 MB/sec    1.01      7.0±0.06ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      8.0±0.06ms     5.1 MB/sec    1.00      8.0±0.05ms     5.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1701.1±16.16µs     9.8 MB/sec    1.01  1711.4±18.14µs     9.7 MB/sec
linter/default-rules/numpy/globals.py      1.00    200.0±2.81µs    14.8 MB/sec    1.01    202.7±3.33µs    14.6 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.6±0.03ms     7.1 MB/sec    1.01      3.6±0.05ms     7.0 MB/sec

charliermarsh · 2023-07-06T20:39:36Z

Those errors in Airflow are legitimate. They're using # flake8: noqa: F401 which causes all diagnostics to be ignored in the file.

MichaReiser

This is cool!

MichaReiser · 2023-07-07T09:20:27Z

crates/ruff/src/noqa.rs

+            let line = ParsedFileExemption::lex_whitespace(line);
+            let Some(line) = ParsedFileExemption::lex_char(line, ':') else {
+                return Ok(None);
+            };


Not that I know. I find it readable.

MichaReiser · 2023-07-07T09:24:57Z

crates/ruff/src/noqa.rs

-    fn try_extract(line: &'a str) -> Option<Self> {
-        let line = line.trim_whitespace_start();
+    fn try_extract(line: &'a str) -> Result<Option<Self>, ParseError> {
+        let line = ParsedFileExemption::lex_whitespace(line);


What could help to clean this up potentially is to implement ParsedFileExemption as a "lexer" which returns different Tokens:

enum ExemptionToken { Whitespace, Flake8, Hash, Noqa, Code,

See SimpleTokenizer with its Cursor implementation for inspiration. try_extract is then a parser where you could have eat etc functions. But this may be very overkill.

Yeah in hindsight perhaps I should've done this. I may look into it as a follow-up PR.

crates/ruff/src/noqa.rs

## Summary We now treat `# flake8: noqa: F401` as turning off F401 for the entire file. (Flake8 treats this as turning off _all rules_ for the entire file). This deviates from Flake8, but I think it's a much more user-friendly deviation than what I introduced in #5571. See #5617 for an explanation. Closes #5617.

9128305 · 2023-07-12T23:00:55Z

ruff 0.0.278 ignores non-code noqa. Is this a desired behavior?

file = open('tmp/t.py', 'r') # noqa: open-file-with-context-handler

This works with 0.0.277 with ruff --select SIM115

charliermarsh · 2023-07-12T23:59:37Z

@9128305 - Yes this is expected! Ruff was previously treating that as equivalent to # noqa ("Ignore any errors on this line"). Flake8 has the same behavior. As of 0.0.278, we should be warning that that's an invalid # noqa code. You should instead use the numeric code for now to ignore SIM115 (e.g., # noqa: SIM115).

We will likely support using human-readable names in # noqa comments in the future, but as of now they aren't part of the public API.

9128305 · 2023-07-13T00:03:46Z

Thanks for the clarification!

charliermarsh · 2023-07-13T00:19:49Z

Any time, apologies for any confusion. I wrote a little more about it in the release blog post: https://astral.sh/blog/ruff-v0.0.278#invalid-noqa-directives-now-emit-warnings

ruff 0.0.278 now checks for lowercase noqa directives see astral-sh/ruff#5571

As a consequence: * Change human-readable noqa to codes, due to Ruff limitations: <astral-sh/ruff#5571> * Fix E721: Do not compare types, use `isinstance()` * Updated and ran pre-commit * Ignore MD024 (we don't care)

charliermarsh added the suppression Related to supression of violations e.g. noqa label Jul 6, 2023

charliermarsh requested a review from MichaReiser July 6, 2023 20:13

charliermarsh commented Jul 6, 2023

View reviewed changes

charliermarsh force-pushed the charlie/noqa-warnings branch 2 times, most recently from 4daf367 to 59f1a29 Compare July 6, 2023 20:16

charliermarsh commented Jul 6, 2023

View reviewed changes

konstin approved these changes Jul 7, 2023

View reviewed changes

MichaReiser approved these changes Jul 7, 2023

View reviewed changes

charliermarsh force-pushed the charlie/noqa-warnings branch from b6745bd to 092baf1 Compare July 7, 2023 15:36

Add error type

31210a0

charliermarsh force-pushed the charlie/noqa-warnings branch from 092baf1 to 31210a0 Compare July 8, 2023 16:26

charliermarsh enabled auto-merge (squash) July 8, 2023 16:30

charliermarsh mentioned this pull request Jul 8, 2023

Support # flake8: noqa: F401 #5617

Closed

charliermarsh merged commit 507961f into main Jul 8, 2023

charliermarsh deleted the charlie/noqa-warnings branch July 8, 2023 16:37

charliermarsh mentioned this pull request Jul 8, 2023

Support individual codes on # flake8: noqa directives #5618

Merged

p-linnane mentioned this pull request Jul 12, 2023

ruff 0.0.278 Homebrew/homebrew-core#136446

Merged

tekumara added a commit to tekumara/fakesnow that referenced this pull request Jul 15, 2023

build: fix invalid noqa directive

c4f7c7c

ruff 0.0.278 now checks for lowercase noqa directives see astral-sh/ruff#5571

konstruktoid mentioned this pull request Aug 2, 2023

journald: basic ruff formating ansible/event-driven-ansible#139

Merged

jeohist mentioned this pull request Nov 30, 2023

Bump dependencies to most recent versions mendix/cf-mendix-buildpack#703

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit warnings for invalid `# noqa` directives #5571

Emit warnings for invalid `# noqa` directives #5571

charliermarsh commented Jul 6, 2023 •

edited

Loading

charliermarsh Jul 6, 2023 •

edited

Loading

charliermarsh Jul 6, 2023

MichaReiser Jul 7, 2023

github-actions bot commented Jul 6, 2023 •

edited

Loading

charliermarsh commented Jul 6, 2023

MichaReiser left a comment

MichaReiser Jul 7, 2023

MichaReiser Jul 7, 2023

charliermarsh Jul 7, 2023

9128305 commented Jul 12, 2023 •

edited

Loading

charliermarsh commented Jul 12, 2023

9128305 commented Jul 13, 2023

charliermarsh commented Jul 13, 2023

Emit warnings for invalid # noqa directives #5571

Emit warnings for invalid # noqa directives #5571

Conversation

charliermarsh commented Jul 6, 2023 • edited Loading

Summary

charliermarsh Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

charliermarsh Jul 6, 2023

Choose a reason for hiding this comment

MichaReiser Jul 7, 2023

Choose a reason for hiding this comment

github-actions bot commented Jul 6, 2023 • edited Loading

PR Check Results

Ecosystem

Benchmark

Linux

Windows

charliermarsh commented Jul 6, 2023

MichaReiser left a comment

Choose a reason for hiding this comment

MichaReiser Jul 7, 2023

Choose a reason for hiding this comment

MichaReiser Jul 7, 2023

Choose a reason for hiding this comment

charliermarsh Jul 7, 2023

Choose a reason for hiding this comment

9128305 commented Jul 12, 2023 • edited Loading

charliermarsh commented Jul 12, 2023

9128305 commented Jul 13, 2023

charliermarsh commented Jul 13, 2023

Emit warnings for invalid `# noqa` directives #5571

Emit warnings for invalid `# noqa` directives #5571

charliermarsh commented Jul 6, 2023 •

edited

Loading

charliermarsh Jul 6, 2023 •

edited

Loading

github-actions bot commented Jul 6, 2023 •

edited

Loading

9128305 commented Jul 12, 2023 •

edited

Loading