Add B034: re.sub/subn/split must pass flags/count/maxsplit as keyword…

… arguments (#398) * Add B034: re.sub/subn/split must pass flags/count/maxsplit as keyword arguments. * remove <3.8 check accidentally added back * Apply suggestions from code review Co-authored-by: Jelle Zijlstra <[email protected]> * update column in testcase * improved wording --------- Co-authored-by: Jelle Zijlstra <[email protected]>
PyCQA · Jul 10, 2023 · dea2e00 · dea2e00
1 parent f1c391a
commit dea2e00
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 1 deletion.
diff --git a/README.rst b/README.rst
@@ -188,6 +188,8 @@ second usage. Save the result to a list if the result is needed multiple times.
 
 **B033**: Sets should not contain duplicate items. Duplicate items will be replaced with a single item at runtime.
 
+**B034**: Calls to `re.sub`, `re.subn` or `re.split` should pass `flags` or `count`/`maxsplit` as keyword arguments. It is commonly assumed that `flags` is the third positional parameter, forgetting about `count`/`maxsplit`, since many other `re` module functions are of the form `f(pattern, string, flags)`.
+
 Opinionated warnings
 ~~~~~~~~~~~~~~~~~~~~
 
@@ -338,6 +340,7 @@ Unreleased
 * Fix a crash and several test failures on Python 3.12, all relating to the B907
   check.
 * Declare support for Python 3.12.
+* Add B034: re.sub/subn/split must pass flags/count/maxsplit as keyword arguments.
 
 23.6.5
 ~~~~~~

diff --git a/bugbear.py b/bugbear.py
@@ -423,8 +423,9 @@ def visit_Call(self, node):
 
             self.check_for_b026(node)
 
-        self.check_for_b905(node)
         self.check_for_b028(node)
+        self.check_for_b034(node)
+        self.check_for_b905(node)
         self.generic_visit(node)
 
     def visit_Module(self, node):
@@ -1400,6 +1401,27 @@ def check_for_b033(self, node):
             else:
                 seen.add(elt.value)
 
+    def check_for_b034(self, node: ast.Call):
+        if not isinstance(node.func, ast.Attribute):
+            return
+        if not isinstance(node.func.value, ast.Name) or node.func.value.id != "re":
+            return
+
+        def check(num_args, param_name):
+            if len(node.args) > num_args:
+                self.errors.append(
+                    B034(
+                        node.args[num_args].lineno,
+                        node.args[num_args].col_offset,
+                        vars=(node.func.attr, param_name),
+                    )
+                )
+
+        if node.func.attr in ("sub", "subn"):
+            check(3, "count")
+        elif node.func.attr == "split":
+            check(2, "maxsplit")
+
 
 def compose_call_path(node):
     if isinstance(node, ast.Attribute):
@@ -1804,6 +1826,13 @@ def visit_Lambda(self, node):
     )
 )
 
+B034 = Error(
+    message=(
+        "B034 {} should pass `{}` and `flags` as keyword arguments to avoid confusion"
+        " due to unintuitive argument positions."
+    )
+)
+
 # Warnings disabled by default.
 B901 = Error(
     message=(

diff --git a/tests/b034.py b/tests/b034.py
@@ -0,0 +1,30 @@
+import re
+from re import sub
+
+# error
+re.sub("a", "b", "aaa", re.IGNORECASE)
+re.sub("a", "b", "aaa", 5)
+re.sub("a", "b", "aaa", 5, re.IGNORECASE)
+re.subn("a", "b", "aaa", re.IGNORECASE)
+re.subn("a", "b", "aaa", 5)
+re.subn("a", "b", "aaa", 5, re.IGNORECASE)
+re.split(" ", "a a a a", re.I)
+re.split(" ", "a a a a", 2)
+re.split(" ", "a a a a", 2, re.I)
+
+# okay
+re.sub("a", "b", "aaa")
+re.sub("a", "b", "aaa", flags=re.IGNORECASE)
+re.sub("a", "b", "aaa", count=5)
+re.sub("a", "b", "aaa", count=5, flags=re.IGNORECASE)
+re.subn("a", "b", "aaa")
+re.subn("a", "b", "aaa", flags=re.IGNORECASE)
+re.subn("a", "b", "aaa", count=5)
+re.subn("a", "b", "aaa", count=5, flags=re.IGNORECASE)
+re.split(" ", "a a a a", flags=re.I)
+re.split(" ", "a a a a", maxsplit=2)
+re.split(" ", "a a a a", maxsplit=2, flags=re.I)
+
+
+# not covered
+sub("a", "b", "aaa", re.IGNORECASE)
diff --git a/tests/test_bugbear.py b/tests/test_bugbear.py
@@ -42,6 +42,7 @@
     B031,
     B032,
     B033,
+    B034,
     B901,
     B902,
     B903,
@@ -502,6 +503,23 @@ def test_b033(self):
         )
         self.assertEqual(errors, expected)
 
+    def test_b034(self):
+        filename = Path(__file__).absolute().parent / "b034.py"
+        bbc = BugBearChecker(filename=str(filename))
+        errors = list(bbc.run())
+        expected = self.errors(
+            B034(5, 24, vars=("sub", "count")),
+            B034(6, 24, vars=("sub", "count")),
+            B034(7, 24, vars=("sub", "count")),
+            B034(8, 25, vars=("subn", "count")),
+            B034(9, 25, vars=("subn", "count")),
+            B034(10, 25, vars=("subn", "count")),
+            B034(11, 25, vars=("split", "maxsplit")),
+            B034(12, 25, vars=("split", "maxsplit")),
+            B034(13, 25, vars=("split", "maxsplit")),
+        )
+        self.assertEqual(errors, expected)
+
     def test_b908(self):
         filename = Path(__file__).absolute().parent / "b908.py"
         bbc = BugBearChecker(filename=str(filename))