Detect unused functions and classes in closures

[adamchainz]

Fixes #392.

[asottile]

looks good to me! thanks for the patch

[asmeurer]

Nice. It looks good to me too. Apparently the redefinition warnings already work with functions and classes, so this adds consistency there.

[asottile]

hmmm I'm not sure about the latest commit -- if someone is doing global decorator magic I think that should be flagged as suspicious

(see above)

[adamchainz]

Decorators don't always count as global or magic. For example in some tests on a project I work on we have a function that returns a flask app, something like:

def make_app():
    app = Flask('test app')
    
    @app.route('/')
    def home():
        return 'Home'
    
    return app

I feel like this is pretty standard Pythonic behaviour. The suspiciousness of whether the variable is used depends vastly on the decorators used. Also one can always course rewrite @decorator to func = decorator(func) and Flake8 will not detect that as suspicious since it's a legitimate variable use.

[asottile]

that decorator is purely side-effects -- in my book, magic

[adamchainz]

I disagree, it would be a block in Ruby or an anonymous function in JavaScript. Python just requires us to name our >1 line functions so you end up with side-effect decorators like this.

Anyway, can we merge this as-is and make a separate issue for detecting such cases? I feel like it would want to be a separate check so it can be enabled/disabled separately.

[asottile]

Anyway, can we merge this as-is and make a separate issue for detecting such cases? I feel like it would want to be a separate check so it can be enabled/disabled separately.

if anything we should merge this without that commit and then discuss separately that controversial portion

[graingert]

I'd be ok with merging this without 8d3dbb8 if all side effecting functions are also forbidden.

requests.post(url, data) # side effecting magic

[adamchainz]

@asottile can you merge this one way or another?

I still believe ignoring decorated functions/classes makes sense.

I ran against Django master, with and without the "ignore decorated items" commit.

Including decorated items, there are 138 errors:

$ python -m pyflakes ~/Documents/Projects/django 2>/dev/null | grep -E 'local (function|class)' | wc -l
     138

Without decorated items, there are 118:

$ git checkout HEAD~1
$ python -m pyflakes ~/Documents/Projects/django 2>/dev/null | grep -E 'local (function|class)' | wc -l
     118

Of these 118, only 1 is an unused local function, that is placed into a Django view's locals to check that its stack-inspecting error logic doesn't accidentally call it:

# django/tests/view_tests/views.py
def raises(request):
    # Make sure that a callable that raises an exception in the stack frame's
    # local vars won't hijack the technical 500 response (#15025).
    def callable():
        raise Exception
    try:
        raise Exception
    except Exception:
        return technical_500_response(request, *sys.exc_info())

The other 117 seem to all be checking for exceptions during class definition (Django models, forms, etc.). Due to meta classes, defining a class is not side-effect-free - just like a decorator - and its these side effects those tests are checking for.

The 20 extra errors seen when including decorated functions/classes seem to all be based on various "registry patterns," similar to my original Flask example, for example with a Django signal:

def test_receiver_single_signal(self):
    @receiver(a_signal)
    def f(val, **kwargs):
        self.state = val
    # ...

Such code can always be rewritten like:

def test_receiver_single_signal(self):
    def f(val, **kwargs):
        self.state = val
    receiver(a_signal)(f)
    # ...

Which pyflakes would count as a legitimate variable use.

I also ran against CPython master. There were also many unused classes, all of which seem to be in tests defined for their side effects, but a number of accidentally unused functions which I fixed in python/cpython#17189 . All unused functions with decorators were using a similar "registry" pattern again.

[asottile]

there's a bit of a weird sign off model here where we need two approvals to merge (and I'd prefer without the decorator patch myself) -- but other than that the change looks good to me 👍

[graingert]

I think the register pattern is extremely useful

[asottile]

Note that the decorator discussion is happening in #488 where the answer from maintainers is pretty unanimously "no"

[jayvdb]

IMO a possible way to approach to break the deadlock here is to create a new message for "UnusedDecorated", which can then be explicitly ignored when a codebase is intentionally using that type of magic.

Any codebase currently using pyflakes will likely already have an explicit ignore for the much broader unused message currently emitted for this. If they are using inline ignores, they will need to swap in the new more precise and self-explanatory ignore code. If they are using a global ignore, they will need to add the new one, and may not notice to remove the more generic code, but over time projects will remove any explicit ignores for the generic unused.

This more precise message type will surface broader user feedback.

[jayvdb]

This test can be moved to a new PR and will pass as-is.

[jayvdb]

This test could be moved to a new PR and UnusedClass introduced in a non-controversial manner.