Warn about unused variables in methods

Are you talking about something like this?

def foo(bar):
    baz = bar + 2
    return 12

I guess it would indeed be useful if this generated a warning about how baz is unused,

Yes, that's exactly what I mean.

I've uploaded a branch to Launchpad that addresses this bug: https://code.edge.launchpad.net/~jml/pyflakes/unused-in-function-2718. I'll attach a diff momentarily.

In the diff I add a new message UnusedVariable and add some code near the end of runFunction that loops over variables in scope and warns if they weren't used. To avoid spurious warnings, I check that the binding is an instance of Assignment. To make that actually work, I added a new Binding type called Argument and made the argument bindings use that and I changed class definition just to use a raw Binding rather than an assignment.

The tests are in test_other, this seemed as good a place as any. After I got the new feature working I had to update a bunch of the existing tests to actually use their locally defined variables.

I cleaned up vertical whitespace occasionally (although now that I think about it, I went PEP 8 rather than Divmod — oops).

This patch is more of an RFC than an actual request to merge. If you (whoever you are) are happy with the approach then please do a thorough review. If not, tell me a better approach and I'll resubmit.

Thanks, jml

Assigning to no-one, following glyph's advice.

Hmm. I like the direction of the patch, but now that I can try the feature against existing code, I see some unexpected side-effects.

• unused names in for loops and list comprehensions: it's quite common to have names bound by a loop which then aren't used; it's particularly common when doing tuple unpacking, as often not all of the fields are of interest. The current patch warns about a lot of these in the code I looked at, and the warnings are basically just noise. It might be worth warning if there is tuple unpacking and none of the names are used (or maybe that would end up just being noise too).
• names bound in a loop: a warning is emitted for a name assigned later in any loop construct if the name isn't used lexically afterwards, but the loop body may be executed again and the result of the assignment used by a lexically earlier piece of code, so these warnings are just wrong most of the time

Also, names which are closed over are reported as unused if only the nested scope reads them. Hopefully that's just a simple bug, nothing complicated.

Sorry for the delay on the review, thanks for working on this. Even after filtering out all the false positives, I found a bunch of dead code while doing this review.

Updated patch, replying to exarkun's comments.

Thanks for the helpful review.

The patch now:

• explicitly ignores assignments that take place in for loop statements, list comprehensions, generator expressions, tuples and lists. It does this by marking them as "bindings" rather than "assignments". The nomenclature here might need revisiting.
• Records if the name has already been used in the scope if it is rebound. This fixes the "for loop" problem.
• Defers unused variable checking until after all functions have been processed. This solves the "closed over" problem you mentioned. The solution here is a little ugly, but tolerable. I think the right thing to do would be to re-jig pyflakes so that it had a more explicit custom event model.
• Does some VWS normalization.

Definitely better. There are some coding standard issues, like new classes, methods, parameters, or attributes without documentation and names which use _ as a word separator (in particular, the Binding/Assignment pair could use some good documentation as that's potentially quite confusing). These should be fixed. Otherwise, I'm really liking the patch.

Diff between 2 and 3 -- style changes.

New diff with style changes.

I've attached an updated patch and a diff showing what's changed from the last patch. I'm not sure if I've got all the coding style issues.

(In [16991]) Branching to 'unused-variable-flakes-2718'

(In [16992]) Apply unused-in-function-2718-3.diff

refs #2718

Cool. A few more pretty simple things:

1. Checker's two new attributes, _deferredFunctions and _deferredAssignments are undocumented
2. I didn't realize what was going on with deferAssignment and deferFunction before, but now I think I do. It'd probably be good if the non-reentrancy of each of these functions were documented. It might be even better if they raised an exception if invoked in that way. Alternatively, if they're folded back into a single function which is reentrant, do the users of the API have their requirements satisfied?
3. deferFunction and deferAssignment say to be called after just before completion. I guess that after isn't supposed to be there?
4. The parent parameter to the handleNode method is undocumented. Also, does it actually need a default value? The WITH method of Checker is the only caller which doesn't supply a parent. Why doesn't it supply a parent? I guess its parent is always a with node, rather than one of the things that the node's parent is checked against, so the fact that it's missing doesn't matter. I feel kind of bad about the parent being inconsistent in this case, even though it has no end-user visible consequences. It might be surprising to a future developer, or something.

3 & 4 are now fixed.

Re 2 & 1, the reason we have both of these things is that:

def foo:
    def bar:
        def baz:

is processed in the order "foo, bar, baz".

But to handle any assignments that occur in "foo" correctly, we need to have first processed "bar" and "baz".

The "right" thing to do is to ditch the deferred approach and instead have a more event-y system. I don't think this is required for this patch.

I'm not actually sure what you mean by the non-reentrancy of those functions. AIUI, they get re-entered all the time.

Hum. You're right. I failed to understand how that part of the code works. So a large portion of the Pyflakes code executes beneath _runDeferred. That code almost certainly ends up invoking deferFunction, which appends to the list _runDeferred is iterating over. That's okay, though, since iteration eventually gets to those new elements.

I still think there's a potential problem, though. Or at least a hypothetical potential problem. ;) After the deferred functions are processed, the deferred assignments are processed. The code in the deferred assignments can also (but does not, as the code stands) call deferFunction. However, deferred function processing is over and these deferments will not be processed. Perhaps this is the kind of thing which makes you suggest the right solution you did. It'd be good if it failed noisily instead of silently in the mean time, though.

By the way, you didn't attach a new patch to the ticket this time around.\

Latest version of patch.

Incremental diff from patch 3

So, it's been a long time, but I haven't actually forgotten about this patch. It's been loitering on my todo list with intent.

I've made it so that deferFunction will fail noisily if called after the deferred function processing is over. It's noisy and messy, and I couldn't really think of a nice way to test it. Since this is only a hypothetical potential problem, perhaps we don't need tests or a nice error.

Patches are attached. The unused-in-function-2718-3-progress.diff applies cleanly to the subversion branch, and has the changes I missed in my previous post.

Thanks for the last review, sorry it took so long to get around to it.

(In [17422]) Apply unused-in-function-2718-3-progress.diff

refs #2718

(In [17423]) Branching to 'unused-variable-flakes-2718-2'

(In [17425]) Fix some docstrings/test method names

refs #2718

With a few simple (mostly doc) changes, I think this looks good. I'll go ahead and merge.

(In [17427]) Merge unused-variable-flakes-2718-2

Author: jml Reviewer: exarkun Fixes: #2718

Add support for detecting local variables which are bound but then never used.