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

[refurb] Implement readlines_in_for lint (FURB129) #9880

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

alex-700
Copy link
Contributor

@alex-700 alex-700 commented Feb 7, 2024

Summary

Implement implicit readlines (FURB129) lint.

Notes

I need a help/an opinion about suggested implementations.

This implementation differs from the original one from refurb in the following way. This implementation checks syntactically the call of the method with the name readlines() inside for {loop|generator expression}. The implementation from refurb also checks that callee is a variable with a type io.TextIOWrapper or io.BufferedReader.

  • I do not see a simple way to implement the same logic.
  • The best I can have is something like
checker.semantic().binding(checker.semantic().resolve_name(attr_expr.value.as_name_expr()?)?).statement(checker.semantic())

and analyze cases. But this will be not about types, but about guessing the type by assignment (or with) expression.

  • Also this logic has several false negatives, when the callee is not a variable, but the result of function call (e.g. open(...)).
  • On the other side, maybe it is good to lint this on other things, where this suggestion is not safe, and push the developers to change their interfaces to be less surprising, comparing with the standard library.
  • Anyway while the current implementation has false-positives (I mentioned some of them in the test) I marked the fixes to be unsafe.

Test Plan

cargo test

Copy link
Contributor

github-actions bot commented Feb 7, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+7 -0 violations, +0 -0 fixes in 3 projects; 40 projects unchanged)

apache/airflow (+5 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ dev/breeze/src/airflow_breeze/global_constants.py:361:21: FURB129 Instead of calling `readlines()`, iterate over file object directly
+ dev/perf/sql_queries.py:143:41: FURB129 Instead of calling `readlines()`, iterate over file object directly
+ docs/exts/redirects.py:43:21: FURB129 Instead of calling `readlines()`, iterate over file object directly
+ scripts/ci/pre_commit/pre_commit_newsfragments.py:35:43: FURB129 Instead of calling `readlines()`, iterate over file object directly
+ tests/system/providers/google/cloud/gcs/resources/transform_script.py:26:39: FURB129 Instead of calling `readlines()`, iterate over file object directly

zulip/zulip (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ tools/lib/provision_inner.py:101:51: FURB129 Instead of calling `readlines()`, iterate over file object directly

indico/indico (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

+ setup.py:18:30: FURB129 Instead of calling `readlines()`, iterate over file object directly

Changes by rule (1 rules affected)

code total + violation - violation + fix - fix
FURB129 7 7 0 0 0

@charliermarsh charliermarsh self-requested a review February 13, 2024 02:15
@charliermarsh charliermarsh added the rule Implementing or modifying a lint rule label Feb 13, 2024
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Thanks! I added some extensions to our (basic) type inference system to support this.

@charliermarsh charliermarsh merged commit dd0ba16 into astral-sh:main Feb 13, 2024
17 checks passed
@alex-700 alex-700 deleted the latyshev/furb129 branch February 13, 2024 09:34
nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
## Summary
Implement [implicit readlines
(FURB129)](https://github.com/dosisod/refurb/blob/master/refurb/checks/iterable/implicit_readlines.py)
lint.

## Notes
I need a help/an opinion about suggested implementations.

This implementation differs from the original one from `refurb` in the
following way. This implementation checks syntactically the call of the
method with the name `readlines()` inside `for` {loop|generator
expression}. The implementation from refurb also
[checks](https://github.com/dosisod/refurb/blob/master/refurb/checks/iterable/implicit_readlines.py#L43)
that callee is a variable with a type `io.TextIOWrapper` or
`io.BufferedReader`.

- I do not see a simple way to implement the same logic.
- The best I can have is something like
```rust
checker.semantic().binding(checker.semantic().resolve_name(attr_expr.value.as_name_expr()?)?).statement(checker.semantic())
```
and analyze cases. But this will be not about types, but about guessing
the type by assignment (or with) expression.
- Also this logic has several false negatives, when the callee is not a
variable, but the result of function call (e.g. `open(...)`).
- On the other side, maybe it is good to lint this on other things,
where this suggestion is not safe, and push the developers to change
their interfaces to be less surprising, comparing with the standard
library.
- Anyway while the current implementation has false-positives (I
mentioned some of them in the test) I marked the fixes to be unsafe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants