Merge lp:~jml/pyflakes/duplicate-class-defs into lp:~vcs-imports/pyflakes/main

[Jonathan Lange]

This branch changes pyflakes to warn about more kinds duplicated definitions:
  * reassigning to a variable without using it first
  * redefining a class with the same name as an existing class / function
  * redefining a function with the same name as an existing class / function

And so forth.

In the branch, I deleted one existing test that seemed to run counter to this desired behaviour.

I've simplified the "check for redefinition" logic so that it checks for any rebindings. I had to be careful not to tread on the special Importation checks/warnings and the Argument re-use warnings. The way I did this was to make a special kind of binding type called Definition, and make Assignment, FunctionDefinition and ClassDefinition inherit from this.

Note that the new code will *not* catch things like this::

  def f(x):
    x = 1

Perhaps it should.

Below is the review discussion from the divmod ticket, which may one day be available on http://divmod.org/trac/ticket/3034.

Agreed. I'll fix this up & resubmit a patch.

===

Okay. If this bothers people, I guess they can rename the class they're smashing to something else.

The special handling of ClassDefinition (mirroring the existing special handling of FunctionDefinition) seems weird though. What about this?

$ cat | pyflakes
def foo(): pass
class foo: pass
$
Seems as warning-worthy as two functions or two classes. I wonder why it doesn't just check bindings. Is this not worth a warning?

$ cat | pyflakes
x = 10
x = 10
$
These all seem like they should be handled like imports are handled:

$ cat | pyflakes
from twisted import foo
from twisted import foo
<stdin>:2: redefinition of unused 'foo' from line 1
<stdin>:2: 'foo' imported but unused

===

Warning on class re-definitions is consistent with warning about function re-definitions:

cat | ./bin/pyflakes
def f():
  pass
x = f()
def f():
  pass
<stdin>:4: redefinition of function 'f' from line 1
With class redefinitions, the only times I've ever used the facility to redefine a class in a Python module, it has been by mistake.

Both observations lend weight to including my patch, although I'll admit it's a matter of judgment.

I *almost* wish pyflakes had some kind of configuration system.

===

This looks like a good thing to fix. I don't know about this though:

$ cat | pyflakes
class foo: pass
class foo(foo): pass
<stdin>:2: redefinition of class 'foo' from line 1
The class being redefined was used before its name was rebound, at least. Worth a warning? Or will someone do this intentionally and be upset at the warning?

It's even less clear in other equivalent cases:

$ cat | pyflakes
class foo: pass
x = foo()
class foo: pass
<stdin>:3: redefinition of class 'foo' from line 1
Any thoughts?

[Robert Collins (lifeless)]

On Fri, Dec 24, 2010 at 6:25 AM, Jonathan Lange <email address hidden> wrote:
> Jonathan Lange has proposed merging lp:~jml/pyflakes/duplicate-class-defs into lp:pyflakes.
>
> Requested reviews:
>  VCS imports (vcs-imports)
>
> For more details, see:
> https://code.launchpad.net/~jml/pyflakes/duplicate-class-defs/+merge/44601
>
> This branch changes pyflakes to warn about more kinds duplicated definitions:
>  * reassigning to a variable without using it first

I would hate to be nagged about this. I do the following a lot:
foo = sanedefault
if condition:
    foo = somethingbecauseofcondition

>  * redefining a class with the same name as an existing class / function
>  * redefining a function with the same name as an existing class / function

Woo.

[Jonathan Lange (jml)]

Don't worry about that Rob, you won't get nagged, even with this branch as it stands.

[lp:~jml/pyflakes/duplicate-class-defs]

48. By Jonathan Lange on 2010-12-24

Add a test allaying Rob's fears

[Jonathan Lange (jml)]

Oops. Got the test wrong. Accidentally asserted that it generated a warning. Fixing now.

[lp:~jml/pyflakes/duplicate-class-defs]

49. By Jonathan Lange on 2010-12-24

Fixinate

=== modified file 'pyflakes/checker.py'
--- pyflakes/checker.py	2010-04-13 15:53:04 +0000
+++ pyflakes/checker.py	2010-12-24 11:21:25 +0000
@@ -85,6 +85,11 @@
     """
 
 
+class Definition(Binding):
+    """
+    A binding that that defines a function or class.
+    """
+
 
 class Assignment(Binding):
     """
@@ -97,7 +102,12 @@
 
 
 
-class FunctionDefinition(Binding):
+class FunctionDefinition(Definition):
+    pass
+
+
+
+class ClassDefinition(Definition):
     pass
 
 
@@ -350,11 +360,6 @@
         - if `reportRedef` is True (default), rebinding while unused will be
           reported.
         '''
-        if (isinstance(self.scope.get(value.name), FunctionDefinition)
-                    and isinstance(value, FunctionDefinition)):
-            self.report(messages.RedefinedFunction,
-                        lineno, value.name, self.scope[value.name].source.lineno)
-
         if not isinstance(self.scope, ClassScope):
             for scope in self.scopeStack[::-1]:
                 existing = scope.get(value.name)
@@ -366,11 +371,15 @@
                     self.report(messages.RedefinedWhileUnused,
                                 lineno, value.name, scope[value.name].source.lineno)
 
+        existing = self.scope.get(value.name)
         if isinstance(value, UnBinding):
             try:
                 del self.scope[value.name]
             except KeyError:
                 self.report(messages.UndefinedName, lineno, value.name)
+        elif isinstance(existing, Definition) and not existing.used:
+            self.report(messages.RedefinedWhileUnused,
+                        lineno, value.name, existing.source.lineno)
         else:
             self.scope[value.name] = value
 
@@ -582,7 +591,7 @@
         for stmt in node.body:
             self.handleNode(stmt, node)
         self.popScope()
-        self.addBinding(node.lineno, Binding(node.name, node))
+        self.addBinding(node.lineno, ClassDefinition(node.name, node))
 
     def ASSIGN(self, node):
         self.handleNode(node.value, node)

=== modified file 'pyflakes/messages.py'
--- pyflakes/messages.py	2009-07-06 13:01:14 +0000
+++ pyflakes/messages.py	2010-12-24 11:21:25 +0000
@@ -68,8 +68,8 @@
         self.message_args = (name,)
 
 
-class RedefinedFunction(Message):
-    message = 'redefinition of function %r from line %r'
+class Redefined(Message):
+    message = 'redefinition of %r from line %r'
     def __init__(self, filename, lineno, name, orig_lineno):
         Message.__init__(self, filename, lineno)
         self.message_args = (name, orig_lineno)

=== modified file 'pyflakes/test/test_imports.py'
--- pyflakes/test/test_imports.py	2010-04-13 15:53:04 +0000
+++ pyflakes/test/test_imports.py	2010-12-24 11:21:25 +0000
@@ -479,9 +479,6 @@
         fu
         ''', m.RedefinedWhileUnused)
 
-    def test_ignoreNonImportRedefinitions(self):
-        self.flakes('a = 1; a = 2')
-
     def test_importingForImportError(self):
         self.flakes('''
         try:

=== modified file 'pyflakes/test/test_other.py'
--- pyflakes/test/test_other.py	2010-04-13 15:53:04 +0000
+++ pyflakes/test/test_other.py	2010-12-24 11:21:25 +0000
@@ -33,7 +33,7 @@
         self.flakes('''
         def a(): pass
         def a(): pass
-        ''', m.RedefinedFunction)
+        ''', m.RedefinedWhileUnused)
 
     def test_redefinedClassFunction(self):
         """
@@ -44,7 +44,7 @@
         class A:
             def a(): pass
             def a(): pass
-        ''', m.RedefinedFunction)
+        ''', m.RedefinedWhileUnused)
 
     def test_functionDecorator(self):
         """
@@ -109,6 +109,87 @@
         ''')
 
 
+    def test_classRedefinition(self):
+        """
+        If a class is defined twice in the same module, a warning is emitted.
+        """
+        self.flakes(
+        '''
+        class Foo:
+            pass
+        class Foo:
+            pass
+        ''', m.RedefinedWhileUnused)
+
+
+
+
+    def test_functionRedefinedAsClass(self):
+        """
+        If a function is redefined as a class, a warning is emitted.
+        """
+        self.flakes(
+        '''
+        def Foo():
+            pass
+        class Foo:
+            pass
+        ''', m.RedefinedWhileUnused)
+
+
+    def test_classRedefinedAsFunction(self):
+        """
+        If a class is redefined as a function, a warning is emitted.
+        """
+        self.flakes(
+        '''
+        class Foo:
+            pass
+        def Foo():
+            pass
+        ''', m.RedefinedWhileUnused)
+
+
+    def test_doubleAssignment(self):
+        """
+        If a variable is re-assigned to without being used, no warning is
+        emitted.
+        """
+        self.flakes(
+        '''
+        x = 10
+        x = 20
+        ''', m.RedefinedWhileUnused)
+    test_doubleAssignment.todo = (
+        "Too hard to make this warn but other cases stay silent")
+
+
+    def test_doubleAssignmentConditionally(self):
+        """
+        If a variable is re-assigned within a conditional, no warning is
+        emitted.
+        """
+        self.flakes(
+        '''
+        x = 10
+        if True:
+            x = 20
+        ''')
+
+
+    def test_doubleAssignmentWithUse(self):
+        """
+        If a variable is re-assigned to after being used, no warning is
+        emitted.
+        """
+        self.flakes(
+        '''
+        x = 10
+        y = x * 2
+        x = 20
+        ''')
+
+
     def test_comparison(self):
         """
         If a defined name is used on either side of any of the six comparison
@@ -128,7 +209,7 @@
 
     def test_identity(self):
         """
-        If a deefined name is used on either side of an identity test, no
+        If a defined name is used on either side of an identity test, no
         warning is emitted.
         """
         self.flakes('''