Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typing for click.decorators.pass_context is not covariant and does not support click.Context subclasses #2623

Closed
dwreeves opened this issue Oct 5, 2023 · 4 comments

Comments

@dwreeves
Copy link

dwreeves commented Oct 5, 2023

Context (no pun intended): I am trying to get rich-click up to par with typing standards, and I ran into a minor nuisance with typing functions decorated by pass_context.

An example of rich_click code that does not work on the user's end with mypy would be this:

import click
from rich_click import RichCommand, RichContext

@click.command(cls=RichCommand)
@click.pass_context
def cli(ctx: RichContext) -> None:
    pass

Here, mypy will yell at the user:

error: Argument 1 to "pass_context" has incompatible type "Callable[[RichContext], Any]"; expected "Callable[[Context], Any]"  [arg-type]
note: This is likely because "cli" has named arguments: "ctx". Consider marking them positional-only

However, I do believe this to be valid typing. The RichCommand has its context_class as RichContext, so it is valid to annotate ctx with RichContext. The issue is related to the fact that the function typing by default is contravariant (e.g. it allows for ctx: object as a valid annotation), but it really should be covariant.

The solution seems to be, instead of this typing...

def pass_context(f: t.Callable[te.Concatenate[Context, P], R]) -> t.Callable[P, R]:
    ...

It should have this typing instead:

def pass_context(f: t.Callable[te.Concatenate[C, P], R]) -> t.Callable[P, R]:
    ...

Where C = t.TypeVar("C", bound=Context). This appears to make it so instead of Context being contravariant, it becomes covariant, which is the desired behavior to support subclasses of click.Context.

Implementing this in the actual click source code causes the following mypy error to pop up, however:

src/click/decorators.py:35: error: Argument 1 has incompatible type "Context"; expected "C"  [arg-type]
            return f(get_current_context(), *args, **kwargs)
                     ^~~~~~~~~~~~~~~~~~~~~
Found 3 errors in 3 files (checked 16 source files)

From here it's unclear what to do. I'd rather not pollute the code with a # type: ignore, since this is also a slight abuse of TypeVars. But on the other hand, I do believe this to be a valid behavior to support.

Environment:

  • Python version: 3.10
  • Click version: 8.1.7
@davidism
Copy link
Member

davidism commented Oct 5, 2023

Happy to review a PR, but I do not understand this aspect of typing and so will not be working on it.

That last part may be something you should report to mypy.

@dwreeves
Copy link
Author

dwreeves commented Oct 5, 2023

To be clear I don't know very much about typing here either. 🤷

That last part may be something you should report to mypy.

I am inclined to think it is not a bug and it is intentional behavior, but also I don't fully understand it. I think the way that I am using TypeVars in the above example is a bit of a hack.

I will either research this more or enlist some help.

@dwreeves
Copy link
Author

dwreeves commented Oct 6, 2023

Hmm. After thinking about it a bit, I think this problem is essentially unsolvable as it stands.

A few observations:

  • ctx: object and ctx: click.Context are both valid type annotations; it is sensible for function signatures to be contravariant. Making the function signature covariant means that ctx: object becomes an invalid annotation, which disrupts an odd yet valid annotation (that also is currently supported).
  • The appropriate type for the ctx comes from a function call that evaluates after pass_context does, due to how the decorators are stacked and evaluated. So passing in the correct annotation that the callback is allowed to be contravariant for is a very difficult task, perhaps impossible.
  • For 99.9% of users, typing for subclasses of click.Context is not a concern. The 0.1% of users for whom this matters are probably advanced users and they can figure it out on their own.

For this reason, I'm concluding it's not feasible to make it work and it's also not worth doing anything to attempt to make it work.

There are only two ways to make everything friendly:

  • use Any instead of Context as the annotation. But this is removing a perfectly fine annotation just to support a minor edge case only 0.1% of (advanced) users face. Probably not a good trade.
  • support what typer does, where context is allowed to be passed into the callback by evaluating type annotations at runtime, rather than by using a decorator. issubclass() checks of the annotation allows for trivial covariance. But, that API decision would not comport with click's mantra, which is to be explicit and to not relying on runtime evaluation of type annotations.

On our end in rich-click, what we will be doing is shimming click.pass_context with this:

P = ParamSpec("P")
R = TypeVar("R")


def pass_context(f: Callable[Concatenate[RichContext, P], R]) -> Callable[P, R]:
    # flake8: noqa: D400,D401
    """Marks a callback as wanting to receive the current context
    object as first argument.
    """
    return click_pass_context(f)  # type: ignore[arg-type]

And for our users, this is an appropriate solution. For any other framework developers looking to use a click.Context subclass, they should consider doing something like this as well, or just using type: ignore.

@dwreeves dwreeves closed this as not planned Won't fix, can't repro, duplicate, stale Nov 26, 2023
@davidism
Copy link
Member

davidism commented Dec 2, 2023

Thank you for the detailed updates here and in those linked issues. Dealing with the fact that type checkers can report that our own library's code passes, but still allows users of our library to have issues with our types is a constant source of frustration. If you find anything else, let us know and we can hopefully figure it out.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants