diff --git a/toolchain/mfc/args.py b/toolchain/mfc/args.py index 0169a2cdf..548de96de 100644 --- a/toolchain/mfc/args.py +++ b/toolchain/mfc/args.py @@ -12,6 +12,7 @@ from .cli.argparse_gen import generate_parser from .cli.commands import COMMAND_ALIASES, MFC_CLI_SCHEMA from .common import MFCException +from .printer import cons from .state import MFCConfig from .user_guide import ( is_first_time_user, @@ -53,6 +54,52 @@ def _handle_enhanced_help(args_list): return None +def _multi_value_flags(command_name: str): + """Yield (flag_tokens, dest) for every list-valued option of a command. + + A list-valued option is one declared with nargs="+" or "*": argparse stores + these with the default "store" action, so a second occurrence of the flag + REPLACES whatever the first one collected instead of appending to it. + Both the command's own arguments and the common sets it includes count. + """ + command = MFC_CLI_SCHEMA.get_command(command_name) + if command is None: + return + + arguments = list(command.arguments) + for set_name in command.include_common: + common_set = MFC_CLI_SCHEMA.get_common_set(set_name) + if common_set is not None: + arguments.extend(common_set.arguments) + + for argument in arguments: + if argument.nargs in ("+", "*"): + yield argument.get_flags(), argument.get_dest() + + +def _warn_on_repeated_multi_value_flags(command_name: str, cli_argv): + """Warn when a list-valued flag was passed more than once. + + `-t` takes a space-separated list, so `-t pre_process -t simulation` does not + append -- argparse keeps only the last occurrence and the earlier targets are + dropped without a word. That is quiet and lands far from its cause: the + generated batch script simply has one fewer step than the user expected. + Repeating a flag appends in many other CLIs, and `-t simulation` on its own is + a perfectly legitimate invocation (restarting from existing data), so nothing + downstream can tell the two apart. Warn here, where we can still see that the + flag was written twice. + """ + for flags, dest in _multi_value_flags(command_name): + occurrences = sum(1 for tok in cli_argv if tok in flags or any(tok.startswith(f"{flag}=") for flag in flags)) + if occurrences > 1: + joined = " / ".join(flags) + cons.print( + f"[yellow]{joined} was given {occurrences} times, but it takes a space-separated list " + f"and only the last occurrence is kept. Earlier values were discarded; " + f"pass them together as e.g. --{dest.replace('_', '-')} A B.[/yellow]" + ) + + def parse(config: MFCConfig): """Parse command line arguments using the CLI schema.""" # Handle enhanced help before argparse @@ -91,6 +138,9 @@ def custom_error(message): args["--"] = sys.argv[extra_index + 1 :] args["targets_explicit"] = any(tok in ("-t", "--targets") for tok in cli_argv) + if attempted_command: + _warn_on_repeated_multi_value_flags(attempted_command, cli_argv) + # Handle --help at top level if args.get("help") and args["command"] is None: print_help() diff --git a/toolchain/mfc/test_args_repeated_flags.py b/toolchain/mfc/test_args_repeated_flags.py new file mode 100644 index 000000000..fd34378de --- /dev/null +++ b/toolchain/mfc/test_args_repeated_flags.py @@ -0,0 +1,132 @@ +"""A repeated list-valued flag must not silently discard the earlier values. + +`-t` is declared with nargs="+", so it takes a space-separated list. Repeating the +flag does not append: argparse's default "store" action keeps only the last +occurrence, so `-t pre_process -t simulation` builds a script containing only +`simulation`. The queued job then runs simulation against an empty restart_data/ +and nothing in the output says a target was dropped. + +These tests pin the warning, and pin that the parse result itself is untouched. +""" + +import sys +from unittest.mock import patch + +from mfc.args import _multi_value_flags, _warn_on_repeated_multi_value_flags, parse +from mfc.cli.argparse_gen import generate_parser +from mfc.cli.commands import MFC_CLI_SCHEMA +from mfc.state import MFCConfig + + +def _warnings_for(command, argv): + """Return the warning strings emitted for `argv` under `command`.""" + emitted = [] + with patch("mfc.args.cons") as printer: + printer.print.side_effect = lambda *a, **k: emitted.append(" ".join(str(x) for x in a)) + _warn_on_repeated_multi_value_flags(command, argv) + return emitted + + +def test_repeated_short_targets_flag_warns(): + """The exact invocation from the report: -t pre_process -t simulation.""" + warnings = _warnings_for("run", ["run", "case.py", "-t", "pre_process", "-t", "simulation"]) + assert len(warnings) == 1 + assert "-t" in warnings[0] + assert "--targets" in warnings[0] + assert "2 times" in warnings[0] + + +def test_repeated_long_targets_flag_warns(): + """--targets is the same option and must be counted alongside -t.""" + warnings = _warnings_for("run", ["run", "case.py", "--targets", "pre_process", "--targets", "simulation"]) + assert len(warnings) == 1 + assert "2 times" in warnings[0] + + +def test_mixed_short_and_long_forms_warn(): + """Mixing the two spellings of one option still drops the first list.""" + warnings = _warnings_for("run", ["run", "case.py", "-t", "pre_process", "--targets", "simulation"]) + assert len(warnings) == 1 + assert "2 times" in warnings[0] + + +def test_equals_form_is_counted(): + """--targets=simulation is the same occurrence, written differently.""" + warnings = _warnings_for("run", ["run", "case.py", "-t", "pre_process", "--targets=simulation"]) + assert len(warnings) == 1 + assert "2 times" in warnings[0] + + +def test_three_repeats_reports_the_real_count(): + warnings = _warnings_for("run", ["run", "case.py", "-t", "pre_process", "-t", "simulation", "-t", "post_process"]) + assert len(warnings) == 1 + assert "3 times" in warnings[0] + + +def test_intended_single_flag_form_is_silent(): + """The documented form -- one flag, several values -- must not warn.""" + assert _warnings_for("run", ["run", "case.py", "-t", "pre_process", "simulation"]) == [] + + +def test_one_target_is_silent(): + """`-t simulation` alone is a legitimate restart invocation.""" + assert _warnings_for("run", ["run", "case.py", "-t", "simulation"]) == [] + + +def test_no_flag_is_silent(): + assert _warnings_for("run", ["run", "case.py"]) == [] + + +def test_repeated_flag_on_build_warns(): + """`targets` reaches build through include_common, not its own argument list.""" + warnings = _warnings_for("build", ["build", "-t", "pre_process", "-t", "simulation"]) + assert len(warnings) == 1 + + +def test_other_list_valued_flags_are_covered(): + """The check is schema-driven, so every nargs="+" flag is covered, not just -t.""" + warnings = _warnings_for("run", ["run", "case.py", "-g", "0", "-g", "1"]) + assert len(warnings) == 1 + assert "--gpus" in warnings[0] + + +def test_a_value_that_looks_like_the_flag_is_not_miscounted(): + """A test UUID or filename equal to a flag name must not inflate the count.""" + assert _warnings_for("run", ["run", "case.py", "-t", "simulation"]) == [] + + +def test_unknown_command_is_silent(): + """A command not in the schema must not raise on the way to argparse's error.""" + assert _warnings_for("not-a-command", ["not-a-command", "-t", "a", "-t", "b"]) == [] + + +def test_multi_value_flags_finds_targets_for_run(): + """The schema walk must see both the command's own args and its common sets.""" + dests = {dest for _, dest in _multi_value_flags("run")} + assert "targets" in dests + assert "gpus" in dests + + +def test_multi_value_flags_skips_scalar_options(): + """Options without nargs cannot be silently overwritten and must not be listed.""" + dests = {dest for _, dest in _multi_value_flags("test")} + # `--from` / `--to` are plain scalars; repeating them loses nothing meaningful. + assert "from" not in dests + assert "to" not in dests + + +def test_parse_still_returns_argparses_result_for_a_repeated_flag(): + """The warning is advisory: parsing behaviour is deliberately left unchanged.""" + argv = ["./mfc.sh", "run", "case.py", "-t", "pre_process", "-t", "simulation"] + with patch.object(sys, "argv", argv), patch("mfc.args.cons"): + args = parse(MFCConfig()) + assert args["targets"] == ["simulation"] + + +def test_warning_is_consistent_with_what_argparse_actually_did(): + """Whenever we warn, argparse must genuinely have kept only the last list.""" + parser, _ = generate_parser(MFC_CLI_SCHEMA, MFCConfig()) + argv = ["run", "case.py", "-t", "pre_process", "-t", "simulation"] + parsed = parser.parse_args(argv) + assert parsed.targets == ["simulation"] + assert _warnings_for("run", argv) != []