Repository: pallets/click
Pull request #2811 (2024-11-24 13:59:05)

Fix eagerness of help option generated by `help_option_names`
When a help option is generated by the use of `help_option_names` setting, it loose its eagerness. This issue has an effect on the order in which parameters (and their callbacks) are processed, and breaks the evaluation order described in the documentation: https://click.palletsprojects.com/en/stable/advanced/#callback-evaluation-order

That's because the utility method `iter_params_for_processing()` is comparing option objects, and the help option is generated on every  `get_help_option()` or `get_params(ctx)` call. This behavior is demonstrated by a unit-test in this PR.

My solution to fix this issue consist in caching locally the auto-generated help option. This solves the issue.

I used this oportunity to clarify some documentation, and unit-test the main method responsible for the ordering of parameters.

This is more or less a follow up of #2563, which has been merged in the upcoming v8.1.8 release.

Checklist:

- [x] Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
- [x] Add or update relevant docs, in the docs folder and in code.
- [x] Add an entry in `CHANGES.rst` summarizing the change and linking to the issue.
- [x] Add `.. versionchanged::` entries in any relevant code docs.
- [x] Run pre-commit hooks and fix any issues.
- [x] Run `pytest` and `tox`, no tests failed.

kdeldycke: Just fixed all the remaining issues.

I was just wondering why `get_help_option()` couldn't be replaced by a `@cached_property` like that:
```python
    @cached_property
    def help_option(self) -> t.Optional["Option"]:
        ctx = self.get_current_context()
        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
        
        return HelpOption(help_options)

    def get_help_option(self, ctx: Context) -> t.Optional["Option"]:
        return self.help_option
```

But I guess there are some edge-cases regarding the context management. And I don't want to propose refactors that are too deep just as 8.1.8 is around the corner and the 8.2.0 is coming soon after.

Anyway, this PR is ready to be merged upstream for the 8.1.8 release.

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index df4e5c7..22b93f8 100644
--- a/CHANGES.rst
+++ b/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
 -------------
diff --git a/src/click/core.py b/src/click/core.py
index c623764..e630501 100644
--- a/src/click/core.py
+++ b/src/click/core.py
@@ -40,6 +40,7 @@ from .utils import PacifyFlushWrapper
 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 @@ class Command(BaseCommand):
         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 @@ class Command(BaseCommand):
         """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."""


Test patch:
diff --git a/tests/test_commands.py b/tests/test_commands.py
index ed9d96f..dcf66ac 100644
--- a/tests/test_commands.py
+++ b/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)