Skip to content

[ruff] add fix safety section (RUF017) #17480

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

Merged

Conversation

VascoSch92
Copy link
Contributor

@VascoSch92 VascoSch92 commented Apr 19, 2025

The PR add the fix safety section for rule RUF017 (#15584 )

Copy link
Contributor

github-actions bot commented Apr 19, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@VascoSch92 VascoSch92 changed the title [ruff] add fix safety section (RUFF017) [ruff] add fix safety section (RUF017) Apr 19, 2025
@AlexWaygood AlexWaygood added the documentation Improvements or additions to documentation label Apr 22, 2025
@dylwil3 dylwil3 self-assigned this Apr 22, 2025
@dylwil3
Copy link
Collaborator

dylwil3 commented Apr 26, 2025

Thanks for this!

Looking at the implementation for this rule, I notice that it is only emitted when the user explicitly provides the start argument for sum as an empty list (either list() or []). The expression sum(lists,[]) would raise a type error if the iterable lists had any non-list items, and the user has no control over the implementation of __add__ or __iadd__ for the primitive list.

However, there is a small difference in implementation between __add__ and __iadd__ for list: the former requires the right summand to be a list, whereas the latter allows for any iterable.

So, unless I'm misunderstanding, I think the only thing the fix could do that would change behavior is turn code that used to raise an exception into code that no longer raises an exception. Namely:

import functools
import operator

ranges = [range(2), range(3)]

# raises a TypeError
sum(ranges, []) 

# evaluates to [0, 1, 0, 1, 2]
functools.reduce(operator.iadd, ranges, [])

So maybe we can reword the fix safety section in light of this?

@VascoSch92
Copy link
Contributor Author

Hey @dylwil3
thanks for the explanation. It is really subtile the case here, and pretty interesting.

Perhaps something on this direction?

The fix is always marked as unsafe because `sum` uses the `__add__` magic method while
`operator.iadd` uses the `__iadd__` magic method, and these behaves different on lists, i.e., 
the former requires the right summand to be a list, whereas the latter allows for any iterable. 
Therefore, the fix could inadvertently cause code that previously raised an error to silently 
succeed with different results. Moreover, the fix could remove comments from the original code.

Another question: what is the politic with deleted comments? Should this be mentioned in the fix safety section? I think yes but not sure.

Copy link
Collaborator

@dylwil3 dylwil3 left a comment

Choose a reason for hiding this comment

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

Thank you! Your wording was good - I added it in with some minor corrections.

@dylwil3 dylwil3 enabled auto-merge (squash) April 28, 2025 22:05
@dylwil3 dylwil3 merged commit 5096824 into astral-sh:main Apr 28, 2025
32 checks passed
dcreager added a commit that referenced this pull request Apr 29, 2025
* main:
  [`ruff`] add fix safety section (`RUF017`) (#17480)
  Add Python 3.14 to configuration options (#17647)
  [`airflow`] Apply auto fixes to cases where the names have changed in Airflow 3 (`AIR302`) (#17553)
@VascoSch92 VascoSch92 mentioned this pull request Apr 29, 2025
71 tasks
@VascoSch92 VascoSch92 deleted the document-fix-safety-quadratic-list-summation branch May 1, 2025 06:03
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

Successfully merging this pull request may close these issues.

3 participants