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

Incorrect return type annotation for shortcuts.redirect if using the default argument for permanent in the call #1138

Closed
SebastiaanZ opened this issue Aug 29, 2022 · 0 comments · Fixed by #1140
Labels
bug Something isn't working

Comments

@SebastiaanZ
Copy link
Contributor

Bug report

What's wrong

The first two overload definitions for shortcuts.redirect overlap, leading to an incorrect return type annotation if the default value for the kwarg permanent is used (i.e., calling the function without providing a value for the kwarg permanent yourself).

Normally, mypy produces an error for such an overlap, but it's explicitly ignored on L24:

@overload
def redirect( # type: ignore
to: Union[Callable, str, SupportsGetAbsoluteUrl], *args: Any, permanent: Literal[True] = ..., **kwargs: Any
) -> HttpResponsePermanentRedirect: ...
@overload
def redirect(
to: Union[Callable, str, SupportsGetAbsoluteUrl], *args: Any, permanent: Literal[False] = ..., **kwargs: Any
) -> HttpResponseRedirect: ...
@overload
def redirect(
to: Union[Callable, str, SupportsGetAbsoluteUrl], *args: Any, permanent: bool = ..., **kwargs: Any
) -> Union[HttpResponseRedirect, HttpResponsePermanentRedirect]: ...

Here's a related mypy issue with an explanation for why this behaviour occurs.

How to reproduce

Adding this line to the check_redirect_return_annotation test case in tests/typecheck/test_shortcuts.yml produces a failing test case:

        reveal_type(redirect(to = '')) # N: Revealed type is "django.http.response.HttpResponseRedirect"

The full test case could now be:

-   case: check_redirect_return_annotation
    main: |
        from django.shortcuts import redirect
        reveal_type(redirect(to = '', permanent = True)) # N: Revealed type is "django.http.response.HttpResponsePermanentRedirect"
        reveal_type(redirect(to = '', permanent = False)) # N: Revealed type is "django.http.response.HttpResponseRedirect"
        reveal_type(redirect(to = '')) # N: Revealed type is "django.http.response.HttpResponseRedirect"

        var = True
        reveal_type(redirect(to = '', permanent = var)) # N: Revealed type is "Union[django.http.response.HttpResponseRedirect, django.http.response.HttpResponsePermanentRedirect]"

This will now produce a failing test:

tests/typecheck/test_shortcuts.yml::check_redirect_return_annotation FAILED

=============================================================================================================================== FAILURES ================================================================================================================================ 
___________________________________________________________________________________________________________________ check_redirect_return_annotation ____________________________________________________________________________________________________________________ 
C:\Users\sze30522\repositories\django-stubs\tests\typecheck\test_shortcuts.yml:57:
E   pytest_mypy_plugins.utils.TypecheckAssertionError: Invalid output: 
E   Actual:
E     ...
E     main:4: note: Revealed type is "django.http.response.HttpResponsePermanentRedirect" (diff)
E     ...
E   Expected:
E     ...
E     main:4: note: Revealed type is "django.http.response.HttpResponseRedirect" (diff)
E     ...
E   Alignment of first line difference:
E     E: ...ttp.response.HttpResponseRedirect"
E     A: ...ttp.response.HttpResponsePermanentRedirect"
E                                    ^
======================================================================================================================== short test summary info ======================================================================================================================== 
FAILED tests/typecheck/test_shortcuts.yml::check_redirect_return_annotation -

In addition, removing the # type: ignore from L24 also reveals the problem with the overlap if mypy is now run:

c:\users\...\repositories\django-stubs\django-stubs\shortcuts.pyi:24: error: Overloaded function signatures 1 and 2 overlap with incompatible return types

How is that should be

As the default value for permanent in the redirect function signature is False, the return type should be django.http.response.HttpResponseRedirect.

To fix this, permanent: Literal[True] = ... can be changed to permanent: Literal[True] in the first overload. This also removes the overlap, which means the # type: ignore can now also be removed.

This results in:

@overload
def redirect(
    to: Union[Callable, str, SupportsGetAbsoluteUrl], *args: Any, permanent: Literal[True], **kwargs: Any
) -> HttpResponsePermanentRedirect: ...
@overload
def redirect(
    to: Union[Callable, str, SupportsGetAbsoluteUrl], *args: Any, permanent: Literal[False] = ..., **kwargs: Any
) -> HttpResponseRedirect: ...
@overload
def redirect(
    to: Union[Callable, str, SupportsGetAbsoluteUrl], *args: Any, permanent: bool = ..., **kwargs: Any
) -> Union[HttpResponseRedirect, HttpResponsePermanentRedirect]: ...

The logic here is that the first overload, with a Literal[True] can never be trigger without providing an argument (i.e., when falling back to the default argument). This means that this overload should not specify the = ... to say that it should match situations in which the default argument is used.

The second overload now matched both an explicit permanent=False, as well as the default argument (which is also False). The third overload still matched the Union case defined in the test.

System information

  • OS: Windows 10 & Ubuntu 22.04
  • python version: 3.9 & 3.10
  • django version: 3.2, 4.0, 4.1
  • mypy version: 0.971
  • django-stubs version: 1.12.0 (master)
  • django-stubs-ext version: 0.6.0 (master)
@SebastiaanZ SebastiaanZ added the bug Something isn't working label Aug 29, 2022
SebastiaanZ added a commit to SebastiaanZ/django-stubs that referenced this issue Aug 30, 2022
The return type for calling `shorcuts.render` without providing a value
for the `permanent` kwarg was `HttpResponsePermanentRedirect`, while it
should be `HttpResponseRedirect`.

The reason is that the first two overloads of the type stub overlap for
the case of using the default argument. While `mypy` does issue an error
for this, it was previously ignored with the `# type: ignore` comment.

As the first overload annotates the function as having the return type
`HttpResponsePermanentRedirect`, this would make mypy assume that the
return type is that instead of `HttpResponseRedirect`.

Since calling `django.shortcuts.redirect` without providing an argument
for `permanent` is the same as calling it with a `Literal[False]`, as
the default value is a `False`, we can improve the stub by only
specifying the option to use the default argument (`= ...`) in the
second overload. This also removes the overlap in stub definitions,
meaning that the `# type: ignore` can now be removed.

This commit fixes typeddjango#1138.
sobolevn pushed a commit that referenced this issue Aug 30, 2022
The return type for calling `shorcuts.render` without providing a value
for the `permanent` kwarg was `HttpResponsePermanentRedirect`, while it
should be `HttpResponseRedirect`.

The reason is that the first two overloads of the type stub overlap for
the case of using the default argument. While `mypy` does issue an error
for this, it was previously ignored with the `# type: ignore` comment.

As the first overload annotates the function as having the return type
`HttpResponsePermanentRedirect`, this would make mypy assume that the
return type is that instead of `HttpResponseRedirect`.

Since calling `django.shortcuts.redirect` without providing an argument
for `permanent` is the same as calling it with a `Literal[False]`, as
the default value is a `False`, we can improve the stub by only
specifying the option to use the default argument (`= ...`) in the
second overload. This also removes the overlap in stub definitions,
meaning that the `# type: ignore` can now be removed.

This commit fixes #1138.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging a pull request may close this issue.

1 participant