Add Python 3 checker for accessing deprecated functions on the `strin…

…g` module. (#1185)

This also triggered a "Rule of 3" refactoring for me to generalize warning about
accessing a given attribute on a module.

CONTRIBUTORS.txt

@@ -115,6 +115,7 @@ Order doesn't matter (not that much, at least ;)
   Added Python 3 check for calling encode/decode with invalid codecs.
   Added Python 3 check for accessing sys.maxint.
   Added Python 3 check for bad import statements.
+  Added Python 3 check for accessing deprecated methods on the 'string' module.
 
 * Erik Eriksson - Added overlapping-except error check.
 

ChangeLog

@@ -208,6 +208,8 @@ Release date: tba
 
     * Added a new Python 3 check for bad imports.
 
+    * Added a new Python 3 check for accessing deprecated string functions.
+
 
 What's new in Pylint 1.6.3?
 ===========================

doc/whatsnew/2.0.rst

@@ -373,6 +373,20 @@ New checkers
   This checker will assume any imports that happen within a conditional or a ``try/except`` block
   are valid.
 
+* A new Python 3 checker was added to warn about accessing deprecated functions on the string
+  module.  Python 3 removed functions that were duplicated from the builtin ``str`` class.  See
+  https://docs.python.org/2/library/string.html#deprecated-string-functions for more information.
+
+  .. code-block:: python
+
+      import string
+      print(string.upper('hello world!'))
+
+  Instead of using ``string.upper``, call the ``upper`` method directly on the string object.
+
+  .. code-block:: python
+
+      "hello world!".upper()
 
 Other Changes
 =============

pylint/checkers/python3.py

@@ -10,6 +10,8 @@
 import re
 import tokenize
 
+import six
+
 import astroid
 from astroid import bases
 
@@ -383,6 +385,10 @@ class Python3Checker(checkers.BaseChecker):
                   'bad-python3-import',
                   'Used when importing a module that no longer exists in Python 3.',
                   {'maxversion': (3, 0)}),
+        'W1649': ('Accessing a function method on the string module',
+                  'deprecated-string-function',
+                  'Used when accessing a string function that has been deprecated in Python 3.',
+                  {'maxversion': (3, 0)}),
     }
 
     _bad_builtins = frozenset([
@@ -441,26 +447,40 @@ class Python3Checker(checkers.BaseChecker):
         'rot_13',
     ])
 
-    _bad_imports = frozenset([
-        'anydbm', 'BaseHTTPServer', '__builtin__', 'CGIHTTPServer', 'ConfigParser', 'copy_reg',
-        'cPickle', 'cProfile', 'cStringIO', 'Cookie', 'cookielib', 'dbhash', 'dbm', 'dumbdbm',
-        'dumbdb', 'Dialog', 'DocXMLRPCServer', 'FileDialog', 'FixTk', 'gdbm', 'htmlentitydefs',
-        'HTMLParser', 'httplib', 'markupbase', 'Queue', 'repr', 'robotparser', 'ScrolledText',
-        'SimpleDialog', 'SimpleHTTPServer', 'SimpleXMLRPCServer', 'StringIO', 'dummy_thread',
-        'SocketServer', 'test.test_support', 'Tkinter', 'Tix', 'Tkconstants', 'tkColorChooser',
-        'tkCommonDialog', 'Tkdnd', 'tkFileDialog', 'tkFont', 'tkMessageBox', 'tkSimpleDialog',
-        'turtle', 'UserList', 'UserString', 'whichdb', '_winreg', 'xmlrpclib', 'audiodev',
-        'Bastion', 'bsddb185', 'bsddb3', 'Canvas', 'cfmfile', 'cl', 'commands', 'compiler',
-        'dircache', 'dl', 'exception', 'fpformat', 'htmllib', 'ihooks', 'imageop', 'imputil',
-        'linuxaudiodev', 'md5', 'mhlib', 'mimetools', 'MimeWriter', 'mimify', 'multifile',
-        'mutex', 'new', 'popen2', 'posixfile', 'pure', 'rexec', 'rfc822', 'sha', 'sgmllib',
-        'sre', 'stat', 'stringold', 'sunaudio', 'sv', 'test.testall', 'thread', 'timing',
-        'toaiff', 'user', 'urllib2', 'urlparse'
-    ])
+    _bad_python3_module_map = {
+        'sys-max-int': {
+            'sys': frozenset(['maxint'])
+        },
+        'bad-python3-import': frozenset([
+            'anydbm', 'BaseHTTPServer', '__builtin__', 'CGIHTTPServer', 'ConfigParser', 'copy_reg',
+            'cPickle', 'cProfile', 'cStringIO', 'Cookie', 'cookielib', 'dbhash', 'dbm', 'dumbdbm',
+            'dumbdb', 'Dialog', 'DocXMLRPCServer', 'FileDialog', 'FixTk', 'gdbm', 'htmlentitydefs',
+            'HTMLParser', 'httplib', 'markupbase', 'Queue', 'repr', 'robotparser', 'ScrolledText',
+            'SimpleDialog', 'SimpleHTTPServer', 'SimpleXMLRPCServer', 'StringIO', 'dummy_thread',
+            'SocketServer', 'test.test_support', 'Tkinter', 'Tix', 'Tkconstants', 'tkColorChooser',
+            'tkCommonDialog', 'Tkdnd', 'tkFileDialog', 'tkFont', 'tkMessageBox', 'tkSimpleDialog',
+            'turtle', 'UserList', 'UserString', 'whichdb', '_winreg', 'xmlrpclib', 'audiodev',
+            'Bastion', 'bsddb185', 'bsddb3', 'Canvas', 'cfmfile', 'cl', 'commands', 'compiler',
+            'dircache', 'dl', 'exception', 'fpformat', 'htmllib', 'ihooks', 'imageop', 'imputil',
+            'linuxaudiodev', 'md5', 'mhlib', 'mimetools', 'MimeWriter', 'mimify', 'multifile',
+            'mutex', 'new', 'popen2', 'posixfile', 'pure', 'rexec', 'rfc822', 'sha', 'sgmllib',
+            'sre', 'stat', 'stringold', 'sunaudio', 'sv', 'test.testall', 'thread', 'timing',
+            'toaiff', 'user', 'urllib2', 'urlparse'
+        ]),
+        'deprecated-string-function': {
+            'string': frozenset([
+                'maketrans', 'atof', 'atoi', 'atol', 'capitalize', 'expandtabs', 'find', 'rfind',
+                'index', 'rindex', 'count', 'lower', 'split', 'rsplit', 'splitfields', 'join',
+                'joinfields', 'lstrip', 'rstrip', 'strip', 'swapcase', 'translate', 'upper',
+                'ljust', 'rjust', 'center', 'zfill', 'replace'
+            ])
+        }
+    }
 
     def __init__(self, *args, **kwargs):
         self._future_division = False
         self._future_absolute_import = False
+        self._modules_warned_about = set()
         super(Python3Checker, self).__init__(*args, **kwargs)
 
     def visit_module(self, node): # pylint: disable=unused-argument
@@ -469,6 +489,7 @@ def visit_module(self, node): # pylint: disable=unused-argument
         self._future_absolute_import = False
 
     def visit_functiondef(self, node):
+        print('Checking {}'.format(node.name))
         if node.is_method() and node.name in self._unused_magic_methods:
             method_name = node.name
             if node.name.startswith('__'):
@@ -493,6 +514,15 @@ def visit_name(self, node):
     def visit_print(self, node):
         self.add_message('print-statement', node=node)
 
+    def _warn_if_deprecated(self, node, module, attributes):
+        for message, module_map in six.iteritems(self._bad_python3_module_map):
+            if module in module_map and module not in self._modules_warned_about:
+                if isinstance(module_map, frozenset):
+                    self._modules_warned_about.add(module)
+                    self.add_message(message, node=node)
+                elif attributes and module_map[module].intersection(attributes):
+                    self.add_message(message, node=node)
+
     def visit_importfrom(self, node):
         if node.modname == '__future__':
             for name, _ in node.names:
@@ -504,8 +534,8 @@ def visit_importfrom(self, node):
             if not self._future_absolute_import:
                 if self.linter.is_message_enabled('no-absolute-import'):
                     self.add_message('no-absolute-import', node=node)
-            if node.modname in self._bad_imports and not _is_conditional_import(node):
-                self.add_message('bad-python3-import', node=node)
+            if not _is_conditional_import(node):
+                self._warn_if_deprecated(node, node.modname, {x[0] for x in node.names})
 
         if node.names[0][0] == '*':
             if self.linter.is_message_enabled('import-star-module-level'):
@@ -515,9 +545,9 @@ def visit_importfrom(self, node):
     def visit_import(self, node):
         if not self._future_absolute_import:
             self.add_message('no-absolute-import', node=node)
-        for name in node.names:
-            if name[0] in self._bad_imports and not _is_conditional_import(node):
-                self.add_message('bad-python3-import', node=node)
+        if not _is_conditional_import(node):
+            for name, _ in node.names:
+                self._warn_if_deprecated(node, name, None)
 
     @utils.check_messages('metaclass-assignment')
     def visit_classdef(self, node):
@@ -569,6 +599,13 @@ def visit_call(self, node):
         self._check_cmp_argument(node)
 
         if isinstance(node.func, astroid.Attribute):
+            try:
+                for inferred_receiver in node.func.expr.infer():
+                    if isinstance(inferred_receiver, astroid.Module):
+                        self._warn_if_deprecated(node, inferred_receiver.name,
+                                                 {node.func.attrname})
+            except astroid.InferenceError:
+                pass
             if node.args:
                 if node.func.attrname in ('encode', 'decode'):
                     if len(node.args) >= 1 and node.args[0]:
@@ -610,10 +647,10 @@ def _validate_encoding(self, encoding, node):
     def visit_subscript(self, node):
         """ Look for indexing exceptions. """
         try:
-            for infered in node.value.infer():
-                if not isinstance(infered, astroid.Instance):
+            for inferred in node.value.infer():
+                if not isinstance(inferred, astroid.Instance):
                     continue
-                if utils.inherit_from_std_ex(infered):
+                if utils.inherit_from_std_ex(inferred):
                     self.add_message('indexing-exception', node=node)
         except astroid.InferenceError:
             return
@@ -629,14 +666,13 @@ def visit_delattr(self, node):
     def visit_attribute(self, node):
         """ Look for accessing message on exceptions. """
         try:
-            for infered in node.expr.infer():
-                if (isinstance(infered, astroid.Instance) and
-                        utils.inherit_from_std_ex(infered)):
+            for inferred in node.expr.infer():
+                if (isinstance(inferred, astroid.Instance) and
+                        utils.inherit_from_std_ex(inferred)):
                     if node.attrname == 'message':
                         self.add_message('exception-message-attribute', node=node)
-                if isinstance(infered, astroid.Module) and infered.name == 'sys':
-                    if node.attrname == 'maxint':
-                        self.add_message('sys-max-int', node=node)
+                if isinstance(inferred, astroid.Module):
+                    self._warn_if_deprecated(node, inferred.name, {node.attrname})
         except astroid.InferenceError:
             return
 

pylint/test/unittest_checker_python3.py

@@ -501,6 +501,16 @@ def test_sys_maxint(self):
         with self.assertAddsMessages(message):
             self.checker.visit_attribute(node)
 
+    @python2_only
+    def test_sys_maxint_imort_from(self):
+        node = astroid.extract_node('''
+        from sys import maxint #@
+        ''')
+        absolute_import_message = testutils.Message('no-absolute-import', node=node)
+        message = testutils.Message('sys-max-int', node=node)
+        with self.assertAddsMessages(absolute_import_message, message):
+            self.checker.visit_importfrom(node)
+
     @python2_only
     def test_object_maxint(self):
         node = astroid.extract_node('''
@@ -581,14 +591,58 @@ def test_bad_import_from(self):
             self.checker.visit_importfrom(node)
 
     @python2_only
-    def test_bad_import_else_block(self):
+    def test_bad_string_attribute(self):
         node = astroid.extract_node('''
-        import six
-        if six.PY3:
-            from io import StringIO
-        else:
-            from cStringIO import StringIO #@
+        import string
+        string.maketrans #@
+        ''')
+        message = testutils.Message('deprecated-string-function', node=node)
+        with self.assertAddsMessages(message):
+            self.checker.visit_attribute(node)
+
+    @python2_only
+    def test_ok_string_attribute(self):
+        node = astroid.extract_node('''
+        import string
+        string.letters #@
+        ''')
+        with self.assertNoMessages():
+            self.checker.visit_attribute(node)
+
+    @python2_only
+    def test_bad_string_call(self):
+        node = astroid.extract_node('''
+        import string
+        string.upper("hello world") #@
+        ''')
+        message = testutils.Message('deprecated-string-function', node=node)
+        with self.assertAddsMessages(message):
+            self.checker.visit_call(node)
+
+    @python2_only
+    def test_ok_string_call(self):
+        node = astroid.extract_node('''
+        import string
+        string.Foramtter() #@
         ''')
+        with self.assertNoMessages():
+            self.checker.visit_call(node)
+
+    @python2_only
+    def test_bad_string_import_from(self):
+        node = astroid.extract_node('''
+         from string import atoi #@
+         ''')
+        absolute_import_message = testutils.Message('no-absolute-import', node=node)
+        message = testutils.Message('deprecated-string-function', node=node)
+        with self.assertAddsMessages(absolute_import_message, message):
+            self.checker.visit_importfrom(node)
+
+    @python2_only
+    def test_ok_string_import_from(self):
+        node = astroid.extract_node('''
+         from string import digits #@
+         ''')
         absolute_import_message = testutils.Message('no-absolute-import', node=node)
         with self.assertAddsMessages(absolute_import_message):
             self.checker.visit_importfrom(node)