Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 39 additions & 19 deletions df12_python_lints/suppressions.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
}

_LINT_DIRECTIVE: typ.Final = re.compile(
r"\bnoqa\b|\bruff\s*:\s*noqa\b|\bpylint\s*:\s*disable",
r"\bnoqa\b|\bpylint\s*:\s*disable",
re.IGNORECASE,
)

Expand All @@ -60,14 +60,34 @@
re.IGNORECASE,
)

# Comma-separated code lists; a space-separated word after the codes is
# prose, so these deliberately do not admit bare spaces between items.
_CODE_LIST: typ.Final = r"[A-Za-z0-9]+(?:\s*,\s*[A-Za-z0-9]+)*"
# Inline `noqa` directives accept comma- or whitespace-separated rule codes.
# Restricting each item to Ruff's letter-plus-digit shape leaves trailing prose
# distinguishable.
_NOQA_CODE_LIST: typ.Final = (
r"[A-Za-z]+[0-9]+"
r"(?:(?:\s*,\s*|\s+)[A-Za-z]+[0-9]+)*"
r"\s*,?"
)
_RUFF_RULE_LIST: typ.Final = r"[\w\-]+(?:\s*,\s*[\w\-]+)*\s*,?"
_NAME_LIST: typ.Final = r"[\w\-]+(?:\s*,\s*[\w\-]+)*"
_RUFF_INLINE_DIRECTIVE: typ.Final = re.compile(
rf"\bruff\s*:\s*ignore\s*\[\s*{_RUFF_RULE_LIST}\s*\](?=\s|$)"
)
_RUFF_STANDALONE_DIRECTIVE: typ.Final = re.compile(
rf"\bruff\s*:\s*(?:file-ignore|disable)"
rf"\s*\[\s*{_RUFF_RULE_LIST}\s*\](?=\s|$)"
)
_RUFF_ENABLE_DIRECTIVE: typ.Final = re.compile(
rf"^\s*#\s*ruff\s*:\s*enable"
rf"\s*\[\s*{_RUFF_RULE_LIST}\s*\](?=\s|$)"
)

_DIRECTIVE_ONLY_SEGMENT: typ.Final = re.compile(
rf"""^\s*(?:
(?:ruff\s*:\s*)? noqa (?:\s*:\s*{_CODE_LIST})?
(?:(?:ruff|flake8)\s*:\s*)? noqa
(?:\s*:\s*{_NOQA_CODE_LIST})?
| (?-i:ruff\s*:\s*(?:ignore|file-ignore|disable|enable)
\s*\[\s*{_RUFF_RULE_LIST}\s*\])
| pylint\s*:\s*disable(?:-next|-line)?\s*=\s*{_NAME_LIST}
| type\s*:\s*ignore (?:\[[\w\s,\-]*\])?
| (?:pyright|ty)\s*:\s*ignore (?:\[[\w\s,\-]*\])?
Expand All @@ -84,18 +104,17 @@ class _Comment(typ.NamedTuple):
is_standalone: bool


def _directive_symbols(comment_text: str) -> tuple[str, ...]:
"""Return the message symbols for pragmas present in *comment_text*.

Examples
--------
``"# noqa: S101"`` maps to the lint suppression symbol; a plain
comment maps to an empty tuple.
"""
def _directive_symbols(comment: _Comment) -> tuple[str, ...]:
"""Return message symbols for pragmas present in *comment*."""
symbols: list[str] = []
if _LINT_DIRECTIVE.search(comment_text):
has_lint_directive = (
_LINT_DIRECTIVE.search(comment.text)
or _RUFF_INLINE_DIRECTIVE.search(comment.text)
or (comment.is_standalone and _RUFF_STANDALONE_DIRECTIVE.search(comment.text))
)
if has_lint_directive:
symbols.append("lint-suppression-without-explanation")
if _TYPE_DIRECTIVE.search(comment_text):
if _TYPE_DIRECTIVE.search(comment.text):
symbols.append("typecheck-suppression-without-explanation")
return tuple(symbols)

Expand Down Expand Up @@ -147,7 +166,7 @@ def process_tokens(self, tokens: list[tokenize.TokenInfo]) -> None:
"""
comments = _collect_comments(tokens)
for row, comment in sorted(comments.items()):
symbols = _directive_symbols(comment.text)
symbols = _directive_symbols(comment)
if not symbols or _has_inline_explanation(comment.text):
continue
if _has_preceding_explanation(comments, row):
Expand Down Expand Up @@ -180,8 +199,7 @@ def _collect_comments(
def _has_preceding_explanation(comments: dict[int, _Comment], row: int) -> bool:
"""Return whether the line above *row* holds an explanatory comment.

Only a standalone comment that is not itself a suppression pragma
counts.
Only a standalone comment containing prose beyond any directives counts.

Examples
--------
Expand All @@ -191,4 +209,6 @@ def _has_preceding_explanation(comments: dict[int, _Comment], row: int) -> bool:
preceding = comments.get(row - 1)
if preceding is None or not preceding.is_standalone:
return False
return not _directive_symbols(preceding.text)
if _RUFF_ENABLE_DIRECTIVE.match(preceding.text):
return False
return _has_inline_explanation(preceding.text)
3 changes: 3 additions & 0 deletions docs/contents.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ documentation set.
- [Version 0.2.0 migration guide](migration-0.2.0.md) explains the new
dataclass-slots rule, the reassigned message identifiers, and the required
pylint configuration changes.
- [Version 0.3.0 migration guide](migration-0.3.0.md) explains the expanded
Ruff suppression grammar, explanation requirements, and neutral range
terminators.
- [ADR 001](adr-001-conservative-dataclass-layout-analysis.md) records the
conservative, cached layout analysis and supported Pylint range for R9111.

Expand Down
22 changes: 22 additions & 0 deletions docs/developers-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,28 @@ comment tokens to find suppression pragmas and the explanations that may
accompany them, because a bare pragma carries no node in the abstract syntax
tree to attach a check to.

Its Ruff grammar follows Ruff 0.16.0:

- Bare `noqa` is case-insensitive, including when it follows code on the same
line. The file-level `ruff: noqa` and `flake8: noqa` aliases are
case-sensitive and must occupy standalone comments; they accept blanket
suppression or rule-code lists.
- `ruff: ignore[...]`, `ruff: file-ignore[...]`, and `ruff: disable[...]`
accept rule codes or preview rule names. Whitespace around the colon, before
the opening bracket, and around comma separators is permitted, as is a
trailing comma. Ruff keywords are case-sensitive; `file-ignore`, `disable`,
and `enable` are recognized only in standalone comments, while `ignore` may
follow code on the same line.
- `ruff: enable[...]` is a range terminator, not a suppression opener. It
emits no C9106 diagnostic and is classified as a directive rather than
explanatory prose.

A suppression opener is explained by non-directive prose in the same comment
segment, prose after a second `#`, or a standalone prose comment immediately
above it. Another pragma on the preceding line never explains it. This includes
`ruff: enable[...]`: the terminator is neutral, so it neither requires an
explanation nor supplies one for the next suppression.

The `ambrleaks` subpackage is a separate, standalone scanner exposed as its own
console script, split into four modules: `rules.py` pairs each detection
pattern with an optional entropy floor and allowlists, `scanner.py` walks
Expand Down
73 changes: 73 additions & 0 deletions docs/migration-0.3.0.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# Migrate to version 0.3.0

Version 0.3.0 expands suppression-comment checking to recognize the valid Ruff
and Flake8 forms accepted by the project. Suppression directives must record a
reason, while a Ruff range terminator remains neutral.

## Explain valid suppression directives

Review existing suppression comments after upgrading. The checker reports an
unexplained valid directive for `ruff: ignore[...]`, standalone
`ruff: file-ignore[...]`, standalone `ruff: disable[...]`, and standalone
`ruff: noqa` or `flake8: noqa`, in addition to the other lint and type-check
suppressions already covered by C9106 and C9107.

An explanation can be non-directive prose in the same comment segment, prose
after a second `#`, or a standalone prose comment immediately above the
directive:

```python
value = eval(text) # ruff: ignore[S307] # input is a vetted config literal

# Generated URLs cannot be wrapped.
# ruff: file-ignore [E501,]

# ruff: disable[F841,] # generated fixture intentionally binds this name

# ruff: noqa: F401 # generated package exports imported names

# flake8: noqa: F401 # generated module exports imported names
```

A pragma on the preceding line is not an explanation. The prose comment above
must explain the following directive; another pragma, including a Ruff range
directive, does not.

## Preserve Ruff syntax constraints

Ruff keywords are case-sensitive. Bare `noqa` is the exception: it is
case-insensitive and may follow code on the same line. The file-level
`ruff: noqa` and `flake8: noqa` aliases are case-sensitive and must be
standalone comments. Among bracketed Ruff directives, only `ruff: ignore[...]`
may follow code; `ruff: file-ignore[...]`, `ruff: disable[...]`, and
`ruff: enable[...]` must be standalone comments.

Whitespace before the selector bracket is valid, as are spaces around comma
separators and a trailing comma. Selectors may be rule codes or preview rule
names. These forms are therefore equivalent for suppression detection:

```python
value = eval(text) # ruff: ignore[S307,]
value = eval(text) # ruff: ignore [S307,]

# ruff: file-ignore [F401, ARG001,]
# ruff: disable[E741, F841,]
```

## Keep `ruff: enable[...]` neutral

`ruff: enable[...]` ends a suppression range; it does not suppress a
diagnostic. It needs no explanation and cannot explain a later suppression:

```python
# ruff: enable [E501,]
value = 1 # ruff: ignore [F841] # retained for generated fixture parity
```

The `enable` directive itself is not reported as C9106, and trailing prose on
that directive does not satisfy the explanation requirement for the next
suppression.

After updating existing comments, run the normal lint targets and resolve any
new C9106 or C9107 diagnostics. Keep explanations close to the directive so the
compatibility reason remains reviewable when the suppression is revisited.
21 changes: 16 additions & 5 deletions docs/users-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,20 +123,31 @@ import os.path
join = os.path.join # flagged
```

Use `from os.path import join` instead, so
importers and type checkers see a real import binding. Call results, aliases of
names defined in the same module, and assignments inside functions are not
flagged.
Use `from os.path import join` instead, so importers and type checkers see a
real import binding. Call results, aliases of names defined in the same module,
and assignments inside functions are not flagged.

### Suppressions without explanations (C9106, C9107)

Two checkers require every suppression pragma to record a reason:

- `lint-suppression-without-explanation` (C9106) covers lint pragmas:
`noqa`, `ruff: noqa`, and `pylint: disable`.
`noqa`, `ruff: noqa`, `flake8: noqa`, `ruff: ignore`, `ruff: file-ignore`,
the range directive `ruff: disable`, and `pylint: disable`.
- `typecheck-suppression-without-explanation` (C9107) covers type-check
pragmas: `type: ignore`, `pyright: ignore`, `ty: ignore`, and `mypy:`.

Bare `noqa`, including inline `noqa` after code, is case-insensitive. The
`ruff: noqa` and `flake8: noqa` file-level aliases have case-sensitive prefixes
and must occupy standalone comments. Other Ruff directive keywords are also
case-sensitive: `ruff: file-ignore`, `ruff: disable`, and `ruff: enable` must
occupy a standalone comment; only `ruff: ignore` may follow code on the same
line.

`ruff: enable[...]` ends a suppression range rather than suppressing a
diagnostic itself. It therefore needs no explanation and does not count as an
explanation for a suppression on the next line.

An explanation may sit after a second `#` in the same comment, as trailing
prose in the pragma segment, or as a standalone comment on the line above:

Expand Down
58 changes: 0 additions & 58 deletions tests/test_properties.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@

from __future__ import annotations

import io
import math
import operator
import tokenize
import typing as typ

import astroid
Expand All @@ -26,12 +24,10 @@
from df12_python_lints.constant_chain import ConstantChainChecker
from df12_python_lints.match_dispatch import MatchDispatchChecker
from df12_python_lints.snapshot_asserts import SnapshotAssertionChecker
from df12_python_lints.suppressions import SuppressionCommentChecker
from tests.dataclass_slots_support import module_classes, parse_module

if typ.TYPE_CHECKING:
from pylint.checkers import BaseChecker

# Constructed identifiers: a fixed prefix guarantees the name is never a
# Python keyword, avoiding the filtering trap.
_SUBJECTS = st.from_regex(r"v_[a-z]{1,6}", fullmatch=True)
Expand All @@ -57,15 +53,6 @@ def _walk_symbols(checker_class: type[BaseChecker], code: str) -> list[str]:
return [message.msg_id for message in linter.release_messages()]


def _token_symbols(code: str) -> list[str]:
"""Collect the suppression checker's symbols over *code*."""
linter = testutils.UnittestLinter()
checker = SuppressionCommentChecker(linter)
tokens = list(tokenize.generate_tokens(io.StringIO(code).readline))
checker.process_tokens(tokens)
return [message.msg_id for message in linter.release_messages()]


def _constant_chain(subject: str, constants: list[int]) -> str:
"""Render an if/elif chain comparing *subject* with *constants*."""
branches = [f" if {subject} == {constants[0]}:\n return 0\n"]
Expand Down Expand Up @@ -195,51 +182,6 @@ def test_threshold_is_nesting_invariant(self, leaves: int, split: int) -> None:
assert symbols == expected, "firing must depend only on the total leaf count"


class TestSuppressionProperties:
"""Generated pragmas are classified uniformly."""

@settings(deadline=None)
@given(
codes=st.lists(
st.from_regex(r"[A-Z]{1,3}[0-9]{2,4}", fullmatch=True),
min_size=1,
max_size=3,
)
)
def test_bare_noqa_always_fires(self, codes: list[str]) -> None:
"""A noqa pragma with any code list and no prose is reported."""
code = f"x = 1 # noqa: {', '.join(codes)}\n"
assert _token_symbols(code) == ["lint-suppression-without-explanation"], (
"a bare noqa must be reported whatever its code list"
)

@settings(deadline=None)
@given(
codes=st.lists(
st.from_regex(r"[A-Z][0-9]{3}", fullmatch=True), min_size=1, max_size=3
),
first=_WORDS,
second=_WORDS,
)
def test_prose_always_explains(
self, codes: list[str], first: str, second: str
) -> None:
"""Two-word prose after a second hash explains any pragma."""
code = f"x = 1 # noqa: {', '.join(codes)} # {first} {second}\n"
assert _token_symbols(code) == [], (
"prose after a second hash must count as an explanation"
)

@settings(deadline=None)
@given(names=st.lists(_WORDS, min_size=1, max_size=3))
def test_bare_pylint_disable_always_fires(self, names: list[str]) -> None:
"""A pylint disable pragma with any name list is reported."""
code = f"x = 1 # pylint: disable={','.join(names)}\n"
assert _token_symbols(code) == ["lint-suppression-without-explanation"], (
"a bare pylint disable must be reported whatever its names"
)


class TestPureKernelProperties:
"""The extracted selection kernels honour their contracts."""

Expand Down
Loading
Loading