Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

formatter: Two iterations required to converge on specific comment + slice case #10355

Closed
aneeshusa opened this issue Mar 12, 2024 · 2 comments · Fixed by #10492
Closed

formatter: Two iterations required to converge on specific comment + slice case #10355

aneeshusa opened this issue Mar 12, 2024 · 2 comments · Fixed by #10492
Assignees
Labels
bug Something isn't working formatter Related to the formatter

Comments

@aneeshusa
Copy link

Tested on ruff v0.3.2 and main (dacec73). No ruff config.
Minimized from a file at $WORK; this cropped up during one of my final rebases to switch to ruff.
Not a blocker since the final formatting is stable.
black 24.2.0 accepts both the original and final style;
it rewrites the intermediate style to the final style.

Input:

repro(
    "some long string that takes up some space"
)[  # some long comment also taking up space
    0
]

1st run of ruff gives:

repro(
    "some long string that takes up some space"
)[0]  # some long comment also taking up space

2nd and beyond runs give:

repro("some long string that takes up some space")[
    0
]  # some long comment also taking up space
Script used
#!/usr/bin/env bash

# no errexit
set -o nounset
set -o pipefail

tmp_dir="$(mktemp -d)"
cleanup() {
    rm -rf "${tmp_dir}"
}
trap cleanup EXIT INT QUIT TERM

cat >"${tmp_dir}/repro.py" <<EOF
repro(
    "some long string that takes up some space"
)[  # some long comment also taking up space
    0
]
EOF

cd ~/code/ruff
git checkout "${1}"
ruff=(cargo run --quiet --package=ruff --)

echo -e '\nStarting hash of file'
(cd "${tmp_dir}" && sha256sum "./repro.py")
cat "${tmp_dir}/repro.py"

echo -e '\nFirst run'
"${ruff[@]}" format --isolated --target-version py310 --no-cache "${tmp_dir}/repro.py"
(cd "${tmp_dir}" && sha256sum "./repro.py")
cat "${tmp_dir}/repro.py"

echo -e '\nSecond run'
"${ruff[@]}" format --isolated --target-version py310 --no-cache "${tmp_dir}/repro.py"
(cd "${tmp_dir}" && sha256sum "./repro.py")
cat "${tmp_dir}/repro.py"

echo -e '\nThird run'
"${ruff[@]}" format --isolated --target-version py310 --no-cache "${tmp_dir}/repro.py"
(cd "${tmp_dir}" && sha256sum "./repro.py")

Didn't see any specific issues for this non-single-step-convergence, let me know if I missed one.

@AlexWaygood AlexWaygood added bug Something isn't working formatter Related to the formatter labels Mar 12, 2024
@charliermarsh
Copy link
Member

Thanks!

@MichaReiser
Copy link
Member

Thanks for the excellent write up. It made it super easy to reproduce and fix (including test cases)

MichaReiser added a commit that referenced this issue Mar 20, 2024
…10492)

## Summary

This PR fixes an instability where formatting a subscribt 
where the `slice` is not an `ExprSlice` and it has a trailing
end-of-line comment after its opening `[` required two formatting passes
to be stable.

The fix is to associate the trailing end-of-line comment as dangling
comment on `[` to preserve its position, similar to how Ruff does it for
other parenthesized expressions.
This also matches how trailing end-of-line subscript comments are
handled when the `slice` is an `ExprSlice`.

Fixes #10355

## Versioning

Shipping this as part of a patch release is fine because:

* It fixes a stability issue
* It doesn't impact already formatted code because Ruff would already
have moved to the comment to the end of the line (instead of preserving
it)

## Test Plan

Added tests
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 a pull request may close this issue.

4 participants