Merge "Add C300 hacking check for set literal in logging calls"

cyborg/hacking/checks.py

@@ -0,0 +1,101 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import ast
+import re
+
+
+NAMED_PLACEHOLDER_RE = re.compile(r"%\([^)]+\)[a-zA-Z%]")
+
+LOG_METHODS = frozenset(
+    (
+        "debug",
+        "info",
+        "warning",
+        "error",
+        "critical",
+        "exception",
+        "log",
+    )
+)
+
+
+def _flatten_str_chain(node):
+    """Extract a string from an AST node or Add-concatenation chain.
+
+    Returns the string value if the node is a string constant or a
+    chain of string constants joined with ``+``.  Returns ``None``
+    for anything else (variables, f-strings, etc.).
+    """
+    parts = []
+
+    def _visit(node):
+        if isinstance(node, ast.Constant) and isinstance(node.value, str):
+            parts.append(node.value)
+            return True
+        if isinstance(node, ast.BinOp) and isinstance(node.op, ast.Add):
+            return _visit(node.left) and _visit(node.right)
+        return False
+
+    if not _visit(node):
+        return None
+    return "".join(parts)
+
+
+class CheckSetLiteralInLogging(ast.NodeVisitor):
+    """C300: set literal passed where dict expected in logging call.
+
+    Detects ``LOG.info("%(key)s", {"key", val})`` where a set literal
+    (comma) was used instead of a dict literal (colon).  This causes
+    ``TypeError: format requires a mapping`` at runtime.
+    """
+
+    name = "check_set_literal_in_logging"
+    version = "0.1"
+
+    CHECK_DESC = (
+        "C300 set literal passed where dict expected for %-format logging"
+    )
+
+    def __init__(self, tree, filename):
+        self._tree = tree
+        self._errors = []
+
+    def run(self):
+        self.visit(self._tree)
+        return self._errors
+
+    def add_error(self, node, message=None):
+        message = message or self.CHECK_DESC
+        self._errors.append(
+            (node.lineno, node.col_offset, message, type(self))
+        )
+
+    def visit_Call(self, node):
+        if (
+            isinstance(node.func, ast.Attribute)
+            and node.func.attr in LOG_METHODS
+        ):
+            # .log(level, msg, *args) vs .info(msg, *args)
+            if node.func.attr == "log":
+                fmt_idx, dict_idx = 1, 2
+            else:
+                fmt_idx, dict_idx = 0, 1
+
+            if len(node.args) >= dict_idx + 1 and isinstance(
+                node.args[dict_idx], ast.Set
+            ):
+                fmt_str = _flatten_str_chain(node.args[fmt_idx])
+                if fmt_str and NAMED_PLACEHOLDER_RE.search(fmt_str):
+                    self.add_error(node.args[dict_idx])
+
+        self.generic_visit(node)

cyborg/tests/unit/test_hacking.py

@@ -0,0 +1,151 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import textwrap
+
+from unittest import mock
+
+import pycodestyle
+
+from cyborg.hacking import checks
+from cyborg.tests import base
+
+
+class HackingTestCase(base.TestCase):
+    @mock.patch(
+        'pycodestyle._checks',
+        {'physical_line': {}, 'logical_line': {}, 'tree': {}},
+    )
+    def _run_check(self, code, checker, filename=None):
+        pycodestyle.register_check(checker)
+        lines = textwrap.dedent(code).strip().splitlines(True)
+        checker = pycodestyle.Checker(filename=filename, lines=lines)
+        with mock.patch('pycodestyle.StandardReport.get_file_results'):
+            checker.check_all()
+        checker.report._deferred_print.sort()
+        return checker.report._deferred_print
+
+    def _assert_has_errors(
+        self, code, checker, expected_errors=None, filename=None
+    ):
+        actual_errors = [
+            e[:3] for e in self._run_check(code, checker, filename)
+        ]
+        self.assertEqual(expected_errors or [], actual_errors)
+
+    def _assert_has_no_errors(self, code, checker, filename=None):
+        self._assert_has_errors(code, checker, filename=filename)
+
+    def test_set_literal_attr_call(self):
+        checker = checks.CheckSetLiteralInLogging
+        code = """
+            import logging
+            LOG = logging.getLogger(__name__)
+            LOG.info("%(fruit)s is ready", {"fruit", fruit})
+        """
+        self._assert_has_errors(
+            code, checker, expected_errors=[(3, 31, "C300")]
+        )
+
+    def test_set_literal_log_method(self):
+        checker = checks.CheckSetLiteralInLogging
+        code = """
+            import logging
+            LOG = logging.getLogger(__name__)
+            LOG.log(logging.INFO, "%(fruit)s", {"fruit", fruit})
+        """
+        self._assert_has_errors(
+            code, checker, expected_errors=[(3, 35, "C300")]
+        )
+
+    def test_set_literal_module_call(self):
+        checker = checks.CheckSetLiteralInLogging
+        code = """
+            import logging
+            logging.info("%(fruit)s is ready", {"fruit", fruit})
+        """
+        self._assert_has_errors(
+            code, checker, expected_errors=[(2, 35, "C300")]
+        )
+
+    def test_set_literal_multiple_placeholders(self):
+        checker = checks.CheckSetLiteralInLogging
+        code = """
+            import logging
+            LOG = logging.getLogger(__name__)
+            LOG.info("%(a)s %(b)s", {"a", a, "b", b})
+        """
+        self._assert_has_errors(
+            code, checker, expected_errors=[(3, 24, "C300")]
+        )
+
+    def test_set_literal_concat_format_string(self):
+        checker = checks.CheckSetLiteralInLogging
+        code = """
+            import logging
+            LOG = logging.getLogger(__name__)
+            LOG.info("%(fruit)s" + " is ready", {"fruit", fruit})
+        """
+        self._assert_has_errors(
+            code, checker, expected_errors=[(3, 36, "C300")]
+        )
+
+    def test_set_literal_multiline(self):
+        checker = checks.CheckSetLiteralInLogging
+        code = """
+            import logging
+            LOG = logging.getLogger(__name__)
+            LOG.info(
+                "%(fruit)s is ready",
+                {"fruit", fruit},
+            )
+        """
+        self._assert_has_errors(
+            code, checker, expected_errors=[(5, 4, "C300")]
+        )
+
+    def test_dict_literal_no_error(self):
+        checker = checks.CheckSetLiteralInLogging
+        code = """
+            import logging
+            LOG = logging.getLogger(__name__)
+            LOG.info("%(fruit)s is ready", {"fruit": fruit})
+        """
+        self._assert_has_no_errors(code, checker)
+
+    def test_positional_placeholder_no_error(self):
+        checker = checks.CheckSetLiteralInLogging
+        code = """
+            import logging
+            LOG = logging.getLogger(__name__)
+            LOG.info("items: %s", {a, b})
+        """
+        self._assert_has_no_errors(code, checker)
+
+    def test_no_placeholders_no_error(self):
+        checker = checks.CheckSetLiteralInLogging
+        code = """
+            import logging
+            LOG = logging.getLogger(__name__)
+            LOG.info("no placeholders", {"a", b})
+        """
+        self._assert_has_no_errors(code, checker)
+
+    def test_variable_format_string_no_error(self):
+        checker = checks.CheckSetLiteralInLogging
+        code = """
+            import logging
+            LOG = logging.getLogger(__name__)
+            msg = "%(fruit)s is ready"
+            LOG.info(msg, {"fruit", fruit})
+        """
+        self._assert_has_no_errors(code, checker)

test-requirements.txt

@@ -5,3 +5,4 @@ stestr>=2.2.0 # Apache-2.0
 testtools>=2.4.0 # MIT
 Pygments>=2.2.0 # BSD license
 WebTest>=2.0.27 # MIT
+pycodestyle>=2.11.0 # MIT

tox.ini

@@ -109,7 +109,7 @@ allowlist_externals = rm
 
 [flake8]
 # We only enable the hacking (H) checks; ruff handles everything else
-select = H
+select = H,C
 # H301 ruff-I enforces one-symbol-per-line where needed
 # H306 ruff-I (rule I001) handles import ordering; its sort key (by
 #      from-module path) differs from H306's (fully-qualified name),
@@ -119,3 +119,9 @@ select = H
 ignore = H301,H306,H404,H405
 show-source = True
 exclude=.venv,.git,.tox,dist,doc,*lib/python*,*egg,build,releasenotes,*sqlalchemy/alembic/versions/*
+
+[flake8:local-plugins]
+extension =
+  C300 = checks:CheckSetLiteralInLogging
+paths =
+  ./cyborg/hacking