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

Autofix SIM102 (NestedIfStatements) #1657

Merged
merged 4 commits into from
Jan 18, 2023

Conversation

andersk
Copy link
Contributor

@andersk andersk commented Jan 5, 2023

I tried to write an autofixer for SIM102 (NestedIfStatements) using LibCST to preserve comments. But I ran into two issues that potentially point to general problems with the with the SourceCodeLocator API and/or the Fix::replacement API. So I’m opening this draft to discuss what the plan should be.


Firstly, if the input if statement is indented, the output if body will be misindented:

 while 1:
     while 2:
-        if 3:
-            if 4:
-                5
+        if 3 and 4:
+    5

In this case, SourceCodeLocator::slice_source_code_range fails to strip the indentation from the second and following lines:

if 3:
            if 4:
                5

and Fix::replacement fails to add that indentation.


Secondly, if the input if statement is actually an elif, the fix fails:

while 1:
    while 2:
        if 3:
            4
        elif 5:
            if 6:
                7

In this case, SourceCodeLocator::slice_source_code_range returns this invalid Python that LibCST can’t parse:

elif 5:
            if 6:
                7

@andersk andersk force-pushed the autofix-sim102 branch 2 times, most recently from 3a955f1 to 5d3b2f2 Compare January 5, 2023 09:22
@charliermarsh
Copy link
Member

The latter issue came up in a slightly different form here: #1694.

@charliermarsh
Copy link
Member

@andersk - Ok if I poke around at this branch? Gonna see if I can come up with some helpers for the issues you're describing.

@charliermarsh
Copy link
Member

@andersk - Ok, I think this works? We do skip a few cases: (1) if there are any comments between the two if statements, those get destroyed in the fix, so we avoid applying the patch in that case (we do preserve comments in the nested if body, so that's fine); and (2) if the resulting statement is too long, we avoid applying it.

@charliermarsh charliermarsh marked this pull request as ready for review January 17, 2023 21:51
@charliermarsh
Copy link
Member

Let me know if you have feedback, and how you feel about merging this change.

ruff_cli/src/diagnostics.rs Outdated Show resolved Hide resolved
src/rules/flake8_simplify/rules/fix_if.rs Outdated Show resolved Hide resolved
bail!("Unable to fix multiline statements");
}

let contents = textwrap::dedent(&contents);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has various failure modes.

while True:
    if True:
        if True:
            """this
is valid"""

            """the indentation on
            this line is significant"""

            "this is" \
"allowed too"

            ("so is"
"this for some reason")

Copy link
Member

Choose a reason for hiding this comment

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

Right, ok, good call. I believe I can fix this by leveraging LibCST's indentation field.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, I think the new version works, but it requires a bunch of trickery.

If the statement is indented, we embed it in a dummy function, like:

def f():
  if ...

Then, at the end, we stripe the function. I think this is the most robust way to preserve indentation. It lets us avoid having to do any indentation or dedentation ourselves (but keeps the source code valid for LibCST).

Separately, we also have to handle elif blocks via special-case.

andersk and others added 3 commits January 18, 2023 02:07
Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@andersk andersk force-pushed the autofix-sim102 branch 2 times, most recently from 51f04bd to 6f55264 Compare January 18, 2023 07:18
Co-authored-by: Charlie Marsh <charlie.r.marsh@gmail.com>
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@andersk
Copy link
Contributor Author

andersk commented Jan 18, 2023

Yeah, that gets the job done I guess.

I cleaned this up a bit more and split out the helper function changes into a series of logical commits suitable for merging with Rebase and merge.

@andersk andersk changed the title Broken autofixer for SIM102 (NestedIfStatements) Autofix SIM102 (NestedIfStatements) Jan 18, 2023
@charliermarsh charliermarsh merged commit 83346de into astral-sh:main Jan 18, 2023
@andersk andersk deleted the autofix-sim102 branch January 18, 2023 14:08
renovate bot referenced this pull request in ixm-one/pytest-cmake-presets Jan 19, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [ruff](https://togithub.com/charliermarsh/ruff) | `^0.0.225` ->
`^0.0.226` |
[![age](https://badges.renovateapi.com/packages/pypi/ruff/0.0.226/age-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://badges.renovateapi.com/packages/pypi/ruff/0.0.226/adoption-slim)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://badges.renovateapi.com/packages/pypi/ruff/0.0.226/compatibility-slim/0.0.225)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://badges.renovateapi.com/packages/pypi/ruff/0.0.226/confidence-slim/0.0.225)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>charliermarsh/ruff</summary>

###
[`v0.0.226`](https://togithub.com/charliermarsh/ruff/releases/tag/v0.0.226)

[Compare
Source](https://togithub.com/charliermarsh/ruff/compare/v0.0.225...v0.0.226)

#### What's Changed

- \[`isort`] Add `constants` and `variables` Options by
[@&#8203;saadmk11](https://togithub.com/saadmk11) in
[https://github.com/charliermarsh/ruff/pull/1951](https://togithub.com/charliermarsh/ruff/pull/1951)
- Fix bad link for flake8-no-pep420 by
[@&#8203;skykasko](https://togithub.com/skykasko) in
[https://github.com/charliermarsh/ruff/pull/1952](https://togithub.com/charliermarsh/ruff/pull/1952)
- Autofix SIM102 (NestedIfStatements) by
[@&#8203;andersk](https://togithub.com/andersk) in
[https://github.com/charliermarsh/ruff/pull/1657](https://togithub.com/charliermarsh/ruff/pull/1657)
- Confine type-of-primitive checks to builtin type calls by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1962](https://togithub.com/charliermarsh/ruff/pull/1962)
- Autofix SIM117 (MultipleWithStatements) by
[@&#8203;andersk](https://togithub.com/andersk) in
[https://github.com/charliermarsh/ruff/pull/1961](https://togithub.com/charliermarsh/ruff/pull/1961)
- \[`isort`] Add `no-lines-before` Option by
[@&#8203;saadmk11](https://togithub.com/saadmk11) in
[https://github.com/charliermarsh/ruff/pull/1955](https://togithub.com/charliermarsh/ruff/pull/1955)
- Use `smallvec` for call path representation by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1960](https://togithub.com/charliermarsh/ruff/pull/1960)
- Treat subscript accesses as unsafe effects for autofix by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1966](https://togithub.com/charliermarsh/ruff/pull/1966)
- Strip whitespace when injecting D209 newline by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1967](https://togithub.com/charliermarsh/ruff/pull/1967)
- README: Link “Flake8” for consistency with the rest of the list by
[@&#8203;andersk](https://togithub.com/andersk) in
[https://github.com/charliermarsh/ruff/pull/1969](https://togithub.com/charliermarsh/ruff/pull/1969)
- Run cargo fmt in pre-commit by [@&#8203;akx](https://togithub.com/akx)
in
[https://github.com/charliermarsh/ruff/pull/1968](https://togithub.com/charliermarsh/ruff/pull/1968)
- Convert remaining call path sites to use `SmallVec` by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1972](https://togithub.com/charliermarsh/ruff/pull/1972)
- Remove artificial wraps from GitHub messages by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1977](https://togithub.com/charliermarsh/ruff/pull/1977)
- Invert order of yoda-conditions message by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1979](https://togithub.com/charliermarsh/ruff/pull/1979)
- Replace misplaced-comparison-constant with SIM300 by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1980](https://togithub.com/charliermarsh/ruff/pull/1980)
- Use relative paths for INP001 by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1981](https://togithub.com/charliermarsh/ruff/pull/1981)
- Avoid removing side effects for boolean simplifications by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1984](https://togithub.com/charliermarsh/ruff/pull/1984)
- Enable suppression of magic values by type by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1987](https://togithub.com/charliermarsh/ruff/pull/1987)
- Exclude None, Bool, and Ellipsis from ConstantType by
[@&#8203;charliermarsh](https://togithub.com/charliermarsh) in
[https://github.com/charliermarsh/ruff/pull/1988](https://togithub.com/charliermarsh/ruff/pull/1988)

#### New Contributors

- [@&#8203;skykasko](https://togithub.com/skykasko) made their first
contribution in
[https://github.com/charliermarsh/ruff/pull/1952](https://togithub.com/charliermarsh/ruff/pull/1952)
- [@&#8203;akx](https://togithub.com/akx) made their first contribution
in
[https://github.com/charliermarsh/ruff/pull/1968](https://togithub.com/charliermarsh/ruff/pull/1968)

**Full Changelog**:
astral-sh/ruff@v0.0.225...v0.0.226

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://app.renovatebot.com/dashboard#github/ixm-one/pytest-cmake-presets).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMDUuNCIsInVwZGF0ZWRJblZlciI6IjM0LjEwNS40In0=-->

Signed-off-by: Renovate Bot <bot@renovateapp.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants