Add a return type annotation for return values that should only be used when the function is used as a decorator

Typing
1

There are many examples of functions that return a value solely for use as a decorator whereas when used imperatively, their return value is meant to be ignored. Would it make sense to have a way of marking such functions in order to

  • mitigate warnings by type checkers about unused return values, and
  • catch confusing and unnecessary uses of return values.

Examples of such functions from the standard library include:

  • dataclasses.dataclass
  • _SingleDispatchCallable.register (the result of functools.singledispatch)
  • functools.update_wrapper

and outside the standard library:

  • jax.register_dataclass
  • jax.custom_jvp.defjvp

Examples may be found in other codebases by inspecting the result of pyright with the option reportUnusedCallResult = true.

Ideally, type checkers would allow:

@dataclass
class X: ...

and

class X: ...
dataclass(X)  # Currently PyRight warns on this when reportUnusedCallResult is true

but would warn on

class X: ...
Y = dataclass(X)  # Unnecessary and confusing assignment

Although the type checker wouldn’t be able to suggest this replacement, this should probably be written:

class X: ...
dataclass(X)
Y = X  # Or don't use the name Y at all.

This is similarly true for all of the other functions mentioned above.

Proposed syntax:

    from typing import DecoratorOnly

    def update_wrapper(
        wrapper: Callable[_PWrapper, _RWrapper],
        wrapped: Callable[_PWrapped, _RWrapped],
        ...
    ) -> DecoratorOnly[_Wrapped[_PWrapped, _RWrapped, _PWrapper, _RWrapper]]: ...

The general concept is that many (if not most) functions either have either side effects or produce return values. It is a common error for an inattentive reader to see that a return value is being produced and forget about the side effects. These decorator-only-return (DOR) functions should be called imperatively from imperative code to make their use easier to understand. It would also be nice for type checkers not to produce any warning when these functions are used imperatively.

2

Why use Y at all?

from dataclasses import dataclass

# Note the name. It should not be in SCREAMING_SNAKE_CASE.
class Foo: ...

Foo = dataclass(Foo)  # pyright => fine
                      # mypy => error: Cannot assign to a type
3

Here’s an example of the kind of unnecessary assignment I mentioned above. It’s confusing because it gives the reader the impression that clz and data_clz may be different:

jax/tests/tree_util_test.py
150:  data_clz = dataclasses.dataclass(**kwargs)(clz)  # type: ignore

You can find more example code that may be confusing to some readers:

scipy/scipy/fft/_helper.py
72:next_fast_len = update_wrapper(lru_cache(_helper.good_size), next_fast_len)
138:prev_fast_len = update_wrapper(lru_cache()(_helper.prev_good_size), prev_fast_len)
jax/tests/tree_util_test.py
150:  data_clz = dataclasses.dataclass(**kwargs)(clz)  # type: ignore
flax/flax/struct.py
111:  data_clz = dataclasses.dataclass(**kwargs)(clz)  # type: ignore
equinox/equinox/_better_abstract.py
243:            cls = dataclasses.dataclass(**kwargs)(cls)
251:            cls = dataclasses.dataclass(**kwargs)(cls)
equinox/equinox/_module.py
327:        cls = dataclass(eq=False, repr=False, frozen=True, init=has_dataclass_init)(
4

Related:

5

If you pass slots=True as a kwarg then clz and data_clz will be different.

1 Like
6

Good point. A couple overloads coule ensure that the common case is still protected.

7

I think it would be a bad habit to get into using dataclasses without using the return value because you can be caught out by slots=True. Using the return value ensures that you’re always getting the correct class while using the input class has no such guarantee. I feel like the default should be to warn if you’re not keeping the return value in this case.

Can’t say anything about the other examples though.

8

It seems to me that given the definition of a decorator is that applying a decorator is the same as name = decorator(name), calling a decorator just for the side effects is bad practice (it may work, but it’ll depend on the decorator and so is unreliable).

2 Likes
9

That’s what the default would be if you enable the option in PyRight. And when slots is not true, I think it’s better not to use the return value, especially not with a new name, as in the above code since that hides the side effects of the function.

Yes, but I’m only proposing this annotation for those functions whose effects are entirely side effects, and whose return value is only provided as a decorator convenience. These functions are not “just decorators”. They are commonly used (and misused) in imperative code as well.

10

The examples you give seem to be things I’d consider as primarily decorators. For them, I’d think it’s perfectly reasonable to add a # type: ignore directive if you want to use it in the non-decorator form.

To put this another way, I don’t think you’ve (yet) provided a particularly compelling argument that this is needed.

11

If you look at the original post, I made two arguments:

The warning can be suppressed with a type-ignore (as annoying as that is).

However, the other benefit is flagging unnecessary uses of return values. This has no current substitute except programmer vigilance.

12

I don’t consider name = decorator(name) to be an unnecessary use of the return value. Rather, it’s correct usage of a decorator function. It’s omitting the assignment of the return value that is risky and should be warned about.

2 Likes
13

I guess we just don’t agree on this. To me, this misleads the reader into thinking that the function returns a transformed input, and has no side effects. Omitting the unnecessary assignment makes it obvious that the function has side effects (or else why would you call it). And marking the function with an annotation can tell type checkers not to complain when they see the omitted return value use.

Consider update_wrapper. Even if someone keeps the return value, what should they do with it? Should they take care to assign it to wherever the thing wrapped object is stored? Turns out, you don’t need to do that. By omitting the return value use, the reader doesn’t even have to consider this question.

14

I’m personally not a fan of the reportUnusedCallResult check in pyright. It produces many false positives in proper Python code. For this reason, it’s never enabled by default — even in strict type checking mode. It’s used by very few pyright users, and I’m doubtful that it prevents many real-world bugs. If you find the tradeoffs of this check worthwhile and decide to enable it, then you take on the burden of dealing with the false positives that will inevitably result.

Instead of using a # type: ignore or # pyright: ignore command, you may find it more acceptable to assign the result to a variable named _. This is documentation to both a type checker / linter and humans that you know the function returns a value but you’re purposely discarding it.

I’m in agreement with @pf_moore that adding a new special form to handle this case is not well motivated. I think it would see little or no adoption and would provide relatively little utility.

4 Likes
15

functools.update_wrapper isn’t a great motivating example, as it isn’t intended to be used as a decorator directly (the signature is wrong for that usage).

Instead, it returns the passed in wrapper function so it plays nicely with partial as a tool for defining a decorator (wraps returns a partial object that delegates to update_wrapper)

If I had a do-over, I probably wouldn’t design the API that way (I’d have update_wrapper return None, and wraps return a nested function instead of a partial object), but a however-many-years-old dubious API design decision shouldn’t be helping to drive new language features.