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

False positive due to distinction between used_attrs and used_names #221

Closed
jingw opened this issue Aug 8, 2020 · 2 comments
Closed

False positive due to distinction between used_attrs and used_names #221

jingw opened this issue Aug 8, 2020 · 2 comments

Comments

@jingw
Copy link

jingw commented Aug 8, 2020

Consider the following code in two files:

# module.py
some_module_var = 1
print(some_module_var)

# module_user.py
import module
module.some_module_var = 2

Running vulture complains that module.some_module_var is an unused attribute

$ vulture module.py module_user.py 
module_user.py:3: unused attribute 'some_module_var' (60% confidence)

How do you feel about this? On one hand, in practice, the cases where I've seen this false positive were test cases that should have been refactored to use mock.patch instead of directly setting module attributes. On the other hand, it's still a legitimate false positive, and someone could conceivably write non-test code like this.

I imagine other classes of false positives may be caused by separating attrs and names. One example that comes to mind is

class Foobar:
    def my_method(self):
        pass

    deprecated_name_for_my_method = my_method

Foobar().deprecated_name_for_my_method()
func.py:2: unused method 'my_method' (60% confidence)

Fixing this can be done by combining used_attrs with used_names in unused_attrs, though that largely defeats the point of separating these. I suppose it's not crazy to collapse them into a single used_names though.

@jendrikseipp
Copy link
Owner

Good point! I agree that folding used_attrs into used_names makes sense (i.e., storing all used attributes in used_names and dropping the used_attrs set and the get_set function). Do you want to create a pull request for this?

@jingw
Copy link
Author

jingw commented Aug 9, 2020

Yep, I can put together that change.

jingw pushed a commit to jingw/vulture that referenced this issue Aug 9, 2020
Note this produces more false negatives, so this is a trade-off.

Fixes jendrikseipp#221
jingw pushed a commit to jingw/vulture that referenced this issue Aug 14, 2020
This fixes false positives when assigning to `x.some_name` but reading via `some_name`.
Note this produces false negatives in the opposite situation when `x.some_name` and `some_name` are different, so this is a trade-off in the direction of more precision / less sensitivity.

This direction is in line with pyflakes design principles: https://github.com/PyCQA/pyflakes#design-principles
> it will try very, very hard to never emit false positives.

Fixing this more accurately probably requires a lot more work to trace references and analyze context.

Fixes jendrikseipp#221
jingw pushed a commit to jingw/vulture that referenced this issue Aug 15, 2020
This fixes false positives when assigning to `x.some_name` but reading via `some_name`.
Note this produces false negatives in the opposite situation when `x.some_name` and `some_name` are different, so this is a trade-off in the direction of more precision / less sensitivity.

This direction is in line with pyflakes design principles: https://github.com/PyCQA/pyflakes#design-principles
> it will try very, very hard to never emit false positives.

Fixing this more accurately probably requires a lot more work to trace references and analyze context.

Fixes jendrikseipp#221
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants