Skip to content

Conversation

@dylwil3
Copy link
Collaborator

@dylwil3 dylwil3 commented Dec 20, 2025

This PR adjusts the logic for skipping formatting so that a fmt: skip can affect multiple statements if they lie on the same line.

Specifically, a fmt: skip comment will now suppress all the statements in the suite in which it appears whose range intersects the line containing the skip directive. For example:

x=[
'1'
];x=2 # fmt: skip

remains unchanged after formatting.

(Note that compound statements are somewhat special and were handled in a previous PR - see #20633).

Closes #17331 and #11430.

Simplest to review commit by commit - the key diffs of interest are the commit introducing the core logic, and the diff between the snapshots introduced in the last commit (compared to the second commit).

Implementation

On main we format a suite of statements by iterating through them. If we meet a statement with a leading or trailing (own-line)fmt: off comment, then we suppress formatting until we meet a fmt: on comment. Otherwise we format the statement using its own formatting rule.

How are fmt: skip comments handled then? They are handled internally to the formatting of each statement. Specifically, calling .fmt on a statement node will first check to see if there is a trailing, end-of-line fmt: skip (or fmt: off/yapf: off), and if so then write the node with suppressed formatting.

In this PR we move the responsibility for handling fmt: skip into the formatting logic of the suite itself. This is done as follows:

  • Before beginning to format the suite, we do a pass through the statements and collect the data of ranges with skipped formatting. More specifically, we create a map with key given by the first skipped statement in a block and value a pair consisting of the last skipped statement and the range to write verbatim.
  • We iterate as before, but if we meet a statement that is a key in the map constructed above, we pause to write the associated range verbatim. We then advance the iterator to the last statement in the block and proceed as before.

Addendum on range formatting

We also had to make some changes to range formatting in order to support this new behavior. For example, we want to make sure that

<RANGE_START>x=1<RANGE_END>;x=2 # fmt: skip

formats verbatim, rather than becoming

x = 1;x=2 # fmt: skip

Recall that range formatting proceeds in two steps:

  1. Find the smallest enclosing node containing the range AND that has enough info to format the range (so it may be larger than you think, e.g. a docstring has enclosing node given by the suite, not the string itself.)
  2. Carve out the formatted range from the result of formatting that enclosing node.

We had to modify (1), since the suite knows how to format skipped nodes, but nodes may not "know" they are skipped. To do this we altered the visit_body bethod of the FindEnclosingNode visitor: now we iterate through the statements and check for skipped ranges intersecting the format range. If we find them, we return without descending. The result is to consider the statement containing the suite as the enclosing node in this case.

@dylwil3 dylwil3 added bug Something isn't working formatter Related to the formatter labels Dec 20, 2025
@astral-sh-bot
Copy link

astral-sh-bot bot commented Dec 20, 2025

ruff-ecosystem results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

@dylwil3 dylwil3 marked this pull request as ready for review December 22, 2025 16:19
@dylwil3 dylwil3 requested a review from MichaReiser as a code owner December 22, 2025 16:19
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this long standing issues.

I haven't done an in-depth review but I left some initial questions. We can also use our 1:1 to discuss some of them

@MichaReiser
Copy link
Member

Regarding the benchmark. I don't think using hyperfine will give you meaningful numbers. There's just too much overhead in ruff's cli (config discovery, directory walking, io) in addition to the noise of measuring a cli tool.

If you want to get meaningful numbers, I suggest adding a benchmark to ruff_benchmark. Even if it's just locally

@dylwil3 dylwil3 force-pushed the fmt-skip-suite branch 2 times, most recently from 2009b0f to f920e49 Compare December 26, 2025 20:20
@dylwil3 dylwil3 marked this pull request as draft December 26, 2025 21:34
@dylwil3 dylwil3 force-pushed the fmt-skip-suite branch 2 times, most recently from 446a376 to 09f82dd Compare January 5, 2026 16:35
@dylwil3 dylwil3 changed the title Respect fmt: skip for multiple statements on same line Respect fmt: skip for multiple statements on same logical line Jan 5, 2026
@dylwil3 dylwil3 mentioned this pull request Jan 5, 2026
@dylwil3 dylwil3 marked this pull request as ready for review January 5, 2026 22:03
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

Nice, this looks pretty simple now.

Do we still need the is_suppressed logic in the statement formatting?

Comment on lines 1001 to 1004
SimpleTokenKind::Continuation => {
tokenizer.next();
}
SimpleTokenKind::Newline => {
return true;
}
_ => {}
Copy link
Member

Choose a reason for hiding this comment

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

I always feel a bit uneasy about tokenizer calls where we have a blanket match arm that eats over any tokens.

I would write this as

    while let Some(token) = tokenizer.next() {
        match token {
            SimpleTokenKind::Continuation => {
                // Skip over the newline
                tokenizer.next();
            }
            SimpleTokenKind::Newline => {
                return true;
            }
            kind => {
                if !kind.is_trivia() {
                    return false;
                }
            }
        }
    }

We could even consider making the kind.is_trivia a debug assertion

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add this but observe that, since we are specifically in a range between statements, I don't think any non-trivia tokens can actually occur.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ended up keeping this as is but adding a comment with the justification I gave above - let me know if you feel strongly and I'll commit your suggestion!

dylwil3 added a commit that referenced this pull request Jan 6, 2026
I am updating these because we didn't have test coverage for the
different handling of `fmt: skip` comments applied to multiple
statements on the same line. This is in preparation for #22119 (to show
before/after deviations).

Follows the same procedure as in #20794

Edit: As it happens, the new fixtures do not even cover the case
relevant to #22119 - they just deal with the already handled case of a
one-line compound statement. Nevertheless, it seems worthwhile to make
this update, especially since it uncovered a (possible?) bug.
@dylwil3
Copy link
Collaborator Author

dylwil3 commented Jan 9, 2026

While fixing the behavior for range formatting, I noticed that, unrelated to any suppression comments, we do the following:

x=1;<RANGE_START>x=2<RANGE_END>

becomes:

x=1;    x = 2

Is this expected or should I make an issue for it?

The enclosing node of the range here is correctly determined to be the full suite (because it can't find the indentation for x=2), but then something must go wrong when narrowing from the formatted

x=1
x=2

back down. My guess is that this comes down to the is_logical_line helper being not quite right, or else maybe we aren't emitting/interpreting source positions correctly here? I haven't looked closely.

@dylwil3 dylwil3 requested a review from MichaReiser January 9, 2026 21:35
@dylwil3
Copy link
Collaborator Author

dylwil3 commented Jan 9, 2026

As you predicted, removing is_suppressed got rid of what little perf regression was left: https://codspeed.io/astral-sh/ruff/runs/compare/69615827861fdc44a0a9aaea..6961718d1af8d4524db090e5

(except for a teensy one on numpy/globals.py)

@MichaReiser
Copy link
Member

MichaReiser commented Jan 10, 2026

Is this expected or should I make an issue for it?

Yeah, this looks very wrong. Opening an issue for it would be great

@dylwil3 dylwil3 enabled auto-merge (squash) January 10, 2026 14:56
@dylwil3 dylwil3 merged commit 880513a into astral-sh:main Jan 10, 2026
40 checks passed
@madduck
Copy link

madduck commented Jan 11, 2026

Thanks a lot for your work on this @dylwil3, and everyone who was also involved! ♥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working formatter Related to the formatter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ruff formatter ignoring fmt: skip

3 participants