Just: let argparse own the options this script acts on

Hand-sorting every argument was justified while `--all-checks` was both a
flag and an assignment, which argparse cannot express. Naming the offer
separately removed that, and the loop was then keeping three silent
mistakes alive: `--codeql built` made `built` a test path, and a bare
`--codeql` or `--extra-check` was forwarded to `codeql test run` to fail
there instead of here.

argparse takes those three options; the shape test stays for the rest,
which belongs to `codeql test run` and is forwarded untouched. Errors are
routed back through `error` so they still arrive in a just banner.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Author: redsun82

misc/just/codeql_test_run.py

@@ -10,6 +10,7 @@
 the caller supplies the switch, so the two are separate options rather than one.
 """
 
+import argparse
 import dataclasses
 import os
 import re
@@ -23,7 +24,6 @@
 CMD_END = os.environ.get("CMD_END", "")
 SEMMLE_CODE = os.environ.get("SEMMLE_CODE")
 
-EXTRA_CHECK_PREFIX = "--extra-check="
 ENV_RE = re.compile(r"^[A-Z_][A-Z_0-9]*=.*$")
 
 
@@ -41,15 +41,37 @@ def error(message):
     print(f"{ERROR}{message}", file=sys.stderr)
 
 
+class _Parser(argparse.ArgumentParser):
+    """An `argparse` parser that fails the way the rest of this script does.
+
+    The default reports to `stderr` in its own format and exits 2, which would arrive
+    in a `just` banner unprefixed and alongside a usage line naming this script rather
+    than the recipe the caller actually typed.
+    """
+
+    def error(self, message):
+        error(message)
+        raise SystemExit(1)
+
+
+def build_parser():
+    # `+` can be an option string only because it is also a prefix character. `-h` and
+    # `--help` are left unclaimed so that they reach `codeql test run`.
+    parser = _Parser(add_help=False, allow_abbrev=False, prefix_chars="-+")
+    parser.add_argument("--codeql")
+    parser.add_argument("--extra-check", action="append", dest="extra_checks")
+    parser.add_argument("--all-checks", "+", action="store_true", dest="all")
+    return parser
+
+
 @dataclasses.dataclass
 class Arguments:
     """A command line sorted into the kinds that are handled differently.
 
-    Sorted by hand rather than by `argparse`, which could own the three options named
-    below but none of the rest: every flag not named here belongs to `codeql test run`
-    and has to survive untouched, `+` is not a spelling `argparse` has, and `CPUS=4` and
-    `ql/test` are both positionals told apart only by shape. Handing it the half it can
-    take would leave this loop in place for the other half.
+    `argparse` owns the three options this script acts on itself. Everything else
+    belongs to `codeql test run` and has to survive untouched, which is what
+    `parse_known_args` hands back, and what is sorted by shape below: a test path and
+    a `CPUS=4` are both positionals, told apart only by how they look.
     """
 
     codeql: str = dataclasses.field(
@@ -62,19 +84,19 @@ class Arguments:
     extra_checks: list = dataclasses.field(default_factory=list)
 
     def parse(self, argv):
-        """Sort arguments into tests, flags and environment assignments."""
-        for arg in argv:
-            if not arg:
-                # an empty argument can come from a caller interpolating an unset
-                # variable
-                continue
-            if arg.startswith(EXTRA_CHECK_PREFIX):
-                self.extra_checks.append(arg[len(EXTRA_CHECK_PREFIX) :])
-            elif arg.startswith("--codeql="):
-                self.codeql = arg.split("=", 1)[1]
-            elif arg in ("+", "--all-checks"):
-                self.all = True
-            elif arg.startswith("-"):
+        """Sort arguments into tests, flags and environment assignments.
+
+        Additive, because `main` parses a second time to apply the checks held back
+        until `--all-checks` asked for them.
+        """
+        # An empty argument can come from a caller interpolating an unset variable.
+        known, rest = build_parser().parse_known_args([arg for arg in argv if arg])
+        if known.codeql:
+            self.codeql = known.codeql
+        self.all = self.all or known.all
+        self.extra_checks += known.extra_checks or []
+        for arg in rest:
+            if arg.startswith("-"):
                 self.flags.append(arg)
             elif ENV_RE.match(arg):
                 self.env.append(arg)

misc/just/test_codeql_test_run.py

@@ -58,6 +58,12 @@ def test_an_extra_check_is_held_back_until_it_is_asked_for(self):
         self.assertEqual(held.flags, [])
         self.assertFalse(held.all)
 
+    def test_a_double_dash_hands_everything_after_it_to_codeql(self):
+        # Standard `--`: past it, an option is the caller's business and not ours.
+        args = sorted_args("--", "--codeql=built")
+        self.assertEqual(args.codeql, "host")
+        self.assertIn("--codeql=built", args.flags)
+
     def test_an_empty_argument_is_ignored(self):
         # One of these comes of a caller interpolating a variable that was never set.
         self.assertEqual(sorted_args("", "test").tests, ["test"])
@@ -74,7 +80,9 @@ def test_an_assignment_whose_value_contains_a_space_stays_whole(self):
         self.assertEqual(sorted_args("EXTRA=a b").env, ["EXTRA=a b"])
 
     def test_sorts_a_whole_command_line_at_once(self):
-        args = sorted_args("-j2", "CPUS=4", "ql/test", "+", "--extra-check=--check-diff")
+        args = sorted_args(
+            "-j2", "CPUS=4", "ql/test", "+", "--extra-check=--check-diff"
+        )
         self.assertEqual(args.flags, ["-j2"])
         self.assertEqual(args.env, ["CPUS=4"])
         self.assertEqual(args.tests, ["ql/test"])