Repository: asottile/pyupgrade

Issue:
universal_newlines conversion can lead to doubled "text"
If for some reason `universal_newlines` and `text` are both passed to a `subprocess.run()`, replacing the first with the latter causes `text` to be doubled. I suggest to in such case remove `universal_newlines`, if `text` has priority anyway with Python 3.6 and later.

Discussion:
asottile: I think it should maybe just skip in this case (and with `**kwargs`) -- in OP's example it would have broken `their_subprocess_wrapper(universal_newlines=True)`

thoughts?

Patch:
diff --git a/pyupgrade/_plugins/subprocess_run.py b/pyupgrade/_plugins/subprocess_run.py
index 48affbe..53cc03d 100644
--- a/pyupgrade/_plugins/subprocess_run.py
+++ b/pyupgrade/_plugins/subprocess_run.py
@@ -79,6 +79,7 @@ def visit_Call(
         stdout_idx = None
         stderr_idx = None
         universal_newlines_idx = None
+        skip_universal_newlines_rewrite = False
         for n, keyword in enumerate(node.keywords):
             if keyword.arg == 'stdout' and is_name_attr(
                 keyword.value,
@@ -96,7 +97,12 @@ def visit_Call(
                 stderr_idx = n
             elif keyword.arg == 'universal_newlines':
                 universal_newlines_idx = n
-        if universal_newlines_idx is not None:
+            elif keyword.arg == 'text' or keyword.arg is None:
+                skip_universal_newlines_rewrite = True
+        if (
+                universal_newlines_idx is not None and
+                not skip_universal_newlines_rewrite
+        ):
             func = functools.partial(
                 _replace_universal_newlines_with_text,
                 arg_idx=len(node.args) + universal_newlines_idx,


Test patch:
diff --git a/tests/features/universal_newlines_to_text_test.py b/tests/features/universal_newlines_to_text_test.py
index 2e7e298..e7acd9e 100644
--- a/tests/features/universal_newlines_to_text_test.py
+++ b/tests/features/universal_newlines_to_text_test.py
@@ -27,6 +27,26 @@ from pyupgrade._main import _fix_plugins
             (3, 7),
             id='universal_newlines not used',
         ),
+        pytest.param(
+            'import subprocess\n'
+            'subprocess.run(\n'
+            '   ["foo"],\n'
+            '   text=True,\n'
+            '   universal_newlines=True\n'
+            ')\n',
+            (3, 7),
+            id='both text and universal_newlines',
+        ),
+        pytest.param(
+            'import subprocess\n'
+            'subprocess.run(\n'
+            '   ["foo"],\n'
+            '   universal_newlines=True,\n'
+            '   **kwargs,\n'
+            ')\n',
+            (3, 7),
+            id='both **kwargs and universal_newlines',
+        ),
     ),
 )
 def test_fix_universal_newlines_to_text_noop(s, version):