Fixes for linting of new style logging format. (#2713)

The number of arguments was not handled properly, leading to an always successful check. See new tests for specific cases this fixes.

5 files changed, 80 insertions, 20 deletions

diff --git a/CONTRIBUTORS.txt b/CONTRIBUTORS.txt
index a479e9461..448eb880d 100644
--- a/CONTRIBUTORS.txt
+++ b/CONTRIBUTORS.txt
@@ -265,3 +265,5 @@ contributors:
 * Nicolas Dickreuter
 
 * Pascal Corpet
+
+* Svetoslav Neykov: contributor
diff --git a/ChangeLog b/ChangeLog
index 60de7a5d4..18703237f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -110,6 +110,12 @@ Release date: TBA
 
  * Add a new option 'check-str-concat-over-line-jumps' to check 'implicit-str-concat-in-sequence'
 
+* Fixes for the new style logging format linter.
+
+  The number of arguments was not handled properly, leading to an always
+  successful check.
+
+
 What's New in Pylint 2.2.2?
 ===========================
 
diff --git a/pylint/checkers/logging.py b/pylint/checkers/logging.py
index 442cf0ed9..13646cdce 100644
--- a/pylint/checkers/logging.py
+++ b/pylint/checkers/logging.py
@@ -286,8 +286,8 @@ class LoggingChecker(checkers.BaseChecker):
         """
         num_args = _count_supplied_tokens(node.args[format_arg + 1 :])
         if not num_args:
-            # If no args were supplied, then all format strings are valid -
-            # don't check any further.
+            # If no args were supplied the string is not interpolated and can contain
+            # formatting characters - it's used verbatim. Don't check any further.
             return
         format_string = node.args[format_arg].value
         if not isinstance(format_string, str):
@@ -305,12 +305,16 @@ class LoggingChecker(checkers.BaseChecker):
                         # special keywords - out of scope.
                         return
                 elif self._format_style == "new":
-                    keys, num_args, manual_pos_arg = utils.parse_format_method_string(
+                    keyword_arguments, implicit_pos_args, explicit_pos_args = utils.parse_format_method_string(
                         format_string
                     )
 
-                    kargs = len(set(k for k, l in keys if not isinstance(k, int)))
-                    required_num_args = kargs + num_args + manual_pos_arg
+                    keyword_args_cnt = len(
+                        set(k for k, l in keyword_arguments if not isinstance(k, int))
+                    )
+                    required_num_args = (
+                        keyword_args_cnt + implicit_pos_args + explicit_pos_args
+                    )
             except utils.UnsupportedFormatCharacter as ex:
                 char = format_string[ex.index]
                 self.add_message(
diff --git a/pylint/checkers/utils.py b/pylint/checkers/utils.py
index d6083d4ef..3cc292115 100644
--- a/pylint/checkers/utils.py
+++ b/pylint/checkers/utils.py
@@ -571,31 +571,31 @@ def parse_format_method_string(
 ) -> Tuple[List[Tuple[str, List[Tuple[bool, str]]]], int, int]:
     """
     Parses a PEP 3101 format string, returning a tuple of
-    (keys, num_args, manual_pos_arg),
-    where keys is the set of mapping keys in the format string, num_args
+    (keyword_arguments, implicit_pos_args_cnt, explicit_pos_args),
+    where keyword_arguments is the set of mapping keys in the format string, implicit_pos_args_cnt
     is the number of arguments required by the format string and
-    manual_pos_arg is the number of arguments passed with the position.
+    explicit_pos_args is the number of arguments passed with the position.
     """
-    keys = []
-    num_args = 0
-    manual_pos_arg = set()
+    keyword_arguments = []
+    implicit_pos_args_cnt = 0
+    explicit_pos_args = set()
     for name in collect_string_fields(format_string):
         if name and str(name).isdigit():
-            manual_pos_arg.add(str(name))
+            explicit_pos_args.add(str(name))
         elif name:
             keyname, fielditerator = split_format_field_names(name)
             if isinstance(keyname, numbers.Number):
                 # In Python 2 it will return long which will lead
                 # to different output between 2 and 3
-                manual_pos_arg.add(str(keyname))
+                explicit_pos_args.add(str(keyname))
                 keyname = int(keyname)
             try:
-                keys.append((keyname, list(fielditerator)))
+                keyword_arguments.append((keyname, list(fielditerator)))
             except ValueError:
                 raise IncompleteFormatString()
         else:
-            num_args += 1
-    return keys, num_args, len(manual_pos_arg)
+            implicit_pos_args_cnt += 1
+    return keyword_arguments, implicit_pos_args_cnt, len(explicit_pos_args)
 
 
 def is_attr_protected(attrname: str) -> bool:
diff --git a/pylint/test/unittest_checker_logging.py b/pylint/test/unittest_checker_logging.py
index a1c111c9e..8fb4cae93 100644
--- a/pylint/test/unittest_checker_logging.py
+++ b/pylint/test/unittest_checker_logging.py
@@ -63,15 +63,63 @@ class TestLoggingModuleDetection(CheckerTestCase):
         with self.assertAddsMessages(Message("logging-not-lazy", node=stmts[1])):
             self.checker.visit_call(stmts[1])
 
-    @set_config(logging_format_style="new")
-    def test_brace_format_style(self):
+    def _assert_brace_format_no_messages(self, stmt):
         stmts = astroid.extract_node(
             """
         import logging #@
-        logging.error('{}', 1) #@
-        """
+        logging.error<placeholder> #@
+        """.replace(
+                "<placeholder>", stmt
+            )
         )
         self.checker.visit_module(None)
         self.checker.visit_import(stmts[0])
         with self.assertNoMessages():
             self.checker.visit_call(stmts[1])
+
+    def _assert_brace_format_message(self, msg, stmt):
+        stmts = astroid.extract_node(
+            """
+        import logging #@
+        logging.error<placeholder> #@
+        """.replace(
+                "<placeholder>", stmt
+            )
+        )
+        self.checker.visit_module(None)
+        self.checker.visit_import(stmts[0])
+        with self.assertAddsMessages(Message(msg, node=stmts[1])):
+            self.checker.visit_call(stmts[1])
+
+    def _assert_brace_format_too_few_args(self, stmt):
+        self._assert_brace_format_message("logging-too-few-args", stmt)
+
+    def _assert_brace_format_too_many_args(self, stmt):
+        self._assert_brace_format_message("logging-too-many-args", stmt)
+
+    @set_config(logging_format_style="new")
+    def test_brace_format_style_matching_arguments(self):
+        self._assert_brace_format_no_messages("('constant string')")
+        self._assert_brace_format_no_messages("('{}')")
+        self._assert_brace_format_no_messages("('{}', 1)")
+        self._assert_brace_format_no_messages("('{0}', 1)")
+        self._assert_brace_format_no_messages("('{named}', {'named': 1})")
+        self._assert_brace_format_no_messages("('{} {named}', 1, {'named': 1})")
+        self._assert_brace_format_no_messages("('{0} {named}', 1, {'named': 1})")
+
+    @set_config(logging_format_style="new")
+    def test_brace_format_style_too_few_args(self):
+        self._assert_brace_format_too_few_args("('{}, {}', 1)")
+        self._assert_brace_format_too_few_args("('{0}, {1}', 1)")
+        self._assert_brace_format_too_few_args("('{named1}, {named2}', {'named1': 1})")
+        self._assert_brace_format_too_few_args("('{0}, {named}', 1)")
+        self._assert_brace_format_too_few_args("('{}, {named}', {'named': 1})")
+        self._assert_brace_format_too_few_args("('{0}, {named}', {'named': 1})")
+
+    @set_config(logging_format_style="new")
+    def test_brace_format_style_not_enough_arguments(self):
+        self._assert_brace_format_too_many_args("('constant string', 1, 2)")
+        self._assert_brace_format_too_many_args("('{}', 1, 2)")
+        self._assert_brace_format_too_many_args("('{0}', 1, 2)")
+        self._assert_brace_format_too_many_args("('{}, {named}', 1, 2, {'named': 1})")
+        self._assert_brace_format_too_many_args("('{0}, {named}', 1, 2, {'named': 1})")