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

Names of positional-only arguments should be allowed to mismatch #1017

Closed
Azureblade3808 opened this issue Mar 5, 2021 · 6 comments
Closed
Labels
bug Something isn't working fixed in next version (main) A fix has been implemented and will appear in an upcoming version

Comments

@Azureblade3808
Copy link

Environment data

  • Language Server version: Pylance 2021.3.0
  • OS and version: Windows 7
  • Python version (& distribution if applicable, e.g. Anaconda): 3.8.5(Anaconda)

Sample code

from __future__ import annotations

from datetime import date as Date
from typing import overload

@overload
def foo(date: Date, /) -> None:
    ...

@overload
def foo(year: int, month: int, day: int, /) -> None:
    ...

"""
Overloaded function implementation is not consistent with signature of overload 1
    Type "(arg0: date | int, arg1: int | None = None, arg2: int | None = None, /) -> None" cannot be assigned
to type "(date: date, /) -> None"
        Parameter name mismatch: "date" versus "arg0" | Pylance(reportGeneralTypeIssues)
"""
def foo(arg0: Date | int, arg1: int | None = None, arg2: int | None = None, /) -> None:
    if isinstance(arg0, Date):
        assert arg1 is None and arg2 is None
        date = arg0
    else:
        assert arg1 is not None and arg2 is not None
        date = Date(arg0, arg1, arg2)

    _ = date

Actual behavior

A warning is emitted.

Expected behavior

The naming in sample code should be considered OK.

Calls to foo would be like either foo(Date(2021, 3, 5)) or foo(2021, 3, 5), and argument names would never get used outside the function implementation. So there would be no conflicts between the mismatching names.

@github-actions github-actions bot added the triage label Mar 5, 2021
@erictraut
Copy link
Contributor

Thanks for the bug report. This will be addressed in the next release.

@erictraut erictraut added bug Something isn't working fixed in next version (main) A fix has been implemented and will appear in an upcoming version and removed triage labels Mar 5, 2021
@jakebailey
Copy link
Member

This issue has been fixed in version 2021.3.1, which we've just released. You can find the changelog here: https://github.com/microsoft/pylance-release/blob/main/CHANGELOG.md#202131-10-march-2021

@Wayofthesin
Copy link

Wayofthesin commented Jul 7, 2021

I think we could have a regression here :(

Environment data

  • Language Server version: Pylance v2021.7.1
  • OS and version: RHEL 7
  • Python version (& distribution if applicable, e.g. Anaconda): 3.6.6 64bit

Sample code

class Foo(Bar):
    @overload
    def find_item(self, product_id: int, name: str) -> requests.Response:
        """Finds item by its name and the product id (UQ)"""

    @overload
    def find_item(self, id: int) -> requests.Response:
        """Finds item by its id"""

    def find_item(self, id_or_product_id: int, name: str = None) -> requests.Response:
        data = {
            'id': id_or_product_id if not name else None,
            'product_id': id_or_product_id if name else None,
            'name': name
        }

        return requests.get(self.url, data=data)

Actual behavior

Overloaded function implementation is not consistent with signature of overload 1
  Type "(self: Foo, id_or_product_id: int, name: str | None = None) -> Response" cannot be assigned to type "(self: Foo, product_id: int, name: str) -> Response"
    Parameter name mismatch: "product_id" versus "id_or_product_id" Pylance(reportGeneralTypeIssues)

Expected behavior

The naming should be considered ok.

Remarks

Calls to Foo.find_item would be either:

foo = Foo()

foo.find_item(60, "my_item") # Find by product ID and name
foo.find_item(183617) # Find by id

One could argue that the overloaded signatures are overlapping and it is massively easy to make a mistake there but the issue still exists in the following example:

class Foo(Bar):
    @overload
    def find_item(self, name: str, product_id: int) -> requests.Response:
        """Finds item by its name and the product id (UQ)"""

    @overload
    def find_item(self, id: int) -> requests.Response:
        """Finds item by its id"""

    def find_item(self, name_or_id: Union[int, str], product_id: int = None) -> requests.Response:
        data = {
            'id': name_or_id if isinstance(name_or_id, int) else None,
            'name': name_or_id if isinstance(name_or_id, str) else None,
            'product_id': product_id
        }

        return requests.get(self.url, data=data)

Complaining about:

Overloaded function implementation is not consistent with signature of overload 1
  Type "(self: Foo, name_or_id: int | str, product_id: int | None = None) -> Response" cannot be assigned to type "(self: Foo, name: str, product_id: int) -> Response"
    Parameter name mismatch: "name" versus "name_or_id" Pylance(reportGeneralTypeIssues)

My type checking mode is set to basic when facing this issue.

@erictraut
Copy link
Contributor

Thanks. This issue is closed. I'd appreciate it if you'd open a new one and provide a minimal, self-contained code sample - one that doesn't depend on classes and types that are not declared in the sample.

@erictraut
Copy link
Contributor

erictraut commented Jul 7, 2021

@Wayofthesin, I think I understand what you're trying to do in your example. Pylance is correct in reporting a bug here. The problem is that the find_item method implement has a parameter called name_or_id. This parameter can be specified by the caller as either positional or by keyword. If it is specified by keyword, your program will crash as it is currently defined.

# This will crash
Foo().find_item(id=3)

If you want to specify that name_or_id can be specified only positionally, you would need to make the following changes:

@overload
def find_item(self, product_id: int, /, name: str) -> requests.Response:
    ...
@overload
def find_item(self, id: int, /) -> requests.Response:
    ...
def find_item(self, id_or_product_id: int, /, name: str = None) -> requests.Response:
    ...

Pylance will no longer emit an error if you make this correction to your code.

@Wayofthesin
Copy link

Yeah I just realized it. Forget the ticket please (facepalm)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed in next version (main) A fix has been implemented and will appear in an upcoming version
Projects
None yet
Development

No branches or pull requests

4 participants