8000 Overloaded function implementation does not accept all possible arguments of signature · Issue #13077 · python/mypy · GitHub
[go: up one dir, main page]
Skip to content

Overloaded function implementation does not accept all possible arguments of signature #13077

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.

Sign up for GitHub

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

Jump to bottom
Closed
adam-grant-hendry opened this issue Jul 5, 2022 · 22 comments
Closed

Overloaded function implementation does not accept all possible arguments of signature #13077

adam-grant-hendry opened this issue Jul 5, 2022 · 22 comments
Labels
bug mypy got something wrong topic-overloads

Comments

@adam-grant-hendry
Copy link
adam-grant-hendry commented Jul 5, 2022 •
edited
Loading

Bug Report

Most likely related to Issue #11004 , but adding here in case there is more going on. The python/typing discussion here helped me understand this is a mypy issue.

To Reproduce

I am using mypy==0.961 exclusively (not any other static type checker; e.g. pyright, pyre, pytyped, etc.) in basicmode (not strict).

I've imported qtpy with PySide6 in python==3.8.10 on Windows 10 x64-bit in VSCode 1.68.1. I am writing a subclass that overwrites QTabWidget.

PySide6 is a binding of the C++ Qt6 library using shiboken6. C++ does not natively support keyword arguments and PySide6 has chosen to support only exporting optional arguments from Qt6 as keyword arguments.

The documentation for PySide6 seems to indicate my stubs below are correct

import typing

from qtpy import QtWidgets, QtGui

class MyTabWidget(QtWidgets.QTabWidget):
    @typing.overload
    def addTab(
        self,
        __widget: QtWidgets.QWidget,
        __arg__2: str,
    ) -> int:
        ...

    @typing.overload
    def addTab(
        self,
        __widget: QtWidgets.QWidget,
        __icon: QtGui.QIcon | QtGui.QPixmap,
        __label: str,
    ) -> int:
        ...

    def addTab(
        self,
        widget: QtWidgets.QWidget,
        icon: QtGui.QIcon | QtGui.QPixmap | None,
        label: str,
    ) -> int:
        # Implementation here

However, mypy complains that the overloaded method doesn't accept all possible arguments:

image

The workaround of specifying the first signature as

@typing.overload
def addTab(
    self,
    __widget: QtWidgets.QWidget,
    icon: None = None,
    __arg__2: str,
) -> int:
    ...

does not work for me because positional arguments cannot come after keyword arguments.

I also tried:

    @typing.overload
    def addTab(  # noqa: D102
        self,
        __widget: QtWidgets.QWidget,
        __arg__2: str,
    ) -> int:
        ...

    @typing.overload
    def addTab(  # noqa: D102
        self,
        __widget: QtWidgets.QWidget,
        __icon: QtGui.QIcon | QtGui.QPixmap | str,
        __label: str,
    ) -> int:
        ...

        def addTab(
        self,
        widget: QtWidgets.QWidget,
        icon: QtGui.QIcon | QtGui.QPixmap | str | None,
        label: str,
    ) -> int:
        # Implementation here

but that didn't work either.

Expected Behavior

Overloads are properly understood.

Actual Behavior

Overloads error that not all possible arguments are accepted.

Your Environment

  • Mypy version used: 0.961
  • Mypy command-line flags:
pyproject.toml 
[tool.mypy]
python_executable = ".venv/Scripts/python.exe"
python_version = "3.8"
disallow_untyped_defs = true
warn_return_any = true
warn_unused_configs = true
warn_unused_ignores = true
warn_redundant_casts = true
show_error_codes = true
no_pretty = true
show_column_numbers = true
plugins = [
    "pydantic.mypy"
]
exclude = [
    'stubs/',
    '[.]venv/',
    'build/',
    'dist/',
    'ci/'
]
fast_module_lookup = true
  • Mypy configuration options from mypy.ini (and other config files): See above
  • Python version used: 3.8.10 x64-bit
  • Operating system and version: Windows 10 x64-bit, Verson 20H2
8000 @adam-grant-hendry adam-grant-hendry added the bug mypy got something wrong label Jul 5, 2022
@adam-grant-hendry
Copy link
Author
adam-grant-hendry commented Jul 5, 2022 •
edited
Loading

I know copyright pyright behaves differently than mypy, so this might not be relevant, but using it with strict mode gave the following ouput (for the typed version above where __icon has str):

Overloaded implementation is not consistent with signature of overload 1
  Type "(self: Self@CollapsibleTabWidget, widget: QWidget, icon: QIcon | QPixmap | str | None, label: str) -> int" cannot be assigned to type "(self: Self@CollapsibleTabWidget, __widget: QWidget, __arg__2: str, /) -> int"
    Function accepts too few positional parameters; expected 4 but received 3
    Keyword parameter "label" is missing in destinationPylance[reportGeneralTypeIssues](https://github.com/microsoft/pylance-release/blob/main/DIAGNOSTIC_SEVERITY_RULES.md#diagnostic-severity-rules)

@erictraut
Copy link

Mypy's error here looks correct to me. The implementation is not consistent with the first overload signature. The first overload indicates that it's OK to pass two position-only arguments (corresponding to __widget and __arg__2), but if you supply only two arguments, you will receive a runtime error because the implementation requires three arguments.

@JelleZijlstra
Copy link
Member

Yes, the error is correct here. I assume by copyright you mean "pyright". Pyright's error is a bit better here but mypy's error message seems clear enough, so I'm closing the issue.

@JelleZijlstra JelleZijlstra closed this as not planned Won't fix, can't repro, duplicate, stale Jul 5, 2022
@adam-grant-hendry
Copy link
Author

@JelleZijlstra @erictraut What is the correct way to type hint this?

@adam-grant-hendry
Copy link
Author

@JelleZijlstra @erictraut For reference, I am trying to be consistent with PySide6

@adam-grant-hendry
Copy link
Author
adam-grant-hendry commented Jul 6, 2022 •
edited
Loading

Yes, the error is correct here. I assume by copyright you mean "pyright". Pyright's error is a bit better here but mypy's error message seems clear enough, so I'm closing the issue.

Ya, typo. I meant pyright. (I corrected this)

@JelleZijlstra
Copy link
Member

The implementation signature should be something like

   def addTab(
        self,
        __widget: QtWidgets.QWidget,
        __icon: QtGui.QIcon | QtGui.QPixmap | str | None,
        __label: str = ...,
    ) -> int:

It may be difficult to type the body of the implementation correctly.

@adam-grant-hendry
Copy link
Author

The implementation signature should be something like

   def addTab(
        self,
        __widget: QtWidgets.QWidget,
        __icon: QtGui.QIcon | QtGui.QPixmap | str | None,
        __label: str = ...,
    ) -> int:

It may be difficult to type the body of the implementation correctly.

@JelleZijlstra That does not work for me either:

image

@JelleZijlstra
Copy link
Member

You're missing = ... for the third argument.

@adam-grant-hendry
Copy link
Author

That gives me

image

@adam-grant-hendry
Copy link
Author

@JelleZijlstra Can I request that you reopen this issue because I'm not seeing how the type hinting can be made to work here.

@adam-grant-hendry
Copy link
Author

@JelleZijlstra FYI, this issue will occur anywhere a Qt class is overloaded that has these kinds of dissimilar numbers and types of arguments.

@JelleZijlstra
Copy link
Member

You still didn't use the code that I put in my previous comment.

@JelleZijlstra
Copy link
Member

I guess I meant the fourth, not the third argument.

@adam-grant-hendry
Copy link
Author

@JelleZijlstra I've tried both ways...perhaps I'm misunderstanding. Could you please show me?

image

image

@adam-grant-hendry
Copy link
Author
adam-grant-hendry commented Jul 6, 2022 •
edited
Loading

@JelleZijlstra Oh you meant the fourth argument. Sorry. Still no dice though 😕
UPDATE: Corrected picture

image

@adam-grant-hendry
Copy link
Author
adam-grant-hendry commented Jul 6, 2022 •
edited
Loading

@JelleZijlstra In addition, seems to not work for either signature AFAICT

image

@JelleZijlstra
Copy link
Member

You need to add it to the implementation, not the overloads. The overloads seem correct, but the implementation needs to be written so it can accept either two or three arguments.

@adam-grant-hendry
Copy link
Author

@JelleZijlstra Are you saying I have to do this, because __label isn't a keyword argument in PySide6:

image

@JelleZijlstra
Copy link
Member

Yes, though I don't see why you can't keep them positional-only. Also, the icon argument doesn't seem to need to accept None.

Also, you can use native positional-only arguments (from PEP 570) instead of the double underscore convention.

@adam-grant-hendry
Copy link
Author
adam-grant-hendry commented Jul 6, 2022 •
edited
Loading

@JelleZijlstra

Also, the icon argument doesn't seem to need to accept None.

Ya, icon is the actual optional argument, so I was trying to see if I could specify it as such.

Also, you can use native positional-only arguments (from PEP 570) instead of the double underscore convention.

From the mypy Python 3 cheat sheet:

# An argument can be declared positional-only by giving it a name
# starting with two underscores:
def quux(__x: int) -> None:
    pass

quux(3)  # Fine
quux(__x=3)  # Error

Was this just a "name mangling trick" until the / PEP 570 syntax could be used? What's the benefit of using one over the other?

the implementation needs to be written so it can accept either two or three arguments.

So is the idea with this:

    @typing.overload
    def addTab(
        self,
        __widget: QtWidgets.QWidget,
        __arg__2: str,
    ) -> int:  # noqa: D102
        ...

    @typing.overload
    def addTab(
        self,
        __widget: QtWidgets.QWidget,
        __icon: QtGui.QIcon | QtGui.QPixmap,
        __label: str,
    ) -> int:  # noqa: D102
        ...

    def addTab(
        self,
        widget: QtWidgets.QWidget,
        icon: QtGui.QIcon | QtGui.QPixmap | str,
        label: str = '',
    ) -> int:

that one still can't call addTab by specifying label as keyword

widget = MyTabWidget(QtWidgets.QWidget(), QtGui.QIcon(), label='Tab')  # This errors; `label` is positional only

and then I would use isinstance checks in the implementation to make sure one is calling it appropriately

if isinstance(icon, str):
    label = icon
else:
    # So I don't have to retype functions called with `icon` here that expect it to be:
    #     QtGui.QIcon | QtGui.QPixmap
    # not
    #     QtGui.QIcon | QtGui.QPixmap | str
    icon = typing.cast(QtGui.QIcon | QtGui.QPixmap, icon)
# rest of implementation

This will be a bit confusing with docstring documentation (since we use pydocstyle with pre-commit), so I'll probably have to add a # noqa: D417 so I can write the (Google-style) documentation properly as

"""Add a widget to the tabs.

This wraps widgets in collapsible containers.

Args:
    widget (QtWidgets.QWidget): Widget to be added.
    icon (QtGui.QIcon | QtGui.QPixmap, optional): The tab icon.
        Defaults to None.
    label (str): The tab label.

Returns:
    int: Index of tab added.
"""

because the docstring signature will have to differ from the actual signature.

@adam-grant-hendry
Copy link
Author

@JelleZijlstra @erictraut I would love your opinions on the above. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug mypy got something wrong topic-overloads
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@JelleZijlstra @erictraut @adam-grant-hendry @AlexWaygood
0