Merge lp:~jml/divmod.org/duplicate-class-defs into lp:divmod.org

[Jonathan Lange]

This merge proposal lived at https://code.launchpad.net/~jml/pyflakes/duplicate-class-defs/+merge/44601 for a while, but after Tristan Seligmann's announcement of the release of pyflakes 0.5.0, I've moved it to this project, which seems more appropriate.

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?

[Christopher Armstrong (radix)]

[1] "A binding that that defines a function or class" -> duplicated "that".

[2] test_other.py line 133: there are four lines of whitespace here, between two methods, instead of 2.

Thanks for replacing the dumb test-for-bad-behavior with a TODOed test for the desired behavior.

I grabbed the branch, manually verified some behaviors (like class foo(foo) not emitting a warning), ran the unit tests, and reviewed the code itself.

This looks good. With the above mentioned lexical changes you have my +1.

I don't believe I have commit access to the trunk branch, so you'll have to get someone else who trusts my review to commit it. Sorry :-(

review: Approve (code)

[Jonathan Lange (jml)]

Thanks, I've made the lexical changes described. Will hassle ~divmod-dev.

[lp:~jml/divmod.org/duplicate-class-defs]

2686. By Jonathan Lange on 2011-11-17

Remove duplicate 'that' and excess whitespace.

=== modified file 'Pyflakes/pyflakes/checker.py'
--- Pyflakes/pyflakes/checker.py	2010-04-13 14:53:04 +0000
+++ Pyflakes/pyflakes/checker.py	2011-11-17 16:26:23 +0000
@@ -85,6 +85,11 @@
     """
 
 
+class Definition(Binding):
+    """
+    A binding 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/pyflakes/messages.py'
--- Pyflakes/pyflakes/messages.py	2009-07-06 12:01:14 +0000
+++ Pyflakes/pyflakes/messages.py	2011-11-17 16:26:23 +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/pyflakes/test/test_imports.py'
--- Pyflakes/pyflakes/test/test_imports.py	2010-04-13 14:53:04 +0000
+++ Pyflakes/pyflakes/test/test_imports.py	2011-11-17 16:26:23 +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/pyflakes/test/test_other.py'
--- Pyflakes/pyflakes/test/test_other.py	2010-04-13 14:53:04 +0000
+++ Pyflakes/pyflakes/test/test_other.py	2011-11-17 16:26:23 +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,85 @@
         ''')
 
 
+    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 +207,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('''