Reject multiple occurrences of optional CLI arguments with values

Ref. eng/recordflux/RecordFlux#1342

CHANGELOG.md

@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+
+### Changed
+
+- Reject duplicate optional arguments in `rflx` CLI (eng/recordflux/RecordFlux#1342)
+
 ## [0.12.0] - 2023-08-22
 
 ### Added
@@ -354,6 +360,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [0.1.0] - 2019-05-14
 
+[Unreleased]: https://github.com/AdaCore/RecordFlux/compare/v0.12.0...HEAD
 [0.12.0]: https://github.com/AdaCore/RecordFlux/compare/v0.11.1...v0.12.0
 [0.11.1]: https://github.com/AdaCore/RecordFlux/compare/v0.11.0...v0.11.1
 [0.11.0]: https://github.com/AdaCore/RecordFlux/compare/v0.10.0...v0.11.0

rflx/cli.py

@@ -61,6 +61,74 @@ def ide(string: str) -> IDE:
             raise ValueError from None
 
 
+class UniqueStore(argparse.Action):
+    """
+    Action which allows at most one occurrence of the given option-value pair.
+
+    The argparse module doesn't check for duplicate optional arguments having action 'store' (the
+    default). However, such usage doesn't make sense (the value of the last matching argument
+    remains, others are overwritten and ignored). This custom action class mimics the 'store' action
+    in every other respect, but additionally prohibits such usage.
+
+    Note: In argparse the 'store' action logic is implemented in argparse._StoreAction. However,
+    since that is a protected class we can't directly inherit from it.
+    """
+
+    def __init__(  # type: ignore[no-untyped-def] # noqa: PLR0913
+        self,
+        option_strings,
+        dest,
+        nargs=None,
+        const=None,
+        default=None,
+        type=None,  # noqa: A002
+        choices=None,
+        required=False,
+        help=None,  # noqa: A002
+        metavar=None,
+    ):
+        # Keep track whether the option has been set already
+        self.is_set = False
+        # Perform the 'store' action-related checks.
+        # Note: This should be equivalent to the logic in argparse._StoreAction__init__(..).
+        if nargs == 0:
+            raise ValueError(
+                "nargs for store actions must be != 0; if you "
+                "have nothing to store, actions such as store "
+                "true or store const may be more appropriate",
+            )
+        if const is not None and nargs != argparse.OPTIONAL:
+            raise ValueError("nargs must be %r to supply const" % argparse.OPTIONAL)
+
+        super().__init__(
+            option_strings=option_strings,
+            dest=dest,
+            nargs=nargs,
+            const=const,
+            default=default,
+            type=type,
+            choices=choices,
+            required=required,
+            help=help,
+            metavar=metavar,
+        )
+
+    def __call__(  # type: ignore [no-untyped-def]
+        self,
+        parser,
+        namespace,
+        values,
+        option_string=None,
+    ):
+        if self.is_set:
+            parser.error(f"{option_string} appears several times")
+        else:
+            self.is_set = True
+            # Perform the 'store' action.
+            # Note: This should be equivalent to the logic in argparse._StoreAction__call__(..).
+            setattr(namespace, self.dest, values)
+
+
 def main(  # noqa: PLR0915
     argv: Sequence[str],
 ) -> Union[int, str]:
@@ -79,13 +147,15 @@ def main(  # noqa: PLR0915
     )
     parser.add_argument(
         "--max-errors",
+        action=UniqueStore,
         type=int,
         default=0,
         metavar=("NUM"),
         help="exit after at most NUM errors",
     )
     parser.add_argument(
         "--workers",
+        action=UniqueStore,
         type=int,
         default=cpu_count(),
         metavar=("NUM"),
@@ -113,6 +183,7 @@ def main(  # noqa: PLR0915
     parser_generate.add_argument(
         "-p",
         "--prefix",
+        action=UniqueStore,
         type=str,
         default=DEFAULT_PREFIX,
         help="add prefix to generated packages (default: %(default)s)",
@@ -125,13 +196,15 @@ def main(  # noqa: PLR0915
     )
     parser_generate.add_argument(
         "-d",
+        action=UniqueStore,
         dest="output_directory",
         type=Path,
         default=".",
         help="output directory",
     )
     parser_generate.add_argument(
         "--debug",
+        action=UniqueStore,
         default=None,
         choices=["built-in", "external"],
         help="enable adding of debug output to generated code",
@@ -143,6 +216,7 @@ def main(  # noqa: PLR0915
     )
     parser_generate.add_argument(
         "--integration-files-dir",
+        action=UniqueStore,
         help="directory for the .rfi files",
         type=Path,
     )
@@ -159,6 +233,7 @@ def main(  # noqa: PLR0915
     parser_graph.add_argument(
         "-f",
         "--format",
+        action=UniqueStore,
         type=str,
         default="svg",
         choices=["dot", "jpg", "pdf", "png", "raw", "svg"],
@@ -181,6 +256,7 @@ def main(  # noqa: PLR0915
     )
     parser_graph.add_argument(
         "-d",
+        action=UniqueStore,
         dest="output_directory",
         type=Path,
         default=".",
@@ -211,27 +287,31 @@ def main(  # noqa: PLR0915
     )
     parser_validate.add_argument(
         "-v",
+        action=UniqueStore,
         dest="valid_samples_directory",
         type=Path,
         help="path to the directory containing known valid samples",
         default=None,
     )
     parser_validate.add_argument(
         "-i",
+        action=UniqueStore,
         dest="invalid_samples_directory",
         type=Path,
         help="path to the directory containing known valid samples",
         default=None,
     )
     parser_validate.add_argument(
         "-c",
+        action=UniqueStore,
         dest="checksum_module",
         type=Path,
         help="name of the module containing the checksum functions",
         default=None,
     )
     parser_validate.add_argument(
         "-o",
+        action=UniqueStore,
         dest="output_file",
         type=Path,
         help="path to output file for validation report in JSON format (file must not exist)",
@@ -254,6 +334,7 @@ def main(  # noqa: PLR0915
     )
     parser_validate.add_argument(
         "--target-coverage",
+        action=UniqueStore,
         metavar="PERCENTAGE",
         type=float,
         default=0,
@@ -269,6 +350,7 @@ def main(  # noqa: PLR0915
     )
     parser_install.add_argument(
         "--gnat-studio-dir",
+        action=UniqueStore,
         dest="gnat_studio_dir",
         type=Path,
         help="path to the GNAT Studio settings directory (default: $HOME/.gnatstudio)",
@@ -297,6 +379,7 @@ def main(  # noqa: PLR0915
     )
     parser_iana.add_argument(
         "-d",
+        action=UniqueStore,
         dest="output_directory",
         type=Path,
         default=".",

tests/unit/cli_test.py

@@ -1,10 +1,12 @@
 from __future__ import annotations
 
+import argparse
 import re
+from argparse import ArgumentParser
 from collections.abc import Callable
 from io import TextIOWrapper
 from pathlib import Path
-from typing import Optional
+from typing import ClassVar, Optional
 
 import pytest
 
@@ -629,3 +631,135 @@ def test_requirement_error() -> None:
         match=r'^cli: error: failed parsing requirement "\(<2,>=1\) pydantic"$',
     ):
         cli.Requirement("(<2,>=1) pydantic")
+
+
+class TestDuplicateArgs:
+    """
+    Test the rejection of duplicate options in the CLI.
+
+    Concretely, such options that have action 'store' (the default action) are tested here. The test
+    data consists of a full list of call arguments (optionally split to prefix and suffix) and the
+    duplicate options. The actual values for the arguments do not matter in these tests, but must
+    have the expected data type.
+    """
+
+    call_main_prefix: ClassVar[list[str]] = [
+        "rflx",
+        "--max-errors",
+        "0",
+        "--workers",
+        "3",
+    ]
+    # Note: 'check' is used below as a concrete rflx subcommand. However, the focus in this set of
+    # test data is on the rflx common arguments.
+    call_main_suffix: ClassVar[list[str]] = [
+        "check",
+        "SPECIFICATION_FILE",
+    ]
+    call_generate: ClassVar[list[str]] = [
+        "rflx",
+        "generate",
+        "SPECIFICATION_FILE",
+        "-p",
+        "PREFIX",
+        "-d",
+        "OUTPUT_DIRECTORY",
+        "--debug",
+        "built-in",
+        "--integration-files-dir",
+        "INTEGRATION_FILES_DIR",
+    ]
+    call_graph: ClassVar[list[str]] = [
+        "rflx",
+        "graph",
+        "SPECIFICATION_FILE",
+        "-f",
+        "jpg",
+        "-d",
+        "OUTPUT_DIRECTORY",
+    ]
+    call_validate: ClassVar[list[str]] = [
+        "rflx",
+        "validate",
+        "SPECIFICATION_FILE",
+        "MESSAGE_IDENTIFIER",
+        "-v",
+        "VALID_SAMPLES_DIRECTORY",
+        "-i",
+        "INVALID_SAMPLES_DIRECTORY",
+        "-c",
+        "CHECKSUM_MODULE",
+        "-o",
+        "OUTPUT_FILE",
+        "--target-coverage",
+        "50",
+    ]
+    call_install: ClassVar[list[str]] = [
+        "rflx",
+        "install",
+        "gnatstudio",
+        "--gnat-studio-dir",
+        "GNAT_STUDIO_DIR",
+    ]
+    call_convert_iana: ClassVar[list[str]] = [
+        "rflx",
+        "convert",
+        "iana",
+        "-d",
+        "SOME_PATH",
+    ]
+
+    class ArgParseError(Exception):
+        """Used to mock the command line parsing error in argparse."""
+
+    def raise_argparse_error(self, message: str) -> None:
+        raise self.ArgParseError(message)
+
+    @pytest.mark.parametrize(
+        ("call_prefix", "duplicate_option", "duplicate_option_value", "call_suffix"),
+        [
+            (call_main_prefix, "--max-errors", "5", call_main_suffix),
+            (call_main_prefix, "--workers", "10", call_main_suffix),
+            (call_generate, "-p", "OTHER_STR", []),
+            (call_generate, "-d", "OTHER_STR", []),
+            (call_generate, "--debug", "external", []),
+            (call_generate, "--integration-files-dir", "OTHER_STR", []),
+            (call_graph, "-f", "png", []),
+            (call_graph, "-d", "OTHER_STR", []),
+            (call_validate, "-v", "OTHER_STR", []),
+            (call_validate, "-i", "OTHER_STR", []),
+            (call_validate, "-c", "OTHER_STR", []),
+            (call_validate, "-o", "OTHER_STR", []),
+            (call_validate, "--target-coverage", "75", []),
+            (call_install, "--gnat-studio-dir", "OTHER_STR", []),
+            (call_convert_iana, "-d", "OTHER_STR", []),
+        ],
+    )
+    def test_duplicate_args(
+        self,
+        monkeypatch: pytest.MonkeyPatch,
+        call_prefix: list[str],
+        duplicate_option: str,
+        duplicate_option_value: str,
+        call_suffix: list[str],
+    ) -> None:
+        monkeypatch.setattr(ArgumentParser, "error", lambda _s, m: self.raise_argparse_error(m))
+        args = [*call_prefix, duplicate_option, duplicate_option_value, *call_suffix]
+        with pytest.raises(self.ArgParseError, match=f"^{duplicate_option} appears several times$"):
+            cli.main(args)
+
+    def test_uniquestore_nargs_is_0(self) -> None:
+        parser = argparse.ArgumentParser()
+        message = (
+            "nargs for store actions must be != 0; if you "
+            "have nothing to store, actions such as store "
+            "true or store const may be more appropriate"
+        )
+        with pytest.raises(ValueError, match=f"^{message}$"):
+            parser.add_argument("-x", action=cli.UniqueStore, nargs=0)
+
+    def test_uniquestore_bad_const(self) -> None:
+        parser = argparse.ArgumentParser()
+        message = "nargs must be '\\?' to supply const"
+        with pytest.raises(ValueError, match=f"^{message}$"):
+            parser.add_argument("-x", action=cli.UniqueStore, const=0)