Fix eagerness of help option generated by help_option_names (#2811)

Co-authored-by: Andreas Backx <[email protected]>

Author: kdeldycke

CHANGES.rst

@@ -18,6 +18,9 @@ Unreleased
     forced on Windows. :issue:`2606`.
 -   More robust bash version check, fixing problem on Windows with git-bash.
     :issue:`2638`
+-   Cache the help option generated by the ``help_option_names`` setting to
+    respect its eagerness. :pr:`2811`
+
 
 Version 8.1.7
 -------------

src/click/core.py

@@ -40,6 +40,7 @@
 if t.TYPE_CHECKING:
     import typing_extensions as te
 
+    from .decorators import HelpOption
     from .shell_completion import CompletionItem
 
 F = t.TypeVar("F", bound=t.Callable[..., t.Any])
@@ -116,9 +117,16 @@ def iter_params_for_processing(
     invocation_order: t.Sequence["Parameter"],
     declaration_order: t.Sequence["Parameter"],
 ) -> t.List["Parameter"]:
-    """Given a sequence of parameters in the order as should be considered
-    for processing and an iterable of parameters that exist, this returns
-    a list in the correct order as they should be processed.
+    """Returns all declared parameters in the order they should be processed.
+
+    The declared parameters are re-shuffled depending on the order in which
+    they were invoked, as well as the eagerness of each parameters.
+
+    The invocation order takes precedence over the declaration order. I.e. the
+    order in which the user provided them to the CLI is respected.
+
+    This behavior and its effect on callback evaluation is detailed at:
+    https://click.palletsprojects.com/en/stable/advanced/#callback-evaluation-order
     """
 
     def sort_key(item: "Parameter") -> t.Tuple[bool, float]:
@@ -1223,6 +1231,7 @@ def __init__(
         self.options_metavar = options_metavar
         self.short_help = short_help
         self.add_help_option = add_help_option
+        self._help_option: t.Optional[HelpOption] = None
         self.no_args_is_help = no_args_is_help
         self.hidden = hidden
         self.deprecated = deprecated
@@ -1288,16 +1297,26 @@ def get_help_option(self, ctx: Context) -> t.Optional["Option"]:
         """Returns the help option object.
 
         Unless ``add_help_option`` is ``False``.
+
+        .. versionchanged:: 8.1.8
+            The help option is now cached to avoid creating it multiple times.
         """
         help_options = self.get_help_option_names(ctx)
 
         if not help_options or not self.add_help_option:
             return None
 
-        # Avoid circular import.
-        from .decorators import HelpOption
+        # Cache the help option object in private _help_option attribute to
+        # avoid creating it multiple times. Not doing this will break the
+        # callback odering by iter_params_for_processing(), which relies on
+        # object comparison.
+        if self._help_option is None:
+            # Avoid circular import.
+            from .decorators import HelpOption
+
+            self._help_option = HelpOption(help_options)
 
-        return HelpOption(help_options)
+        return self._help_option
 
     def make_parser(self, ctx: Context) -> OptionParser:
         """Creates the underlying option parser for this command."""

tests/test_commands.py

@@ -305,6 +305,138 @@ def test_group_add_command_name(runner):
     assert result.exit_code == 0
 
 
+@pytest.mark.parametrize(
+    ("invocation_order", "declaration_order", "expected_order"),
+    [
+        # Non-eager options.
+        ([], ["-a"], ["-a"]),
+        (["-a"], ["-a"], ["-a"]),
+        ([], ["-a", "-c"], ["-a", "-c"]),
+        (["-a"], ["-a", "-c"], ["-a", "-c"]),
+        (["-c"], ["-a", "-c"], ["-c", "-a"]),
+        ([], ["-c", "-a"], ["-c", "-a"]),
+        (["-a"], ["-c", "-a"], ["-a", "-c"]),
+        (["-c"], ["-c", "-a"], ["-c", "-a"]),
+        (["-a", "-c"], ["-a", "-c"], ["-a", "-c"]),
+        (["-c", "-a"], ["-a", "-c"], ["-c", "-a"]),
+        # Eager options.
+        ([], ["-b"], ["-b"]),
+        (["-b"], ["-b"], ["-b"]),
+        ([], ["-b", "-d"], ["-b", "-d"]),
+        (["-b"], ["-b", "-d"], ["-b", "-d"]),
+        (["-d"], ["-b", "-d"], ["-d", "-b"]),
+        ([], ["-d", "-b"], ["-d", "-b"]),
+        (["-b"], ["-d", "-b"], ["-b", "-d"]),
+        (["-d"], ["-d", "-b"], ["-d", "-b"]),
+        (["-b", "-d"], ["-b", "-d"], ["-b", "-d"]),
+        (["-d", "-b"], ["-b", "-d"], ["-d", "-b"]),
+        # Mixed options.
+        ([], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]),
+        (["-a"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]),
+        (["-b"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]),
+        (["-c"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-c", "-a"]),
+        (["-d"], ["-a", "-b", "-c", "-d"], ["-d", "-b", "-a", "-c"]),
+        (["-a", "-b"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]),
+        (["-b", "-a"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]),
+        (["-d", "-c"], ["-a", "-b", "-c", "-d"], ["-d", "-b", "-c", "-a"]),
+        (["-c", "-d"], ["-a", "-b", "-c", "-d"], ["-d", "-b", "-c", "-a"]),
+        (["-a", "-b", "-c", "-d"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]),
+        (["-b", "-d", "-a", "-c"], ["-a", "-b", "-c", "-d"], ["-b", "-d", "-a", "-c"]),
+        ([], ["-b", "-d", "-e", "-a", "-c"], ["-b", "-d", "-e", "-a", "-c"]),
+        (["-a", "-d"], ["-b", "-d", "-e", "-a", "-c"], ["-d", "-b", "-e", "-a", "-c"]),
+        (["-c", "-d"], ["-b", "-d", "-e", "-a", "-c"], ["-d", "-b", "-e", "-c", "-a"]),
+    ],
+)
+def test_iter_params_for_processing(
+    invocation_order, declaration_order, expected_order
+):
+    parameters = {
+        "-a": click.Option(["-a"]),
+        "-b": click.Option(["-b"], is_eager=True),
+        "-c": click.Option(["-c"]),
+        "-d": click.Option(["-d"], is_eager=True),
+        "-e": click.Option(["-e"], is_eager=True),
+    }
+
+    invocation_params = [parameters[opt_id] for opt_id in invocation_order]
+    declaration_params = [parameters[opt_id] for opt_id in declaration_order]
+    expected_params = [parameters[opt_id] for opt_id in expected_order]
+
+    assert (
+        click.core.iter_params_for_processing(invocation_params, declaration_params)
+        == expected_params
+    )
+
+
+def test_help_param_priority(runner):
+    """Cover the edge-case in which the eagerness of help option was not
+    respected, because it was internally generated multiple times.
+
+    See: https://github.com/pallets/click/pull/2811
+    """
+
+    def print_and_exit(ctx, param, value):
+        if value:
+            click.echo(f"Value of {param.name} is: {value}")
+            ctx.exit()
+
+    @click.command(context_settings={"help_option_names": ("--my-help",)})
+    @click.option("-a", is_flag=True, expose_value=False, callback=print_and_exit)
+    @click.option(
+        "-b", is_flag=True, expose_value=False, callback=print_and_exit, is_eager=True
+    )
+    def cli():
+        pass
+
+    # --my-help is properly called and stop execution.
+    result = runner.invoke(cli, ["--my-help"])
+    assert "Value of a is: True" not in result.stdout
+    assert "Value of b is: True" not in result.stdout
+    assert "--my-help" in result.stdout
+    assert result.exit_code == 0
+
+    # -a is properly called and stop execution.
+    result = runner.invoke(cli, ["-a"])
+    assert "Value of a is: True" in result.stdout
+    assert "Value of b is: True" not in result.stdout
+    assert "--my-help" not in result.stdout
+    assert result.exit_code == 0
+
+    # -a takes precedence over -b and stop execution.
+    result = runner.invoke(cli, ["-a", "-b"])
+    assert "Value of a is: True" not in result.stdout
+    assert "Value of b is: True" in result.stdout
+    assert "--my-help" not in result.stdout
+    assert result.exit_code == 0
+
+    # --my-help is eager by default so takes precedence over -a and stop
+    # execution, whatever the order.
+    for args in [["-a", "--my-help"], ["--my-help", "-a"]]:
+        result = runner.invoke(cli, args)
+        assert "Value of a is: True" not in result.stdout
+        assert "Value of b is: True" not in result.stdout
+        assert "--my-help" in result.stdout
+        assert result.exit_code == 0
+
+    # Both -b and --my-help are eager so they're called in the order they're
+    # invoked by the user.
+    result = runner.invoke(cli, ["-b", "--my-help"])
+    assert "Value of a is: True" not in result.stdout
+    assert "Value of b is: True" in result.stdout
+    assert "--my-help" not in result.stdout
+    assert result.exit_code == 0
+
+    # But there was a bug when --my-help is called before -b, because the
+    # --my-help option created by click via help_option_names is internally
+    # created twice and is not the same object, breaking the priority order
+    # produced by iter_params_for_processing.
+    result = runner.invoke(cli, ["--my-help", "-b"])
+    assert "Value of a is: True" not in result.stdout
+    assert "Value of b is: True" not in result.stdout
+    assert "--my-help" in result.stdout
+    assert result.exit_code == 0
+
+
 def test_unprocessed_options(runner):
     @click.command(context_settings=dict(ignore_unknown_options=True))
     @click.argument("args", nargs=-1, type=click.UNPROCESSED)