Detect unused functions and classes in closures

[adamchainz]

Fixes #392.

[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

[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 https://github.com/python/cpython/pull/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 👍

[asottile]

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