sourcery-ai / sourcery

Instant AI code reviews
https://sourcery.ai
MIT License
1.54k stars 68 forks source link

Variables only used in convoluted runtime type hints may be removed with for-index-underscore #344

Open JacobHayes opened 1 year ago

JacobHayes commented 1 year ago

Checklist

Description

I was testing out sourcery on https://github.com/artigraph/artigraph and it broke one of the tests (but overall looks quite good!), which included some convoluted dynamic function creation that relies on runtime type hinting.

Code snippet that reproduces issue

Here's a minimal example:

from typing import get_type_hints

def test() -> None:
    for hint in [int]:
        class X:
            def f(self) -> hint:  # type: ignore[valid-type]
                return 0

        print(get_type_hints(X.f))

test()

When run as is, it works as expected:

$ python3 x.py
{'return': <class 'int'>}

Sourcery suggest renaming hint to _ (and misses that hint is referenced in the def f return hint):

$ sourcery review x.py --verbose
Reviewed x.py
──────────────────────────────────────────────── Overview ────────────────────────────────────────────────
Function test refactored with the following changes:

 • Replace unused for index with underscore (for-index-underscore)
  4  def test() -> None:
  5 -    for hint in [int]:
  6 +    for _ in [int]:
  7          class X:
  8              def f(self) -> hint:  # type: ignore[valid-type]
  9                  return 0

──────────────────────────────────────────────── Overview ────────────────────────────────────────────────

 • 1 file scanned.
 • 1 issue detected.

Issues by Type

 • 1 issue could be fixed by Sourcery. Run sourcery review --fix
 • 0 issues need to be fixed manually.

Issues by Rule ID
┏━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━┓
┃ Rule ID              ┃ Count ┃
┡━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━┩
│ for-index-underscore │     1 │
├──────────────────────┼───────┤
│ Total                │     1 │
└──────────────────────┴───────┘

Applying the change results in:

$ python3 x.py
Traceback (most recent call last):
  File "/Users/jacobhayes/src/github.com/artigraph/artigraph/x.py", line 13, in <module>
    test()
  File "/Users/jacobhayes/src/github.com/artigraph/artigraph/x.py", line 6, in test
    class X:
  File "/Users/jacobhayes/src/github.com/artigraph/artigraph/x.py", line 7, in X
    def f(self) -> hint:  # type: ignore[valid-type]
                   ^^^^
NameError: name 'hint' is not defined. Did you mean: 'int'?

Notably, this seems to only happen when the whole thing is inside a function (makes sense - if the loop was top level, nothing guarantees the variable won't be referenced elsewhere) and f is a method - if I remove the wrapping class, it doesn't suggest any changes.

Debug Information

IDE Version: NA

Sourcery Version: 1.2.0

Operating system and Version: macOS 13.3.1

reka commented 1 year ago

Thanks for raising this issue, it's a bug indeed. We'll look into it, but I can't promise a timeline when we'll fix it. As a workaround, my suggestion is to add a comment in such case:

  # sourcery skip: for-index-underscore