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

Better documentation of unsafe fixes #8482

Closed
hauntsaninja opened this issue Nov 3, 2023 · 12 comments
Closed

Better documentation of unsafe fixes #8482

hauntsaninja opened this issue Nov 3, 2023 · 12 comments
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@hauntsaninja
Copy link
Contributor

hauntsaninja commented Nov 3, 2023

It would be nice if it was easier to figure out what checks have unsafe fixes, e.g. maybe there could be a different icon next to the rules in https://docs.astral.sh/ruff/rules/

It would also be nice if the per-rule documentation also mentioned why the fix is unsafe. E.g. for https://docs.astral.sh/ruff/rules/unnecessary-comprehension/ I'm left guessing, is it because ruff will apply the fix even when dict is bound to something else (edit: maybe because cases where we unpack we now lose errors)?

(For some user context: on a lot of our code, we only run ruff for autofixes and do not report errors for unfixable lints. Autofixing is a killer feature)

@charliermarsh
Copy link
Member

Yeah that's a nice idea. We've started doing this in some places via a "Known problems" section. I'll try to get the remaining rules covered next week.

(If helpful, you can also set unsafe-fixes = true in your configuration file to get the same behavior as before we shipped the applicability settings -- but I assume you're asking because you want to know which rules you should be fixing vs. not :))

@charliermarsh charliermarsh added the documentation Improvements or additions to documentation label Nov 3, 2023
@charliermarsh charliermarsh self-assigned this Nov 3, 2023
@Avasam
Copy link

Avasam commented Nov 4, 2023

There's some overlap in the ideas from my comment in #8075 (comment) so I'll copy here as well:

On the rule page itself, I think you could have:
"Fix is always available" / "Fix is sometimes available" / (no fix available)
and separately mention that "Some fixes are marked unsafe by default"

I don't think it's necessary to mention that "All fixes are unsafe". But at a user I wouldn't mind being a touch more exact. As long as it is show that I need to run --unsafe-fixes to get all the available fixes.


As for the "all rules" page.
You could differentiate between "sometimes autofixable" and "always autofixable" with 🔨 or 🔧, it's visually just 🛠️ but with a tool missing. I personally quite like that.

As fox fix safety, I'm less certain of the best way to display that / convey the information concisely. I think that adding a column should be fine.
The first thing that comes to mind is ⚠️, but it looks out of place/scary even when grayed out:
image
Other suggestions may include ⁉, 🚦, or just annotating the autofix with *, or even get fancy with it with a set-width column (or a 0-width element translated to the right position):
image

@zanieb
Copy link
Member

zanieb commented Nov 28, 2023

Please let us know if there's a rule with an unsafe fix that is not explained in the documentation, we can start tracking adding documentation at least.

@hauntsaninja
Copy link
Contributor Author

Rule with unsafe fix that caused me to open this issue was C416. I'll post on here as I notice others :-)

@sh-at-cs
Copy link

Why the fix is unsafe is not explained for UP007, as mentioned in #8868.

@charliermarsh
Copy link
Member

(Adding docs for those rules now.)

@charliermarsh
Copy link
Member

Done in #8918.

@hauntsaninja
Copy link
Contributor Author

hauntsaninja commented Dec 17, 2023

A few more unsafe fixes that could use docs:

  • UP028
  • PERF102
  • PT014
  • PYI025
  • PLR6201

@charliermarsh
Copy link
Member

I think UP028 might be safe except in very rare circumstances. I'm trying to come up with an example in which the modification could lead to a breaking change.

@notatallshaw-gts
Copy link

notatallshaw-gts commented Jan 2, 2024

I think UP028 might be safe except in very rare circumstances. I'm trying to come up with an example in which the modification could lead to a breaking change.

Sure, when the generator is being sent values (and discarding them) but it's yielding values from an iterator it is an unsafe fix.

E.g. this code works fine:

def foo():
    for x in range(10):
        yield x

gen_foo = foo()
next(gen_foo)
gen_foo.send(10)

But UP028 fixes is to this:

def foo():
    yield from range(10)

gen_foo = foo()
next(gen_foo)
gen_foo.send(10)

Which throws the exception:

Traceback (most recent call last):
  File "yield_example.py", line 7, in <module>
    gen_foo.send(10)
  File "yield_example.py", line 2, in foo
    yield from range(10)
AttributeError: 'range_iterator' object has no attribute 'send'

Now, there probably should be some rule about sending to a generator that doesn't read the values. But I'm sure someone has some code somewhere that does this.

@charliermarsh
Copy link
Member

@notatallshaw-gts - thank you!

@charliermarsh
Copy link
Member

Okay, all of the enumerated rules in the issue have either been documented or are being prompted to safe in v0.2.0. Going forward, I'm also ensuring that all unsafe fixes are included in the rule documentation for new fixes and rules.

charliermarsh added a commit that referenced this issue Jan 29, 2024
charliermarsh added a commit that referenced this issue Jan 29, 2024
## Summary

This seems safe to me. See
#8482 (comment).

## Test Plan

`cargo test`
charliermarsh added a commit that referenced this issue Jan 29, 2024
#9679)

## Summary

Prompted by
#8482 (comment).
The rename is only unsafe when the symbol is exported, so we can narrow
the conditions.
zanieb pushed a commit that referenced this issue Jan 29, 2024
zanieb pushed a commit that referenced this issue Jan 29, 2024
## Summary

This seems safe to me. See
#8482 (comment).

## Test Plan

`cargo test`
zanieb pushed a commit that referenced this issue Jan 29, 2024
#9679)

## Summary

Prompted by
#8482 (comment).
The rename is only unsafe when the symbol is exported, so we can narrow
the conditions.
zanieb pushed a commit that referenced this issue Jan 30, 2024
#9679)

## Summary

Prompted by
#8482 (comment).
The rename is only unsafe when the symbol is exported, so we can narrow
the conditions.
zanieb pushed a commit that referenced this issue Feb 1, 2024
#9679)

## Summary

Prompted by
#8482 (comment).
The rename is only unsafe when the symbol is exported, so we can narrow
the conditions.
zanieb pushed a commit that referenced this issue Feb 1, 2024
#9679)

## Summary

Prompted by
#8482 (comment).
The rename is only unsafe when the symbol is exported, so we can narrow
the conditions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

6 participants