Cache help option generated by help_option_names

This enforce respect of its eagerness

CHANGES.rst

@@ -78,6 +78,9 @@ Unreleased
     -   ``echo_via_pager`` will not ignore ``KeyboardInterrupt`` anymore. This
         allows the user to search for future output of the generator when
         using less and then aborting the program using ctrl-c.
+-   Cache the help option generated by the ``help_option_names`` setting to
+    respect its eagerness. :pr:`2811`
+
 
 Version 8.1.8
 -------------

src/click/core.py

@@ -116,9 +116,16 @@ def iter_params_for_processing(
     invocation_order: cabc.Sequence[Parameter],
     declaration_order: cabc.Sequence[Parameter],
 ) -> 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) -> tuple[bool, float]:
@@ -1011,7 +1018,11 @@ def get_help_option_names(self, ctx: Context) -> list[str]:
         return list(all_names)
 
     def get_help_option(self, ctx: Context) -> Option | None:
-        """Returns the help option object."""
+        """Returns the help option object.
+
+        .. versionchanged:: 8.2.0
+            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:
@@ -1022,14 +1033,21 @@ def show_help(ctx: Context, param: Parameter, value: str) -> None:
                 echo(ctx.get_help(), color=ctx.color)
                 ctx.exit()
 
-        return Option(
-            help_options,
-            is_flag=True,
-            is_eager=True,
-            expose_value=False,
-            callback=show_help,
-            help=_("Show this message and exit."),
-        )
+        # 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 not getattr(self, "_help_option", None):
+            self._help_option = Option(
+                help_options,
+                is_flag=True,
+                is_eager=True,
+                expose_value=False,
+                callback=show_help,
+                help=_("Show this message and exit."),
+            )
+
+        return self._help_option
 
     def make_parser(self, ctx: Context) -> _OptionParser:
         """Creates the underlying option parser for this command."""

tests/test_commands.py

@@ -301,6 +301,144 @@ 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 not result.stderr
+    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 not result.stderr
+    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 not result.stderr
+    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 not result.stderr
+        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 not result.stderr
+    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 not result.stderr
+    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)