Merge pull request #3118 from gilles-peskine-arm/pylint-up-to-2.4-development

Pass Pylint up to 2.4

.pylintrc

@@ -40,7 +40,27 @@ max-attributes=15
 max-module-lines=2000
 
 [MESSAGES CONTROL]
-disable=
+# * locally-disabled, locally-enabled: If we disable or enable a message
+#   locally, it's by design. There's no need to clutter the Pylint output
+#   with this information.
+# * logging-format-interpolation: Pylint warns about things like
+#   ``log.info('...'.format(...))``. It insists on ``log.info('...', ...)``.
+#   This is of minor utility (mainly a performance gain when there are
+#   many messages that use formatting and are below the log level).
+#   Some versions of Pylint (including 1.8, which is the version on
+#   Ubuntu 18.04) only recognize old-style format strings using '%',
+#   and complain about something like ``log.info('{}', foo)`` with
+#   logging-too-many-args (Pylint supports new-style formatting if
+#   declared globally with logging_format_style under [LOGGING] but
+#   this requires Pylint >=2.2).
+# * no-else-return: Allow the perfectly reasonable idiom
+#    if condition1:
+#        return value1
+#    else:
+#        return value2
+# * unnecessary-pass: If we take the trouble of adding a line with "pass",
+#   it's because we think the code is clearer that way.
+disable=locally-disabled,locally-enabled,logging-format-interpolation,no-else-return,unnecessary-pass
 
 [REPORTS]
 # Don't diplay statistics. Just the facts.

scripts/abi_check.py

@@ -29,7 +29,7 @@ from types import SimpleNamespace
 import xml.etree.ElementTree as ET
 
 
-class AbiChecker(object):
+class AbiChecker:
     """API and ABI checker."""
 
     def __init__(self, old_version, new_version, configuration):

scripts/config.py

@@ -283,9 +283,13 @@ class ConfigFile(Config):
     def __init__(self, filename=None):
         """Read the Mbed TLS configuration file."""
         if filename is None:
-            for filename in self.default_path:
-                if os.path.lexists(filename):
+            for candidate in self.default_path:
+                if os.path.lexists(candidate):
+                    filename = candidate
                     break
+            else:
+                raise Exception('Mbed TLS configuration file not found',
+                                self.default_path)
         super().__init__()
         self.filename = filename
         self.current_section = 'header'
@@ -448,7 +452,7 @@ if __name__ == '__main__':
                 value = config[args.symbol]
                 if value:
                     sys.stdout.write(value + '\n')
-            return args.symbol not in config
+            return 0 if args.symbol in config else 1
         elif args.command == 'set':
             if not args.force and args.symbol not in config.settings:
                 sys.stderr.write("A #define for the symbol {} "
@@ -461,6 +465,7 @@ if __name__ == '__main__':
         else:
             config.adapt(args.adapter)
         config.write(args.write)
+        return 0
 
     # Import modules only used by main only if main is defined and called.
     # pylint: disable=wrong-import-position

tests/scripts/all.sh

@@ -1773,15 +1773,6 @@ component_test_zeroize () {
     unset gdb_disable_aslr
 }
 
-support_check_python_files () {
-    # Find the installed version of Pylint. Installed as a distro package this can
-    # be pylint3 and as a PEP egg, pylint.
-    if type pylint >/dev/null 2>/dev/null || type pylint3 >/dev/null 2>/dev/null; then
-        true;
-    else
-        false;
-    fi
-}
 component_check_python_files () {
     msg "Lint: Python scripts"
     record_status tests/scripts/check-python-files.sh

tests/scripts/check-files.py

@@ -17,7 +17,7 @@ import codecs
 import sys
 
 
-class FileIssueTracker(object):
+class FileIssueTracker:
     """Base class for file-wide issue tracking.
 
     To implement a checker that processes a file as a whole, inherit from
@@ -37,20 +37,31 @@ class FileIssueTracker(object):
         self.files_with_issues = {}
 
     def should_check_file(self, filepath):
+        """Whether the given file name should be checked.
+
+        Files whose name ends with a string listed in ``self.files_exemptions``
+        will not be checked.
+        """
         for files_exemption in self.files_exemptions:
             if filepath.endswith(files_exemption):
                 return False
         return True
 
     def check_file_for_issue(self, filepath):
+        """Check the specified file for the issue that this class is for.
+
+        Subclasses must implement this method.
+        """
         raise NotImplementedError
 
     def record_issue(self, filepath, line_number):
+        """Record that an issue was found at the specified location."""
         if filepath not in self.files_with_issues.keys():
             self.files_with_issues[filepath] = []
         self.files_with_issues[filepath].append(line_number)
 
     def output_file_issues(self, logger):
+        """Log all the locations where the issue was found."""
         if self.files_with_issues.values():
             logger.info(self.heading)
             for filename, lines in sorted(self.files_with_issues.items()):
@@ -70,6 +81,10 @@ class LineIssueTracker(FileIssueTracker):
     """
 
     def issue_with_line(self, line, filepath):
+        """Check the specified line for the issue that this class is for.
+
+        Subclasses must implement this method.
+        """
         raise NotImplementedError
 
     def check_file_line(self, filepath, line, line_number):
@@ -77,6 +92,10 @@ class LineIssueTracker(FileIssueTracker):
             self.record_issue(filepath, line_number)
 
     def check_file_for_issue(self, filepath):
+        """Check the lines of the specified file.
+
+        Subclasses must implement the ``issue_with_line`` method.
+        """
         with open(filepath, "rb") as f:
             for i, line in enumerate(iter(f.readline, b"")):
                 self.check_file_line(filepath, line, i + 1)
@@ -170,7 +189,7 @@ class MergeArtifactIssueTracker(LineIssueTracker):
         return False
 
 
-class IntegrityChecker(object):
+class IntegrityChecker:
     """Sanity-check files under the current directory."""
 
     def __init__(self, log_file):

tests/scripts/check-python-files.sh

@@ -9,15 +9,10 @@
 # Run 'pylint' on Python files for programming errors and helps enforcing
 # PEP8 coding standards.
 
-# Find the installed version of Pylint. Installed as a distro package this can
-# be pylint3 and as a PEP egg, pylint. We prefer pylint over pylint3
-if type pylint >/dev/null 2>/dev/null; then
-    PYLINT=pylint
-elif type pylint3 >/dev/null 2>/dev/null; then
-    PYLINT=pylint3
+if type python3 >/dev/null 2>/dev/null; then
+    PYTHON=python3
 else
-    echo 'Pylint was not found.'
-    exit 1
+    PYTHON=python
 fi
 
-$PYLINT -j 2 scripts/*.py tests/scripts/*.py
+$PYTHON -m pylint -j 2 scripts/*.py tests/scripts/*.py

tests/scripts/check-test-cases.py

@@ -77,6 +77,7 @@ def check_description(results, seen, file_name, line_number, description):
     seen[description] = line_number
 
 def check_test_suite(results, data_file_name):
+    """Check the test cases in the given unit test data file."""
     in_paragraph = False
     descriptions = {}
     with open(data_file_name, 'rb') as data_file:
@@ -94,6 +95,7 @@ def check_test_suite(results, data_file_name):
             in_paragraph = True
 
 def check_ssl_opt_sh(results, file_name):
+    """Check the test cases in ssl-opt.sh or a file with a similar format."""
     descriptions = {}
     with open(file_name, 'rb') as file_contents:
         for line_number, line in enumerate(file_contents, 1):

tests/scripts/generate_test_code.py

@@ -208,7 +208,7 @@ class GeneratorInputError(Exception):
     pass
 
 
-class FileWrapper(io.FileIO, object):
+class FileWrapper(io.FileIO):
     """
     This class extends built-in io.FileIO class with attribute line_no,
     that indicates line number for the line that is read.
@@ -402,8 +402,7 @@ def parse_dependencies(inp_str):
     :param inp_str: Input string with macros delimited by ':'.
     :return: list of dependencies
     """
-    dependencies = [dep for dep in map(validate_dependency,
-                                       inp_str.split(':'))]
+    dependencies = list(map(validate_dependency, inp_str.split(':')))
     return dependencies
 
 

tests/scripts/mbedtls_test.py

@@ -1,3 +1,5 @@
+#!/usr/bin/env python3
+
 # Greentea host test script for Mbed TLS on-target test suite testing.
 #
 # Copyright (C) 2018, Arm Limited, All Rights Reserved
@@ -46,7 +48,7 @@ class TestDataParserError(Exception):
     pass
 
 
-class TestDataParser(object):
+class TestDataParser:
     """
     Parses test name, dependencies, test function name and test parameters
     from the data file.
@@ -260,7 +262,7 @@ class MbedTlsTest(BaseHostTest):
             data_bytes += bytearray(dependencies)
         data_bytes += bytearray([function_id, len(parameters)])
         for typ, param in parameters:
-            if typ == 'int' or typ == 'exp':
+            if typ in ('int', 'exp'):
                 i = int(param, 0)
                 data_bytes += b'I' if typ == 'int' else b'E'
                 self.align_32bit(data_bytes)

tests/scripts/test_config_script.py

@@ -92,6 +92,7 @@ def list_presets(options):
         return re.split(r'[ ,]+', options.presets)
     else:
         help_text = subprocess.run([options.script, '--help'],
+                                   check=False, # config.pl --help returns 255
                                    stdout=subprocess.PIPE,
                                    stderr=subprocess.STDOUT).stdout
         return guess_presets_from_help(help_text.decode('ascii'))

tests/scripts/test_generate_test_code.py

@@ -294,7 +294,7 @@ class GenDispatch(TestCase):
         self.assertEqual(code, expected)
 
 
-class StringIOWrapper(StringIO, object):
+class StringIOWrapper(StringIO):
     """
     file like class to mock file object in tests.
     """
@@ -1127,9 +1127,8 @@ Diffie-Hellman selftest
 dhm_selftest:
 """
         stream = StringIOWrapper('test_suite_ut.function', data)
-        tests = [(name, test_function, dependencies, args)
-                 for name, test_function, dependencies, args in
-                 parse_test_data(stream)]
+        # List of (name, function_name, dependencies, args)
+        tests = list(parse_test_data(stream))
         test1, test2, test3, test4 = tests
         self.assertEqual(test1[0], 'Diffie-Hellman full exchange #1')
         self.assertEqual(test1[1], 'dhm_do_dhm')
@@ -1170,9 +1169,8 @@ dhm_do_dhm:10:"93450983094850938450983409623":10:"9345098304850938450983409622"
 
 """
         stream = StringIOWrapper('test_suite_ut.function', data)
-        tests = [(name, function_name, dependencies, args)
-                 for name, function_name, dependencies, args in
-                 parse_test_data(stream)]
+        # List of (name, function_name, dependencies, args)
+        tests = list(parse_test_data(stream))
         test1, test2 = tests
         self.assertEqual(test1[0], 'Diffie-Hellman full exchange #1')
         self.assertEqual(test1[1], 'dhm_do_dhm')

tests/scripts/test_psa_constant_names.py

@@ -453,7 +453,7 @@ def main():
     tests.run_all(inputs)
     tests.report(sys.stdout)
     if tests.errors:
-        exit(1)
+        sys.exit(1)
 
 if __name__ == '__main__':
     main()