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

Ruff has unstable results with target-version (normally hidden by caching) #10267

Closed
aneeshusa opened this issue Mar 7, 2024 · 1 comment · Fixed by #10274
Closed

Ruff has unstable results with target-version (normally hidden by caching) #10267

aneeshusa opened this issue Mar 7, 2024 · 1 comment · Fixed by #10274
Assignees
Labels
bug Something isn't working formatter Related to the formatter

Comments

@aneeshusa
Copy link

aneeshusa commented Mar 7, 2024

I am switching from shed to ruff at $WORK and found what looks like a bug in Ruff's formatter (unstable output);
normally this appears to be hidden by caching, implying a second bug in the caching logic.
Here's a minimized repro with target-version and a particular code pattern
(further minimization ideas I tried broke the repro but some likely exist):

Python 3.11.6 | packaged by conda-forge | (main, Oct  3 2023, 10:37:07) [Clang 15.0.7 ]
ruff 0.3.1
N.B. also tested on {python3.11,python3.12} x {ruff==0.3.0, ruff==0.3.1}

Starting hash of file:
57be245f1161c2069df7c842b935038e30c0a17aa31c924218aa925566203636  wat.py
Starting file contents:
with (
    open(
        "/etc/hosts"  # This is an incredibly long comment that has been replaced for sanitization
    )
):
    pass

When reading from the cache (default behavior), stable after first run:
1 file reformatted
1acf49d050ef0ac27cb93c028af71a4d8d918a91992dbb497c6da52393d91f98  wat.py
with open(
    "/etc/hosts"  # This is an incredibly long comment that has been replaced for sanitization
):
    pass
1 file left unchanged
1acf49d050ef0ac27cb93c028af71a4d8d918a91992dbb497c6da52393d91f98  wat.py
1 file left unchanged
1acf49d050ef0ac27cb93c028af71a4d8d918a91992dbb497c6da52393d91f98  wat.py

If you disable reading from the cache, output flip flops on each run:
1 file reformatted
57be245f1161c2069df7c842b935038e30c0a17aa31c924218aa925566203636  wat.py
1 file reformatted
1acf49d050ef0ac27cb93c028af71a4d8d918a91992dbb497c6da52393d91f98  wat.py
1 file reformatted
57be245f1161c2069df7c842b935038e30c0a17aa31c924218aa925566203636  wat.py

N.B. I have no ruff config settings, and didn't see related issues/PRs but let me know if I missed one!

Script that produced this output
#!/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


cd "${tmp_dir}"
# N.B. does not repro if comment is removed and "/etc/hosts" made very long instead
cat >./wat.py <<EOF
with (
    open(
        "/etc/hosts"  # This is an incredibly long comment that has been replaced for sanitization
    )
):
    pass
EOF

python -m venv ./venv
./venv/bin/pip install --quiet ruff==0.3.1
./venv/bin/python --version --version
./venv/bin/ruff --version
echo 'N.B. also tested on {python3.11,python3.12} x {ruff==0.3.0, ruff==0.3.1}'

echo -e "\nStarting hash of file:"
sha256sum wat.py
echo "Starting file contents:"
cat wat.py

echo -e "\nWhen reading from the cache (default behavior), stable after first run:"
./venv/bin/ruff format --isolated --target-version py310 wat.py
sha256sum wat.py
cat wat.py
./venv/bin/ruff format --isolated --target-version py310 wat.py
sha256sum wat.py
./venv/bin/ruff format --isolated --target-version py310 wat.py
sha256sum wat.py

echo -e "\nIf you disable reading from the cache, output flip flops on each run:"
./venv/bin/ruff format --isolated --target-version py310 --no-cache wat.py
sha256sum wat.py
./venv/bin/ruff format --isolated --target-version py310 --no-cache wat.py
sha256sum wat.py
./venv/bin/ruff format --isolated --target-version py310 --no-cache wat.py
sha256sum wat.py
@aneeshusa aneeshusa changed the title Ruff cache causes unstable results Ruff has unstable results with target-version (normally hidden by caching) Mar 7, 2024
@MichaReiser MichaReiser added bug Something isn't working formatter Related to the formatter labels Mar 7, 2024
@MichaReiser
Copy link
Member

MichaReiser commented Mar 7, 2024

To summarize the report: The problem @aneeshusa is facing is a formatter stability issue when formatting with statements

# Input
with (
    open(
        "/etc/hosts"  # This is an incredibly long comment that has been replaced for sanitization
    )
):
    pass

# Formatted once
with open(
    "/etc/hosts"  # This is an incredibly long comment that has been replaced for sanitization
):
    pass

# Formatted twice
with (
    open(
        "/etc/hosts"  # This is an incredibly long comment that has been replaced for sanitization
    )
):
    pass

Playground

@MichaReiser MichaReiser self-assigned this Mar 7, 2024
charliermarsh pushed a commit that referenced this issue Mar 8, 2024
## Summary

Fixes #10267

The issue with the current formatting is that the formatter flips
between the `SingleParenthesizedContextManager` and
`ParenthesizeIfExpands` or `SingleWithTarget` because the layouts use
incompatible formatting ( `SingleParenthesizedContextManager`:
`maybe_parenthesize_expression(context)` vs `ParenthesizeIfExpands`:
`parenthesize_if_expands(item)`, `SingleWithTarget`:
`optional_parentheses(item)`.

The fix is to ensure that the layouts between which the formatter flips
when adding or removing parentheses are the same. I do this by
introducing a new `SingleWithoutTarget` layout that uses the same
formatting as `SingleParenthesizedContextManager` if it has no target
and prefer `SingleWithoutTarget` over using `ParenthesizeIfExpands` or
`SingleWithTarget`.

## Formatting change

The downside is that we now use `maybe_parenthesize_expression` over
`parenthesize_if_expands` for expressions where
`can_omit_optional_parentheses` returns `false`. This can lead to stable
formatting changes. I only found one formatting change in our ecosystem
check and, unfortunately, this is necessary to fix the instability (and
instability fixes are okay to have as part of minor changes according to
our versioning policy)

The benefit of the change is that `with` items with a single context
manager and without a target are now formatted identically to how the
same expression would be formatted in other clause headers.

## Test Plan

I ran the ecosystem check locally
nkxxll pushed a commit to nkxxll/ruff that referenced this issue Mar 10, 2024
## Summary

Fixes astral-sh#10267

The issue with the current formatting is that the formatter flips
between the `SingleParenthesizedContextManager` and
`ParenthesizeIfExpands` or `SingleWithTarget` because the layouts use
incompatible formatting ( `SingleParenthesizedContextManager`:
`maybe_parenthesize_expression(context)` vs `ParenthesizeIfExpands`:
`parenthesize_if_expands(item)`, `SingleWithTarget`:
`optional_parentheses(item)`.

The fix is to ensure that the layouts between which the formatter flips
when adding or removing parentheses are the same. I do this by
introducing a new `SingleWithoutTarget` layout that uses the same
formatting as `SingleParenthesizedContextManager` if it has no target
and prefer `SingleWithoutTarget` over using `ParenthesizeIfExpands` or
`SingleWithTarget`.

## Formatting change

The downside is that we now use `maybe_parenthesize_expression` over
`parenthesize_if_expands` for expressions where
`can_omit_optional_parentheses` returns `false`. This can lead to stable
formatting changes. I only found one formatting change in our ecosystem
check and, unfortunately, this is necessary to fix the instability (and
instability fixes are okay to have as part of minor changes according to
our versioning policy)

The benefit of the change is that `with` items with a single context
manager and without a target are now formatted identically to how the
same expression would be formatted in other clause headers.

## Test Plan

I ran the ecosystem check locally
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.

2 participants