Improve typing of exception handlers

[antonagestam]

Supersedes #1139.

[antonagestam]

This makes sure mypy checks for compatibility with Python 3.6 and onwards, which seems to be the oldest version Starlette supports (?).

[tomchristie]

I feel similarly about this as I do about #1449 (comment) - except more strongly in this case.

There's a point at which typing in Python starts to actively make things worse, and is a hinderance rather than a help to the programmer. To my eyes, this kind of case fits that bill.

I can kinda parse this...

        exception_handlers: typing.Mapping[
             typing.Any,
             typing.Callable[
                 [Request, Exception], typing.Union[Response, typing.Awaitable[Response]]
             ],
         ] = None,

...it's still complex, but I can wrap my head around it.

Once we get into this kind of thing...

_ExcKey = typing.TypeVar(
     "_ExcKey",
     bound=typing.Union[int, typing.Type[Exception]],
     contravariant=True,
 )
 # Breaking out Exception to a bounded TypeVar here allows defining a narrower type in
 # handlers, like HTTPException.
 _HandledException = typing.TypeVar("_HandledException", bound=Exception)
 _ExcHandler = typing.Callable[
     [Request, _HandledException], typing.Union[Response, typing.Awaitable[Response]]
 ]
 _ExcDict = typing.Dict[_ExcKey, _ExcHandler]
 _C = typing.TypeVar("_C", bound=typing.Callable)

exception_handlers: typing.Mapping[_ExcKey, _ExcHandler] = None,

...I'm just a bit lost. And I've got goodness-knows-how-many years of experience with Python and a 20+ year career in software development.

So my instinct on this is that we ought to be holding off on it as much as possible. And more in general, that we should just try to slow down the rate-of-change in Starlette and instead get a grip on narrowing down the existing backlog, and getting into a good sustainable flow.

I know, I'm having to wave the big "nope" banner a lot at the moment, but I don't know any other way to do this.

Perhaps I'm being overly awkward here?

[antonagestam]

@tomchristie While I'm not of the same opinion, I can absolutely sympathize with your feedback and I find your skepticism sound. I do agree that these types are not easily grokable, but they can be inspected and understood independently in steps.

One thing is also that we're bound to legacy syntax, and for some things there are (soon-to-be) proposed syntax that would help immensely. The case with the mapping that you mention, could possibly "soon" (not really soon for starlette which due to backwards compatibility needs to live in the past 😉) be expressed like this:

        exception_handlers: typing.Mapping[
             typing.Any,
             (Request, Exception) -> Response | typing.Awaitable[Response],
         ] = None,

I also feel like the choice to import typing module instead of names from within it hampers readability quite a lot here, the reduce noise below helps readability a lot for me:

        exception_handlers: Mapping[
             Any,
             Callable[
                 [Request, Exception], Union[Response, Awaitable[Response]]
             ],
         ] = None,

And the two applied together:

        exception_handlers: Mapping[
             Any,
             (Request, Exception) -> Response | Awaitable[Response],
         ] = None,

Anyhow, I felt the need to through that just to lift the difference between a complex type, and a "syntactically complex looking type".

And just like with any code, type definitions also gain "grokkability" from being inspected piece-by-piece, so here goes.

Here we're saying the key of our mapping can be integers or exception types. Yes, variance is a niche concept, but it too can be studied and understood independently.

_ExcKey = TypeVar(
    "_ExcKey",
    bound=int | type[Exception],
    contravariant=True,
)

This is the most complex part, arguably. We're saying that the callback is a function whose second argument that is some subtype of Exception. Why can we not just use Exception instead of the typevar? Because that would mean that our user's callbacks would have to accept every kind of exception, instead of allowing them to use an appropriate subclass, like HttpError.

 _HandledException = TypeVar("_HandledException", bound=Exception)
_ExcHandler = (Request, _HandledException) -> Response | Awaitable[Response]

It's necessary to define this outside of the class, as otherwise mypy mandates that the typevars are bound to the class. Pondering on this, it kind of seems like typevars are overloaded to have two different meanings?

 _ExcDict = dict[_ExcKey, _ExcHandler]

Would some of these gain from having more verbose commentary?

---

The real reason I don't agree with you is not because I think these types are simple (look at how many words I've used already on explaining them! 😅). I think in judging these types one need to take into account how they are presented to a user of these interfaces. For me it can be OK that things are complex behind the scenes as long as errors presented to a user can make sense. So what sort of error messages does this added complexity yield?

Using values of wrong type for keys:

app = Starlette(
    exception_handlers={
        "500": handler
    },
)
# error: Value of type variable "_ExcKey" of "Starlette" cannot be "str"  [type-var]

Specifying a handler that takes a requests Request instead of a starlette Request:

from requests import Request

def handler(request: Request, exc: HttpError) -> Response:
    ...

app = Starlette(
    exception_handlers={
        500: handler
    },
)
# error: Dict entry 0 has incompatible type "int": "Callable[[requests.models.Request, HttpError], Response]"; expected "int": "Callable[[starlette.requests.Request, Any], Union[Response, Awaitable[Response]]]"  [dict-item]

Specifying an error type that isn't a subtype of Exception:

class HttpError:
    ...

def handler(request: Request, exc: HttpError) -> Response:
    ...

app = Starlette(
    exception_handlers={
        500: handler
    },
)
# Oops! This passes!

So, it turns out my complex solution didn't catch that case, and so probably needs more refinement.

These error messages might not be the easiest to parse, and so my case here might not be that strong, but at least now the cards are on the table for you to judge! 🙂

In the projects I maintain myself, I use type testing with pytest-mypy-plugins which could have found this type-level bug, and made sure the type checker raises an error for a situation like that.

Perhaps I'm being overly awkward here?

Not at all, the level of care that's being taken for this library is much appreciated! 👍

[tomchristie]

Well - I very much appreciate the thoughtful response here. 😌

About all I can say right now is... interesting trade-off isn't it?

[antonagestam]

@tomchristie Exactly, it's all trade-offs!

One thing though, the updated type for the decorator would be quite nice to get in, as that currently "eats up" the typing efforts of users. It will turn the signatures of their handler functions into Callable[..., Any]. Would you be amenable to accepting smaller chunks of this PR?

[tomchristie]

I also feel like the choice to import typing module instead of names from within it hampers readability quite a lot here, the reduce noise below helps readability a lot for me

I'm okay either ways with that. Obvs consistency within the project is a good plan, but besides that I'd be up for considering either style.

for some things there are (soon-to-be) proposed syntax that would help immensely

That's a neat style you've shown there - it's (probably) more obvious to me than the existing Callable[...] syntax. Is there a PEP for it or some other resource I can take a look at?

[antonagestam]

I actually thought callable syntax was still just discussions on typing-sig, but it turns out PEP 677 exists now. Note the support for async which is relevant for this case.

[adriangb]

So if I understand correctly the idea is that:

class SomeException(Exception):
    pass

def specific_error_handler(request: Request, exc: SomeException) -> Response:
    ...

Starlette(exception_handlers={SomeException: specific_error_handler})  # ok
Starlette(exception_handlers={Exception: specific_error_handler})  # not okay

?

Maybe there's something to be said for forcing a signature:

ExceptionHandler = Callable[[Request, Exception], Union[Response, Awaitable[Response]]]

And then having users either accept the type error, use a # type: ignore when they define the mapping of error handlers or always write their error handlers to accept an Exception type and do an isisntance check or a # type: ignore in the error handler.

Personally, I would just always write my error handler to accept an Exception and raise an error if I got the wrong type (because I likely configured my app incorrectly).

[antonagestam]

@adriangb

So if I understand correctly the idea is that:

No, that's not quite it, the current code actually doesn't connect the key type with the handled type. The issue that the _HandledException typevar solves is that in this example specific_error_handler is not a valid subtype of NaiveHandler, because it doesn't accept any Exception.

NaiveHandler = Callable[
    [Request, Exception], Response | Awaitable[Response]
]

def specific_error_handler(request: Request, exc: SomeException) -> Response:
    ...

Maybe there's something to be said for forcing a signature

I don't think that would be an attractive design. Starlette already does the work of narrowing exceptions by type, it would be weird to require users to do that narrowing again within exception handlers.

[antonagestam]

I broke out the typevar usage for the decorator to this PR, I will remove that from this one.

[adriangb]

I have an alternative proposal:

from typing import TypeVar, Callable, Type, Iterable


ExcType = TypeVar("ExcType", bound=Exception, contravariant=True)

class ExcHandler:
    def __init__(
        self,
        exc: Type[ExcType],
        handler: Callable[[ExcType], None],
    ) -> None:
        ...


class App:
    def __init__(
        self,
        exc_handlers: Iterable[ExcHandler],
    ) -> None:
        ...


def handle_val_err(exc: ValueError) -> None:
    ...


def handle_type_err(exc: TypeError) -> None:
    ...


class MyValueError(ValueError):
    ...


def handle_my_val_error(exc: MyValueError) -> None:
    ...


App(
    exc_handlers=[
        ExcHandler(ValueError, handle_val_err),
        ExcHandler(MyValueError, handle_val_err),
        ExcHandler(TypeError, handle_type_err),
        ExcHandler(ValueError, handle_type_err),   # error
        ExcHandler(ValueError, handle_my_val_error),   # error
    ]
)

https://mypy-play.net/?mypy=latest&python=3.10&gist=80b5629d99e6ace63c8b9743c43ed054

This doesn't fix typing for the existing argument types, but it would allow users to opt into a fully typed alternative.
I think it's pretty straightforward to understand, and as far as I can tell it works with all situations and catches all errors.

[michaeloliverx]

It looks like PEP 677 – Callable Type Syntax has been rejected.

Perhaps we could lean on callback protocols here?

Example:

from typing import Protocol

from starlette.requests import Request
from starlette.responses import Response


class SyncExceptionHandler(Protocol):
    def __call__(self, __request: Request, __exc: type[Exception]) -> Response:
        ...


class AsyncExceptionHandler(Protocol):
    async def __call__(self, __request: Request, __exc: type[Exception]) -> Response:
        ...

[antonagestam]

@michaeloliverx I really only brought up callable syntax to make the point that "syntactical complexity" can obfuscate something that isn't really complex. Callback protocols shouldn't be necessary here, Callable will do just fine. And I suspect as the critique brought up here is that the types look too intimidating for users to understand them, callback protocols really wouldn't help the case.

[adriangb]

I don't think any solution using a mapping will work, including the code currently in this PR. Please take a look at this mypy-play gist and correct me if I am missing something: https://mypy-play.net/?mypy=latest&python=3.10&gist=7c4212ebacbcfeeae7e57ed4dd70c658

The fundamental issue is that collections are (as far as typing is concerned) homogenous containers: you can only have 1 key type and 1 value type, not pairs of key/value types.

I believe the only way to express what we want is to add a level of indirection, i.e. #1456 (comment).
It can be made an optional level of indirection: https://mypy-play.net/?mypy=latest&python=3.10&gist=978441ac3033a4b54aa86c882e0fe06c

All of these proposals are complicated in their own right, there is something nice about just accepting a mapping.

[antonagestam]

@adriangb I think your analysis is correct.

Because of the general pushback on ambitious static typing here, and vagueness on whether or not that would even be considered for merging, I don't feel very enthusiastic about pursuing this. I'll close this PR for now, feel free to use any parts of it as you see fit.

[adriangb]

Hey @antonagestam I just want to personally say that I appreciate your effort in submitting the contribution and understand the frustration of not having clarity on the other end (I am sorry for any part I may have played in this); you're completely justified in deciding to not pursue this further. I hope this experience doesn't dissuade you from future collaboration.