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

FURB148 generates invalid fix when arg is a generator #7656

Closed
Skylion007 opened this issue Sep 25, 2023 · 6 comments · Fixed by #7781
Closed

FURB148 generates invalid fix when arg is a generator #7656

Skylion007 opened this issue Sep 25, 2023 · 6 comments · Fixed by #7781
Assignees
Labels
bug Something isn't working type-inference Requires more advanced type inference.

Comments

@Skylion007
Copy link

Skylion007 commented Sep 25, 2023

a = (b for b in range(1, 100))
for i, _ in enumerate(a):
    print(i)

gets autofixed to:

a = (b for b in range(1, 100))
for i in range(len(a)):
    print(i)

but a is a generator, and len, surprisingly cannot take a generator (len needs a SizedSequence). Running the code in this autofix will generate the following error:

Traceback (most recent call last):
  File "test_file.py", line 2, in <module>
    for i in range(len(a)):
TypeError: object of type 'generator' has no len()

So while this would work if object was a SizedSequence like a tuple, list, dict, or set, it does not work in this case and will generate invalid.

As an aside, using enumerate to generate an index here when it's a generator is rather nifty, not sure there is an easier way to actually get a range length on generator, using enumerate might actually be most efficient in this case.

The command run is ruff test_file.py --select FURB148 --preview --fix
on ruff 0.0.291

@charliermarsh
Copy link
Member

Thanks!

@dhruvmanila
Copy link
Member

This will probably require type inference as the variable might be defined outside the enumerate call site as is in this example.

@dhruvmanila dhruvmanila added the type-inference Requires more advanced type inference. label Sep 26, 2023
@Skylion007
Copy link
Author

@dhruvmanila True, but in the meantime we shouldn't suggest or force the change because if the arg is a generator, enumerate might strictly be better than range (I can't think of a cleaner way to do it).

@tjkuson
Copy link
Contributor

tjkuson commented Oct 2, 2023

Suppressing the fix would still have the rule trigger, which is less destructive but still frustrating for the user who has to figure out it's a false negative and suppress the rule. Limiting the rule to known safe sequences (for example, using ruff_python_semantic::analyze::typing::is_list) would eliminate this, but result in a whole load of false negatives. We could do that to eventually promote the rule out of the preview category, and then create an issue for generator discrimination when Ruff has better type inference?

@charliermarsh
Copy link
Member

Yeah, I think my suggestion would be to only trigger the enumerate case if is_list or is_set or is_dict or is_tuple (i.e., it's a known container) is truthy.

@tjkuson
Copy link
Contributor

tjkuson commented Oct 2, 2023

Feel free to assign to me, I can work on a merge request tomorrow.

charliermarsh pushed a commit that referenced this issue Oct 3, 2023
…48`) `len` suggestion (#7781)

## Summary

Check that the sequence type is a list, set, dict, or tuple before
recommending replacing the `enumerate(...)` call with `range(len(...))`.
Document behaviour so users are aware of the type inference limitation
leading to false negatives.

Closes #7656.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working type-inference Requires more advanced type inference.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants